[FFmpeg-devel] lavc,lavf: add libxavs2 encoder

Add Chinese AVS2 video encoder, FFmpeg can make use of the libxavs2 library for AVS2 encoding.

Signed-off-by: hwren <hwrenx@126.com>
---
 Changelog                |   2 +-
 configure                |   6 +
 doc/general.texi         |   8 ++
 libavcodec/Makefile      |   1 +
 libavcodec/allcodecs.c   |   1 +
 libavcodec/libxavs2.c    | 347 +++++++++++++++++++++++++++++++++++++++++++++++
 libavformat/Makefile     |   1 +
 libavformat/allformats.c |   1 +
 libavformat/rawenc.c     |  13 ++
 9 files changed, 379 insertions(+), 1 deletion(-)
 create mode 100644 libavcodec/libxavs2.c

On 26/05/18 07:08, hwren wrote:
> Add Chinese AVS2 video encoder, FFmpeg can make use of the libxavs2 library for AVS2 encoding.
> 
> Signed-off-by: hwren <hwrenx@126.com>
> ---
>  Changelog                |   2 +-
>  configure                |   6 +
>  doc/general.texi         |   8 ++
>  libavcodec/Makefile      |   1 +
>  libavcodec/allcodecs.c   |   1 +
>  libavcodec/libxavs2.c    | 347 +++++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/rawenc.c     |  13 ++
>  9 files changed, 379 insertions(+), 1 deletion(-)
>  create mode 100644 libavcodec/libxavs2.c
> 
> diff --git a/Changelog b/Changelog
> index 0f97679..87751d9 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -9,7 +9,7 @@ version <next>:
>  - aderivative and aintegral audio filters
>  - pal75bars and pal100bars video filter sources
>  - support mbedTLS based TLS
> -- AVS2 video decoder
> +- AVS2 video encoder and decoder

Note the library used.

>  
>  
>  version 4.0:
> diff --git a/configure b/configure
> index 9aec00d..44c7b60 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-libxavs2        enable AVS2 encoding via xavs2 [no]
>    --enable-libdavs2        enable AVS2 decoding via davs2 [no]
>    --enable-libxcb          enable X11 grabbing using XCB [autodetect]
>    --enable-libxcb-shm      enable X11 grabbing shm communication [autodetect]
> @@ -1642,6 +1643,7 @@ EXTERNAL_LIBRARY_GPL_LIST="
>      libx264
>      libx265
>      libxavs
> +    libxavs2
>      libdavs2
>      libxvid
>  "
> @@ -3097,6 +3099,7 @@ libx264rgb_encoder_deps="libx264 x264_csp_bgr"
>  libx264rgb_encoder_select="libx264_encoder"
>  libx265_encoder_deps="libx265"
>  libxavs_encoder_deps="libxavs"
> +libxavs2_encoder_deps="libxavs2"
>  libdavs2_decoder_deps="libdavs2"
>  libxvid_encoder_deps="libxvid"
>  libzvbi_teletext_decoder_deps="libzvbi"
> @@ -6107,6 +6110,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 libxavs2          && require_pkg_config libxavs2 xavs2 "stdint.h xavs2.h" xavs2_api_get &&
> +                          { check_cpp_condition xavs2 xavs2.h "XAVS2_BUILD >= 12" ||
> +                            die "ERROR: xavs2 version must be >= 12." ; }

Same comments as on libdavs2 - stdint.h, version from pkgconfig?

