[FFmpeg-devel] lavc,lavf: add libdavs2 decoder

Add Chinese AVS2 video decoder, FFmpeg can make use of the libdavs2 library for AVS2 decoding.

Signed-off-by: hwren <hwrenx@126.com>
---
 Changelog               |   1 +
 configure               |   6 ++
 doc/general.texi        |   8 ++
 libavcodec/Makefile     |   1 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/avcodec.h    |   1 +
 libavcodec/codec_desc.c |   7 ++
 libavcodec/libdavs2.c   | 216 ++++++++++++++++++++++++++++++++++++++++++++++++
 libavformat/riff.c      |   1 +
 9 files changed, 242 insertions(+)
 create mode 100644 libavcodec/libdavs2.c

On 26/05/18 05:41, hwren wrote:
> Add Chinese AVS2 video decoder, FFmpeg can make use of the libdavs2 library for AVS2 decoding.
> 
> Signed-off-by: hwren <hwrenx@126.com>
> ---
>  Changelog               |   1 +
>  configure               |   6 ++
>  doc/general.texi        |   8 ++
>  libavcodec/Makefile     |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h    |   1 +
>  libavcodec/codec_desc.c |   7 ++
>  libavcodec/libdavs2.c   | 216 ++++++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/riff.c      |   1 +
>  9 files changed, 242 insertions(+)
>  create mode 100644 libavcodec/libdavs2.c
> 
> diff --git a/Changelog b/Changelog
> index 3d25564..0f97679 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -9,6 +9,7 @@ version <next>:
>  - aderivative and aintegral audio filters
>  - pal75bars and pal100bars video filter sources
>  - support mbedTLS based TLS
> +- AVS2 video decoder