>  enabled libdavs2          && require_pkg_config libdavs2 davs2 "stdint.h davs2.h" davs2_decoder_decode &&
>                            { check_cpp_condition davs2 davs2.h "DAVS2_BUILD >= 12" ||
>                              die "ERROR: libdavs2 version must be >= 12." ; }
> diff --git a/doc/general.texi b/doc/general.texi
> index d3c1503..c80d610 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -25,6 +25,14 @@ 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 libxavs2
> +
> +FFmpeg can make use of the libxavs2 library for AVS2 encoding.
> +
> +Go to @url{https://github.com/pkuvcl/xavs2} and follow the instructions for
> +installing the library. Then pass @code{--enable-libxavs2} 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 e85b74c..88b7b84 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_LIBXAVS2_ENCODER)           += libxavs2.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 6103081..ca776b0 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_libxavs2_encoder;
>  extern AVCodec ff_libdavs2_decoder;
>  extern AVCodec ff_libxvid_encoder;
>  extern AVCodec ff_libzvbi_teletext_decoder;
> diff --git a/libavcodec/libxavs2.c b/libavcodec/libxavs2.c
> new file mode 100644
> index 0000000..b9197ce
> --- /dev/null
> +++ b/libavcodec/libxavs2.c
> @@ -0,0 +1,347 @@
> +/*
> + * AVS2 encoding using the xavs2 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 <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <ctype.h>
> +#include <math.h>
> +#include <stdint.h>
> +#include <float.h>
> +#include <xavs2.h>
> +
> +#include "avcodec.h"
> +#include "internal.h"
> +#include "libavutil/internal.h"
> +#include "libavutil/mem.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/imgutils.h"
> +
> +#define DELAY_FRAMES 8
> +
> +const xavs2_api_t *api = NULL;
> +
> +typedef struct XAVS2EContext {
> +    AVClass        *class;
> +
> +    void*         handle;
> +
> +    int i_lcurow_threads;
> +    int i_frame_threads;
> +    int i_initial_qp;                     

Trailing space on this line (also more below).

> +    int preset_level;
> +    int intra_period;
> +    int sourcewigth;
> +    int sourceheight;
> +
> +    void *encoder;
> +    char *xavs2_opts;
> +
> +    int b_hierarchical_reference;
> +    int num_b_frames;
> +
> +    xavs2_outpacket_t packet;
> +    xavs2_param_t *param;
> +
> +} XAVS2EContext;
> +
> +static const float AVS2_FRAME_RATE[8] = {
> +    24000.0f / 1001.0f, 24.0f, 25.0f, 30000.0f / 1001.0f, 30.0f, 50.0f, 60000.0f / 1001.0f, 60.0f

Numbers like 30000/1001 are exact as floats.  Can you use rationals instead?

> +};
> +
> +static int xavs2e_find_framerate_code(AVCodecContext *avctx)
> +{
> +    int fps_num = avctx->time_base.den;
> +    int fps_den = avctx->time_base.num;
> +    float fps = (float)(fps_num) / (float)(fps_den);
> +    int i;
> +
> +    av_log(avctx, AV_LOG_WARNING, "frame rate: %d/%d, %.3f\n", fps_num, fps_den, fps);

Shouldn't be a warning; maybe DEBUG?

> +    for (i = 0; i < 7; i++) {
> +         if (fps <= AVS2_FRAME_RATE[i]) {
> +             break;
> +         }
> +    }
> +    av_log(avctx, AV_LOG_WARNING, "frame rate code: %d\n", i + 1);
> +    return i + 1;
> +}

Given that these framerate values appear to be the the same as for MPEG-1, can you use "ff_mpeg12_find_best_frame_rate(avctx->framerate, &code, NULL, NULL, 0)" to replace this function?

> +
> +static int xavs2e_init(AVCodecContext *avctx)
> +{
> +    XAVS2EContext *cae= avctx->priv_data;
> +
> +    char str_bd[16], str_iqp[16], str_w[16], str_h[16], str_preset[16], str_hr[16], str_bf[16];
> +    char str_iv[16], str_TBR[16], str_fr[16];
> +
> +    if (avctx->pix_fmt == AV_PIX_FMT_YUV420P) {
> +        /* get API handler */
> +        api = xavs2_api_get(8);
> +       
> +        cae->param = api->opt_alloc();

Allocation can fail?

> +
> +        sprintf(str_bd, "%d", 8);
> +        sprintf(str_iqp, "%d", 32);
> +
> +        api->opt_set2(cae->param, "bitdepth", str_bd);
> +        api->opt_set2(cae->param, "initial_qp", str_iqp);

You have an argument for the initial qp, but then hardcode a different value here.

> +    } else if (avctx->pix_fmt == AV_PIX_FMT_YUV420P10) {
> +        /* get API handler */
> +        api = xavs2_api_get(10);
> +
> +        cae->param = api->opt_alloc();
> +
> +        sprintf(str_bd, "%d", 10);
> +        sprintf(str_iqp, "%d", 45);
> +
> +        api->opt_set2(cae->param, "bitdepth", str_bd);
> +        api->opt_set2(cae->param, "initial_qp", str_iqp);
> +    }
> +
> +
> +    sprintf(str_w, "%d", avctx->width);
> +    sprintf(str_h, "%d", avctx->height);
> +
> +    api->opt_set2(cae->param,"width",str_w);
> +    api->opt_set2(cae->param,"height",str_h);
> +    api->opt_set2(cae->param,"rec","0");
> +    api->opt_set2(cae->param,"log","0");

Maybe you could make a function which does the snprintf() and calls opt_set2(), so you can write something like "xavs2e_opt_set2(cae, "foo", value)" for all of these rather than having all the separate string arrays.

> +
> +    /* preset level */
> +    sprintf(str_preset, "%d", cae->preset_level);
> +
> +    api->opt_set2(cae->param,"preset", str_preset);
> +    /* bframes */
> +    av_log(avctx, AV_LOG_WARNING,
> +          "HierarchicalReference %d, Number B Frames %d.\n", cae->b_hierarchical_reference, cae->num_b_frames);

Not a warning.

> +    
> +    sprintf(str_hr, "%d", cae->b_hierarchical_reference);
> +    sprintf(str_bf, "%d", cae->num_b_frames);

Use AVCodecContext.max_b_frames rather than a private option?

> +
> +    api->opt_set2(cae->param, "hierarchical_ref",  str_hr);
> +    api->opt_set2(cae->param, "bframes",  str_bf);
> +
> +    if (cae->xavs2_opts) {
> +        AVDictionary *dict    = NULL;
> +        AVDictionaryEntry *en = NULL;
> +
> +        if (!av_dict_parse_string(&dict, cae->xavs2_opts, "=", ":", 0)) {
> +            while ((en = av_dict_get(dict, "", en, AV_DICT_IGNORE_SUFFIX))) {
> +                int i_value = isdigit(en->value[0]) ? atoi(en->value) : 0;
> +
> +                sprintf(str_iv, "%d", i_value);
> +
> +                int parse_ret = api->opt_set2(cae->param, en->key, str_iv);
> +
> +                if (parse_ret < 0) {
> +                    av_log(avctx, AV_LOG_WARNING,
> +                          "[xavs2] Invalid value for %s: %s.\n", en->key, en->value);

The logging context will already already add a tag - you don't need the "[xavs2]" here.

> +                }
> +            }
> +            av_dict_free(&dict);
> +        }
> +    }
> +
> +    /* Rate control */
> +    if (avctx->bit_rate > 0) {
> +        api->opt_set2(cae->param, "RateControl",  "1");  // VBR
> +
> +        sprintf(str_TBR, "%d", avctx->bit_rate);
> +
> +        api->opt_set2(cae->param, "TargetBitRate", str_TBR);
> +    }

What sort of output do you get if bit_rate isn't set?  (Some kind of constant quality?)

> +    api->opt_set2(cae->param, "intraperiod", "50");

Is this the GOP size?  Can you use AVCodecContext.gop_size instead so it's configurable?