Note the library that is used here.  (This isn't a native decoder, it requires an external library.)

>  
>  
>  version 4.0:
> diff --git a/configure b/configure
> index 09ff0c5..9aec00d 100755
> --- a/configure
> +++ b/configure
> @@ -276,6 +276,7 @@ External library support:
>    --enable-libx264         enable H.264 encoding via x264 [no]
>    --enable-libx265         enable HEVC encoding via x265 [no]
>    --enable-libxavs         enable AVS encoding via xavs [no]
> +  --enable-libdavs2        enable AVS2 decoding via davs2 [no]

This list should stay in alphabetical order.

>    --enable-libxcb          enable X11 grabbing using XCB [autodetect]
>    --enable-libxcb-shm      enable X11 grabbing shm communication [autodetect]
>    --enable-libxcb-xfixes   enable X11 grabbing mouse rendering [autodetect]
> @@ -1641,6 +1642,7 @@ EXTERNAL_LIBRARY_GPL_LIST="
>      libx264
>      libx265
>      libxavs
> +    libdavs2

Likewise this one, and more below.

>      libxvid
>  "
>  
> @@ -3095,6 +3097,7 @@ libx264rgb_encoder_deps="libx264 x264_csp_bgr"
>  libx264rgb_encoder_select="libx264_encoder"
>  libx265_encoder_deps="libx265"
>  libxavs_encoder_deps="libxavs"
> +libdavs2_decoder_deps="libdavs2"
>  libxvid_encoder_deps="libxvid"
>  libzvbi_teletext_decoder_deps="libzvbi"
>  vapoursynth_demuxer_deps="vapoursynth"
> @@ -6104,6 +6107,9 @@ enabled libx264           && { check_pkg_config libx264 x264 "stdint.h x264.h" x
>  enabled libx265           && require_pkg_config libx265 x265 x265.h x265_api_get &&
>                               require_cpp_condition x265.h "X265_BUILD >= 68"
>  enabled libxavs           && require libxavs "stdint.h xavs.h" xavs_encoder_encode "-lxavs $pthreads_extralibs $libm_extralibs"
> +enabled libdavs2          && require_pkg_config libdavs2 davs2 "stdint.h davs2.h" davs2_decoder_decode &&

That shouldn't need stdint.h?  If it does, then the library should probably be fixed to include it correctly.

> +                          { check_cpp_condition davs2 davs2.h "DAVS2_BUILD >= 12" ||
> +                            die "ERROR: libdavs2 version must be >= 12." ; }

Can you check the version with pkgconfig instead?

>  enabled libxvid           && require libxvid xvid.h xvid_global -lxvidcore
>  enabled libzimg           && require_pkg_config libzimg "zimg >= 2.7.0" zimg.h zimg_get_api_version
>  enabled libzmq            && require_pkg_config libzmq libzmq zmq.h zmq_ctx_new
> diff --git a/doc/general.texi b/doc/general.texi
> index 2583006..d3c1503 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -17,6 +17,14 @@ for more formats. None of them are used by default, their use has to be
>  explicitly requested by passing the appropriate flags to
>  @command{./configure}.
>  
> +@section libdavs2
> +
> +FFmpeg can make use of the libdavs2 library for AVS2 decoding.
> +
> +Go to @url{https://github.com/pkuvcl/davs2} and follow the instructions for
> +installing the library. Then pass @code{--enable-libdavs2} to configure to
> +enable it.
> +
>  @section Alliance for Open Media libaom
>  
>  FFmpeg can make use of the libaom library for AV1 decoding.
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 3ab071a..e85b74c 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -985,6 +985,7 @@ OBJS-$(CONFIG_LIBX262_ENCODER)            += libx264.o
>  OBJS-$(CONFIG_LIBX264_ENCODER)            += libx264.o
>  OBJS-$(CONFIG_LIBX265_ENCODER)            += libx265.o
>  OBJS-$(CONFIG_LIBXAVS_ENCODER)            += libxavs.o
> +OBJS-$(CONFIG_LIBDAVS2_DECODER)           += libdavs2.o
>  OBJS-$(CONFIG_LIBXVID_ENCODER)            += libxvid.o
>  OBJS-$(CONFIG_LIBZVBI_TELETEXT_DECODER)   += libzvbi-teletextdec.o ass.o
>  
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 7b7a8c7..6103081 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -705,6 +705,7 @@ extern AVCodec ff_libx264_encoder;
>  extern AVCodec ff_libx264rgb_encoder;
>  extern AVCodec ff_libx265_encoder;
>  extern AVCodec ff_libxavs_encoder;
> +extern AVCodec ff_libdavs2_decoder;
>  extern AVCodec ff_libxvid_encoder;
>  extern AVCodec ff_libzvbi_teletext_decoder;
>  
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index fb0c6fa..a8cbc7b 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -303,6 +303,7 @@ enum AVCodecID {
>      AV_CODEC_ID_KMVC,
>      AV_CODEC_ID_FLASHSV,
>      AV_CODEC_ID_CAVS,
> +    AV_CODEC_ID_AVS2,

This needs to go at the end of the list.  (Changing existing values breaks ABI.)

This change and the one to add the descriptor should be a separate patch, and include an APIchanges entry because they modify an installed header.

>      AV_CODEC_ID_JPEG2000,
>      AV_CODEC_ID_VMNC,
>      AV_CODEC_ID_VP5,
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 79552a9..9b50dae 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -651,6 +651,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .props     = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_REORDER,
>      },
>      {
> +        .id        = AV_CODEC_ID_AVS2,
> +        .type      = AVMEDIA_TYPE_VIDEO,
> +        .name      = "avs2",
> +        .long_name = NULL_IF_CONFIG_SMALL("Chinese AVS2 (Audio Video Standar) (AVS2-P2, JiZhun profile)"),
> +        .props     = AV_CODEC_PROP_LOSSY,
> +    },
> +    {
>          .id        = AV_CODEC_ID_JPEG2000,
>          .type      = AVMEDIA_TYPE_VIDEO,
>          .name      = "jpeg2000",
> diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c
> new file mode 100644
> index 0000000..3537f40
> --- /dev/null
> +++ b/libavcodec/libdavs2.c
> @@ -0,0 +1,216 @@
> +/*
> + * AVS2 decoding using the davs2 library
> + *
> + * Copyright (C) 2018 Yiqun Xu, <yiqun.xu@vipl.ict.ac.cn>
> + *                    Falei Luo, <falei.luo@gmail.com>
> + *                    Huiwen Ren, <hwrenx@gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/avassert.h"
> +#include "libavutil/common.h"
> +#include "libavutil/avutil.h"
> +#include "avcodec.h"
> +#include "libavutil/imgutils.h"
> +#include "internal.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>

These two headers aren't used?

> +#include <davs2.h>
> +
> +typedef struct DAVS2Context {
> +    AVCodecContext *avctx;
> +    void *dec_handle;
> +    int got_seqhdr;
> +
> +    void *decoder;
> +
> +    AVFrame *frame;
> +    davs2_param_t    param;      // decoding parameters
> +    davs2_packet_t   packet;     // input bitstream
> +    int ret; 
> +
> +    int img_width[3];
> +    int img_height[3];
> +    int out_flag;
> +    int decoded_frames;
> +    

Don't add trailing spaces.  (Also more below.)

> +    davs2_picture_t  out_frame;  // output data, frame data
> +    davs2_seq_info_t headerset;  // output data, sequence header
> +
> +}DAVS2Context;

Some of the fields of this structure look like they should be on the stack in functions below instead.

> +
> +static int ff_davs2_init(AVCodecContext *avctx)
> +{
> +    DAVS2Context *cad = avctx->priv_data;
> +
> +    /* init the decoder */
> +    cad->param.threads      = 0;
> +    cad->param.i_info_level = 0;
> +    cad->decoder = davs2_decoder_open(&cad->param);
> +    avctx->flags |= AV_CODEC_FLAG_TRUNCATED;  
> +
> +    av_log(avctx, AV_LOG_WARNING, "[davs2] decoder created. 0x%llx\n", cad->decoder);

This shouldn't be a warning.  Maybe VERBOSE?

void* can't be printed with "%llx", if you want the pointer in this log line then "%p".

> +    return 0;
> +}
> +
> +/* ---------------------------------------------------------------------------
> + */
> +static int DumpFrames(AVCodecContext *avctx, davs2_picture_t *pic, davs2_seq_info_t *headerset, AVFrame *frm)
> +{
> +    DAVS2Context *cad = avctx->priv_data;
> +    avctx->flags |= AV_CODEC_FLAG_TRUNCATED;

Why this line?

> +
> +    if (headerset == NULL) {

It's a pointer, just "!headerset".

> +        return 0;
> +    }
> +
> +    if (pic == NULL || pic->ret_type == DAVS2_GOT_HEADER) {
> +        avctx->frame_size   = (headerset->horizontal_size * headerset->vertical_size * 3 * headerset->bytes_per_sample) >> 1;

frame_size isn't used for video.

> +        avctx->coded_width  = headerset->horizontal_size;
> +        avctx->coded_height = headerset->vertical_size;
> +        avctx->width        = headerset->horizontal_size;
> +        avctx->height       = headerset->vertical_size;

Must the coded size and actual size really be equal?

> +        avctx->pix_fmt      = (headerset->output_bitdepth == 10 ? AV_PIX_FMT_YUV420P10 : AV_PIX_FMT_YUV420P);
> +        avctx->framerate.num = (int)(headerset->frame_rate);
> +        avctx->framerate.den = 1;

frame_rate is a float in the API - maybe use av_d2q() to make this so that it supports non-integer rates?

> +        return 0;
> +    }
> +
> +    const int bytes_per_sample = pic->bytes_per_sample;
> +    int i;

Mixed code and declarations are not allowed.

> +
> +    for (i = 0; i < 3; ++i) {
> +        int size_plane = pic->width[i] * pic->lines[i] * bytes_per_sample;
> +        frm->buf[i]      = av_buffer_alloc(size_plane);

Allocations must be checked.

> +        frm->data[i]     = frm->buf[i]->data;
> +        frm->linesize[i] = pic->width[i] * bytes_per_sample;
> +        memcpy(frm->data[i], pic->planes[i], size_plane);

This is the same format as libavcodec wants, so is there any way to keep a reference to it so we can avoid this copy?

> +    }
> +
> +    frm->width  = cad->headerset.horizontal_size;
> +    frm->height = cad->headerset.vertical_size;
> +    frm->pts    = cad->out_frame.pts;
> +
> +    frm->key_frame = 1;
> +    frm->pict_type = AV_PICTURE_TYPE_I;

I don't think this is an intra-only codec.  If you don't have any way to determine the picture type then just don't set these fields.

> +    frm->format = avctx->pix_fmt;
> +    cad->out_flag = 1;
> +
> +    cad->decoded_frames++;
> +    return 1;
> +}
> +
> +static int ff_davs2_end(AVCodecContext *avctx)
> +{
> +    DAVS2Context *cad = avctx->priv_data;
> +
> +    /* close the decoder */
> +    if (cad->decoder != NULL) {

"if (cad->decoder) {"

> +        davs2_decoder_close(cad->decoder);
> +        av_log(avctx, AV_LOG_WARNING, "[davs2] decoder destroyed. 0x%llx; frames %d\n", cad->decoder, cad->decoded_frames);        

Same comments as on the above log line.

> +        cad->decoder = NULL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int davs2_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPacket *avpkt)
> +{
> +    DAVS2Context *cad = avctx->priv_data;
> +    int buf_size       = avpkt->size;
> +    const uint8_t *buf_ptr = avpkt->data;
> +    AVFrame *frm = data;
> +
> +    *got_frame = 0;
> +    cad->out_flag=0;
> +    cad->frame = frm;

What is this trying to do?  cad->frame is never referenced anywhere else.

> +    avctx->flags |= AV_CODEC_FLAG_TRUNCATED;
> +
> +    if (buf_size == 0) {
> +        cad->packet.data = buf_ptr;
> +        cad->packet.len  = buf_size;
> +        cad->packet.pts  = avpkt->pts;
> +        cad->packet.dts  = avpkt->dts;
> +
> +        for (;;) {
> +            cad->ret = davs2_decoder_flush(cad->decoder, &cad->headerset, &cad->out_frame);
> +    
> +            if (cad->ret < 0) {
> +                return 0;
> +            }
> +    
> +            if (cad->out_frame.ret_type != DAVS2_DEFAULT) {
> +                *got_frame = DumpFrames(avctx, &cad->out_frame, &cad->headerset, frm);
> +                davs2_decoder_frame_unref(cad->decoder, &cad->out_frame);
> +            }
> +            if(cad->out_flag == 1) { 
> +                break;

out_flag seems to have exactly the same meaning as *got_frame - maybe you can remove it?

> +            }
> +        }
> +        return 0;
> +    } else {
> +        for(; buf_size > 0;) {
> +            int len = buf_size;   // for API-3, pass all data in
> +    
> +            cad->packet.marker = 0;
> +            cad->packet.data = buf_ptr;
> +            cad->packet.len  = len;
> +            cad->packet.pts  = avpkt->pts;
> +            cad->packet.dts  = avpkt->dts;
> +
> +            len = davs2_decoder_decode(cad->decoder, &cad->packet, &cad->headerset, &cad->out_frame);
> +
> +            if (cad->out_frame.ret_type != DAVS2_DEFAULT) {
> +                *got_frame = DumpFrames(avctx, &cad->out_frame, &cad->headerset, frm);
> +                davs2_decoder_frame_unref(cad->decoder, &cad->out_frame);
> +            }
> +    
> +            if (len < 0) {
> +                av_log(NULL, AV_LOG_ERROR, "An decoder error counted\n");

The log line should be the AVCodecContext as the logging context.

Can you get any more detail about the error?

> +
> +                return -1;

Does this require any cleanup?

It should return an AVERROR() code rather than -1.

> +            }
> +    
> +            buf_ptr     += len;
> +            buf_size    -= len;

Can you explain how the consuming partial packets works?  It looks like if this consumes part of a packet and they returns a frame, the rest of the packet will be discarded (because the next call will be given a new packet).

> +
> +            if(cad->out_flag == 1) {
> +                break;
> +            }
> +        }
> +    }
> +
> +    buf_size = (buf_ptr - avpkt->data);
> +
> +    return buf_size;
> +}
> +
> +AVCodec ff_libdavs2_decoder = {
> +    .name           = "libdavs2",
> +    .long_name      = NULL_IF_CONFIG_SMALL("Decoder for Chinese AVS2"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_AVS2,
> +    .priv_data_size = sizeof(DAVS2Context),
> +    .init           = ff_davs2_init,
> +    .close          = ff_davs2_end,
> +    .decode         = davs2_decode_frame,
> +    .capabilities   =  AV_CODEC_CAP_DELAY,//AV_CODEC_CAP_DR1 |
> +    .pix_fmts       = (const enum AVPixelFormat[]) { AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV420P10,
> +                                                     AV_PIX_FMT_NONE },
> +};
> diff --git a/libavformat/riff.c b/libavformat/riff.c
> index 8911725..4153372 100644
> --- a/libavformat/riff.c
> +++ b/libavformat/riff.c
> @@ -369,6 +369,7 @@ const AVCodecTag ff_codec_bmp_tags[] = {
>      { AV_CODEC_ID_ZMBV,         MKTAG('Z', 'M', 'B', 'V') },
>      { AV_CODEC_ID_KMVC,         MKTAG('K', 'M', 'V', 'C') },
>      { AV_CODEC_ID_CAVS,         MKTAG('C', 'A', 'V', 'S') },
> +    { AV_CODEC_ID_AVS2,         MKTAG('A', 'V', 'S', '2') },
>      { AV_CODEC_ID_JPEG2000,     MKTAG('m', 'j', 'p', '2') },
>      { AV_CODEC_ID_JPEG2000,     MKTAG('M', 'J', '2', 'C') },
>      { AV_CODEC_ID_JPEG2000,     MKTAG('L', 'J', '2', 'C') },
> 

This hunk looks like it should be a separate patch?

- Mark

Thanks for your patience, we've amended some of our patches add resent them.







At 2018-05-26 22:28:27, "Mark Thompson" <sw@jkqxz.net> wrote:
>On 26/05/18 05:41, hwren wrote:
>> Add Chinese AVS2 video decoder, FFmpeg can make use of the libdavs2 library for AVS2 decoding.
>> 
>> Signed-off-by: hwren <hwrenx@126.com>
>> ---
>> [...] 
>> +static int DumpFrames(AVCodecContext *avctx, davs2_picture_t *pic, davs2_seq_info_t *headerset, AVFrame *frm) 
>> +{ 
>> + DAVS2Context *cad = avctx->priv_data; 
>> + avctx->flags |= AV_CODEC_FLAG_TRUNCATED; 
> 
>Why this line?

we won't send a complete frame to our decoder, so we need to chage the flag. if not, we may get error info.

> 
>> [...] 
>> + frm->data[i] = frm->buf[i]->data; 
>> + frm->linesize[i] = pic->width[i] * bytes_per_sample; 
>> + memcpy(frm->data[i], pic->planes[i], size_plane); 
> 
>This is the same format as libavcodec wants, so is there any way to keep a reference to it so we can avoid this copy?  

We've tried to make it, while it did not work well. May avoid this copy in the future.

> 
>> [...] 
>> + buf_ptr += len; 
>> + buf_size -= len; 
> 
>Can you explain how the consuming partial packets works? It looks like if this consumes part of a packet and they returns a frame, the rest of the packet will be discarded (because the next call will be given a new packet).

Once we decode a frame, indeed, there are still some data left in the packet. so we just move the pointer and decode the next part (decode until our flag turns or out of buf_size range).

> 
>>[...]