> +
> +    sprintf(str_fr, "%d", xavs2e_find_framerate_code(avctx));
> +
> +    api->opt_set2(cae->param, "FrameRate", str_fr);
> +
> +    cae->encoder = api->encoder_create(cae->param);
> +
> +    if (cae->encoder == NULL) {
> +        fprintf(stderr, "Error: Can not create encoder. Null pointer returned.\n");

Log with av_log().  Is there any way to get more specific information about what went wrong?

> +
> +        return -1;

Return an AVERROR() code.

> +    }
> +    return 0;
> +}
> +
> +/* ---------------------------------------------------------------------------
> + */
> +static void dump_encoded_data(void *coder, xavs2_outpacket_t *packet)
> +{    
> +     api->encoder_packet_unref(coder, packet);
> +}
> +
> +/** 
> +* Encode data to an AVPacket. 
> +* 
> +* @param      avctx          codec context 
> +* @param      avpkt          output AVPacket (may contain a user-provided buffer) 
> +* @param[in]  frame          AVFrame containing the raw data to be encoded 
> +* @param[out] got_packet_ptr encoder sets to 0 or 1 to indicate that a 
> +*                            non-empty packet was returned in avpkt. 
> +* @return 0 on success, negative error code on failure 
> +*/ 

This comment is copied from avcodec.h, don't include it here.

> +static int xavs2e_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> +                      const AVFrame *frame, int *got_packet)
> +{
> +    XAVS2EContext *cae = avctx->priv_data;
> +
> +    xavs2_picture_t pic;
> +
> +    int j, k;
> +
> +    /* create the XAVS2 video encoder */
> +    /* read frame data and send to the XAVS2 video encoder */
> +    if (api->encoder_get_buffer(cae->encoder, &pic) < 0) {
> +        fprintf(stderr, "failed to get frame buffer [%3d].\n", pic.i_pts);
> +        return -1; 
> +    }
> +    if (frame) {  
> +
> +        switch (frame->format) {
> +            case AV_PIX_FMT_YUV420P:
> +                if (pic.img.in_sample_size != pic.img.enc_sample_size) {
> +                    const int shift_in = atoi(api->opt_get(cae->param, "SampleShift"));
> +                    for (k = 0; k < 3; k++) {
> +                        int i_stride = pic.img.i_stride[k];
> +                        for (j = 0; j < pic.img.i_lines[k]; j++) {
> +                            uint16_t *p_plane = (uint16_t *)&pic.img.img_planes[k][j * i_stride];
> +                            int i;
> +                            uint8_t *p_buffer = frame->data[k] + frame->linesize[k] * j;
> +                            memset(p_plane, 0, i_stride);
> +                            for (i = 0; i < pic.img.i_width[k]; i++) {
> +                                p_plane[i] = p_buffer[i] << shift_in;

Does a simple shift actually do the right thing here?  For full-range data I would expect 8->10 bit conversion to be more like "out = (in << 2) | (in >> 6)".

> +                            }
> +                        }
> +                    }
> +                } else {
> +                    for (k = 0; k < 3; k++) {
> +                        for (j = 0; j < pic.img.i_lines[k]; j++) {
> +                            memcpy(pic.img.img_planes[k] + pic.img.i_stride[k] * j, frame->data[k]+frame->linesize[k] * j, pic.img.i_width[k] * pic.img.in_sample_size);
> +                        }
> +                    }

Does the library look at the frame data after the encode call?  If not, you shouldn't need this copy at all.  If so, you might be able to hold references to the input buffers to avoid it.

> +                }
> +            break;
> +            case AV_PIX_FMT_YUV420P10:
> +                if (pic.img.in_sample_size == 2) {
> +                    for (k = 0; k < 3; k++) {
> +                        for (j = 0; j < pic.img.i_lines[k]; j++) {
> +                            memcpy(pic.img.img_planes[k] + pic.img.i_stride[k] * j, frame->data[k]+frame->linesize[k] * j, pic.img.i_width[k] * pic.img.in_sample_size);
> +                        }
> +                    }
> +                } else {
> +                    av_log(avctx, AV_LOG_ERROR,
> +                          "[xavs2] Unsupportted input pixel format\n");

Probably explain what actually went wrong here.  I'm guessing that means libxavs2 was built without 10-bit support?

> +                }
> +            break;
> +            default:
> +                av_log(avctx, AV_LOG_ERROR,
> +                      "[xavs2] Unsupportted pixel format\n");
> +            break;
> +        }
> +
> +        pic.i_state = 0;
> +        pic.i_pts  = frame->pts;
> +        pic.i_type = XAVS2_TYPE_AUTO;
> +
> +        api->encoder_encode(cae->encoder, &pic, &cae->packet);

Can this return an error?

> +        dump_encoded_data(cae->encoder, &cae->packet);
> +    } else {
> +        api->encoder_encode(cae->encoder, NULL, &cae->packet); 
> +        dump_encoded_data(cae->encoder, &cae->packet);

What does the unref included in the dump function do?

> +    }
> +
> +    if((cae->packet.len != 0) && (cae->packet.state != XAVS2_STATE_FLUSH_END)){
> +        av_new_packet(pkt, cae->packet.len); 

This is an allocation and needs to be checked.

> +
> +        pkt->pts = cae->packet.pts;
> +        pkt->dts = cae->packet.dts;
> +
> +        memcpy(pkt->data, cae->packet.stream, cae->packet.len);

Can this copy be avoided by holding a reference to the output data?

> +        pkt->data=cae->packet.len;
> +
> +        pkt->size = cae->packet.len;
> +        *got_packet = 1;
> +    } else {
> +        *got_packet = 0;
> +    }
> +
> +    return 0;
> +}
> +
> +static int xavs2e_close(AVCodecContext *avctx)
> +{
> +    XAVS2EContext *cae = avctx->priv_data;
> +    /* destroy the encoder */
> +    api->encoder_destroy(cae->encoder);

Does this API accept a null pointer?

> +
> +    if (cae->param != NULL) {

"if (cae->param) {"

> +        api->opt_destroy(cae->param);
> +    }
> +    
> +    return 0;
> +}
> +
> +#define OFFSET(x) offsetof(XAVS2EContext, x)
> +#define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
> +
> +static const AVOption options[] = {
> +   { "i_lcurow_threads",           "number of parallel threads for rows"     ,       OFFSET(i_lcurow_threads),  AV_OPT_TYPE_INT,    {.i64 =  5 }, 1,   8,  VE },
> +    { "i_frame_threads" ,           "number of parallel threads for frames"   ,       OFFSET(i_frame_threads) ,  AV_OPT_TYPE_INT,    {.i64 =  1 }, 1,   4,  VE },

These options aren't used?  Can you do something with AVCodecContext.thread_count rather than adding these private options?

> +    { "i_initial_qp"    ,           "Quantization parameter",       OFFSET(i_initial_qp)    ,  AV_OPT_TYPE_INT,    {.i64 = 34 }, 1,  63,  VE },
> +    { "preset_level"    ,           "Speed level"           ,       OFFSET(preset_level)    ,  AV_OPT_TYPE_INT,    {.i64 =  0 }, 0,   9,  VE },
> +    { "intra_period"    ,           "Intra period"          ,       OFFSET(intra_period)    ,  AV_OPT_TYPE_INT,    {.i64 =  4 }, 3, 100,  VE },
> +    { "hierarchical_ref",           "hierarchical reference",       OFFSET(b_hierarchical_reference)    ,  AV_OPT_TYPE_INT,    {.i64 =  1 }, 0, 1,  VE },
> +    { "num_bframes"     ,           "number of B frames"    ,       OFFSET(num_b_frames)    ,  AV_OPT_TYPE_INT,    {.i64 =  7 }, 0,  15,  VE },
> +    { "xavs2-params",    "set the xavs2 configuration using a :-separated list of key=value parameters", OFFSET(xavs2_opts), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE },
> +    { NULL },
> +};
> +
> +static const AVClass xavs2e_class = {
> +    .class_name = "XAVS2EContext",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};
> +
> +static const AVCodecDefault xavs2e_defaults[] = {
> +    { "b",                "0" },
> +    { NULL },
> +};
> +
> +AVCodec ff_libxavs2_encoder = {
> +    .name           = "libxavs2",
> +    .long_name      = NULL_IF_CONFIG_SMALL("xavs2 Chinese AVS2 (Audio Video Standard)"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_AVS2,
> +    .priv_data_size = sizeof(XAVS2EContext),
> +    .init           = xavs2e_init,
> +    .encode2        = xavs2e_encode_frame,
> +    .close          = xavs2e_close,
> +    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,
> +    .pix_fmts       = (const enum AVPixelFormat[]) { AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV420P10, AV_PIX_FMT_NONE },
> +    .priv_class     = &xavs2e_class,
> +    .defaults       = xavs2e_defaults,
> +} ;
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index a5585dc..9748d3f 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -130,6 +130,7 @@ OBJS-$(CONFIG_CAF_DEMUXER)               += cafdec.o caf.o mov_chan.o mov_esds.o
>  OBJS-$(CONFIG_CAF_MUXER)                 += cafenc.o caf.o riff.o
>  OBJS-$(CONFIG_CAVSVIDEO_DEMUXER)         += cavsvideodec.o rawdec.o
>  OBJS-$(CONFIG_CAVSVIDEO_MUXER)           += rawenc.o
> +OBJS-$(CONFIG_CAVS2VIDEO_MUXER)          += rawenc.o
>  OBJS-$(CONFIG_CDG_DEMUXER)               += cdg.o
>  OBJS-$(CONFIG_CDXL_DEMUXER)              += cdxl.o
>  OBJS-$(CONFIG_CINE_DEMUXER)              += cinedec.o
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index a94364f..e8dcd9c 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -91,6 +91,7 @@ extern AVInputFormat  ff_caf_demuxer;
>  extern AVOutputFormat ff_caf_muxer;
>  extern AVInputFormat  ff_cavsvideo_demuxer;
>  extern AVOutputFormat ff_cavsvideo_muxer;
> +extern AVOutputFormat ff_cavs2video_muxer;
>  extern AVInputFormat  ff_cdg_demuxer;
>  extern AVInputFormat  ff_cdxl_demuxer;
>  extern AVInputFormat  ff_cine_demuxer;
> diff --git a/libavformat/rawenc.c b/libavformat/rawenc.c
> index 809ca23..616062e 100644
> --- a/libavformat/rawenc.c
> +++ b/libavformat/rawenc.c
> @@ -130,6 +130,19 @@ AVOutputFormat ff_cavsvideo_muxer = {
>  };
>  #endif
>  
> +#if CONFIG_CAVS2VIDEO_MUXER
> +AVOutputFormat ff_cavs2video_muxer = {
> +    .name              = "cavs2video",
> +    .long_name         = NULL_IF_CONFIG_SMALL("raw Chinese AVS2 (2nd Audio Video Standard)"),
> +    .extensions        = "avs2",
> +    .audio_codec       = AV_CODEC_ID_NONE,
> +    .video_codec       = AV_CODEC_ID_AVS2,
> +    .write_header      = force_one_stream,
> +    .write_packet      = ff_raw_write_packet,
> +    .flags             = AVFMT_NOTIMESTAMPS,
> +};
> +#endif
> +
>  #if CONFIG_CODEC2RAW_MUXER
>  AVOutputFormat ff_codec2raw_muxer = {
>      .name              = "codec2raw",
> 

The parts in libavformat should be a separate patch.

- Mark

On Sat, May 26, 2018 at 14:08:56 +0800, hwren wrote:
> Add Chinese AVS2 video encoder, FFmpeg can make use of the libxavs2 library for AVS2 encoding.
[...]
> +    .long_name         = NULL_IF_CONFIG_SMALL("raw Chinese AVS2 (2nd Audio Video Standard)"),

Is that really the name of the codec? Otherwise, "Chinese" seems
awkward to me. (Same for the decoder.)

> +Go to @url{https://github.com/pkuvcl/xavs2} and follow the instructions for
> +installing the library. Then pass @code{--enable-libxavs2} to configure to
> +enable it.

Proper documentation of the encoder's options would be appreciated!

One more important thing: libdavs2 and libxavs2 are licensed under
GPL2, not LGPL. You therefore need a handful of extra boilerplate in
configure (such as EXTERNAL_LIBRARY_GPL_LIST, and adding "gpl" as a
dependency), as far as I understand.

Cheers,
Moritz

At 2018-05-27 02:20:22, "Moritz Barsnick" <barsnick@gmx.net> wrote:
>On Sat, May 26, 2018 at 14:08:56 +0800, hwren wrote:
>> Add Chinese AVS2 video encoder, FFmpeg can make use of the libxavs2 library for AVS2 encoding.
>[...]
>> +    .long_name         = NULL_IF_CONFIG_SMALL("raw Chinese AVS2 (2nd Audio Video Standard)"),
>
>Is that really the name of the codec? Otherwise, "Chinese" seems
>awkward to me. (Same for the decoder.)

Changed.

>
>> +Go to @url{https://github.com/pkuvcl/xavs2} and follow the instructions for
>> +installing the library. Then pass @code{--enable-libxavs2} to configure to
>> +enable it.
>
>Proper documentation of the encoder's options would be appreciated!

I'm sorry to say that xavs2 could only use '-h' to get some information about its options. We'll make a documentation soon.

>
>One more important thing: libdavs2 and libxavs2 are licensed under
>GPL2, not LGPL. You therefore need a handful of extra boilerplate in
>configure (such as EXTERNAL_LIBRARY_GPL_LIST, and adding "gpl" as a
>dependency), as far as I understand.

Thanks for suggestions. We've put them into EXTERNAL_LIBRARY_GPL_LIST and added the gpl dependency. I'm not sure if  I did right, new patches are submitted.

>
>Cheers,
>Moritz
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On Mon, May 28, 2018 at 19:16:49 +0800, hwren wrote:
> >Is that really the name of the codec? Otherwise, "Chinese" seems
> >awkward to me. (Same for the decoder.)
> 
> Changed.

I still see it in your newly pushlished patches.

> >Proper documentation of the encoder's options would be appreciated!
> 
> I'm sorry to say that xavs2 could only use '-h' to get some
> information about its options. We'll make a documentation soon.

I don't understand. You explicitly implemented these options:

+static const AVOption options[] = {
+   { "i_lcurow_threads",           "number of parallel threads for rows"     ,       OFFSET(i_lcurow_threads),  AV_OPT_TYPE_INT,    {.i64 =  5 }, 1,   8,  VE },
+    { "i_frame_threads" ,           "number of parallel threads for frames"   ,       OFFSET(i_frame_threads) ,  AV_OPT_TYPE_INT,    {.i64 =  1 }, 1,   4,  VE },
+    { "i_initial_qp"    ,           "Quantization parameter",       OFFSET(i_initial_qp)    ,  AV_OPT_TYPE_INT,    {.i64 = 34 }, 1,  63,  VE },
+    { "preset_level"    ,           "Speed level"           ,       OFFSET(preset_level)    ,  AV_OPT_TYPE_INT,    {.i64 =  0 }, 0,   9,  VE },
+    { "intra_period"    ,           "Intra period"          ,       OFFSET(intra_period)    ,  AV_OPT_TYPE_INT,    {.i64 =  4 }, 3, 100,  VE },
+    { "hierarchical_ref",           "hierarchical reference",       OFFSET(b_hierarchical_reference)    ,  AV_OPT_TYPE_INT,    {.i64 =  1 }, 0, 1,  VE },
+    { "num_bframes"     ,           "number of B frames"    ,       OFFSET(num_b_frames)    ,  AV_OPT_TYPE_INT,    {.i64 =  7 }, 0,  15,  VE },
+    { "xavs2-params",    "set the xavs2 configuration using a :-separated list of key=value parameters", OFFSET(xavs2_opts), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE },
+    { NULL },
+};

Their description (incl. ranges with explanations, if possible) should
also go into doc/*.texi. At least mention them there initially, if you
don't know what they actually do.

> Thanks for suggestions. We've put them into EXTERNAL_LIBRARY_GPL_LIST
> and added the gpl dependency. I'm not sure if I did right, new
> patches are submitted.

Let's see what the other say.

Cheers,
Moritz