[FFmpeg-devel] [PATCH v3 1/2] avcodec: add external enc libvvenc for H266/VVC
Nuo Mi
nuomi2021 at gmail.com
Fri May 24 16:03:12 EEST 2024
Hi @Andreas,
Thank you for the review.
Seems Christian still needs some guidance from you, see 1, 2, 3
On Thu, May 16, 2024 at 11:22 PM Christian <chris10317h5 at gmail.com> wrote:
>
>
> > On 14. May 2024, at 18:49, Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
> >
> > Christian Bartnik:
> >> From: Thomas Siedel <thomas.ff at spin-digital.com>
> >>
> >> Add external encoder VVenC for H266/VVC encoding.
> >> Register new encoder libvvenc.
> >> Add libvvenc to wrap the vvenc interface.
> >> libvvenc implements encoder option: preset,qp,period,subjopt,
> >> vvenc-params,levelidc,tier.
> >> Enable encoder by adding --enable-libvvenc in configure step.
> >>
> >> Co-authored-by: Christian Bartnik chris10317h5 at gmail.com
> >> Signed-off-by: Christian Bartnik <chris10317h5 at gmail.com>
> >> ---
> >> configure | 4 +
> >> doc/encoders.texi | 65 +++++
> >> libavcodec/Makefile | 1 +
> >> libavcodec/allcodecs.c | 1 +
> >> libavcodec/libvvenc.c | 566 +++++++++++++++++++++++++++++++++++++++++
> >> 5 files changed, 637 insertions(+)
> >> create mode 100644 libavcodec/libvvenc.c
> >>
> >> diff --git a/configure b/configure
> >> index a909b0689c..5d9a14821b 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -293,6 +293,7 @@ External library support:
> >> --enable-libvorbis enable Vorbis en/decoding via libvorbis,
> >> native implementation exists [no]
> >> --enable-libvpx enable VP8 and VP9 de/encoding via libvpx
> [no]
> >> + --enable-libvvenc enable H.266/VVC encoding via vvenc [no]
> >> --enable-libwebp enable WebP encoding via libwebp [no]
> >> --enable-libx264 enable H.264 encoding via x264 [no]
> >> --enable-libx265 enable HEVC encoding via x265 [no]
> >> @@ -1966,6 +1967,7 @@ EXTERNAL_LIBRARY_LIST="
> >> libvmaf
> >> libvorbis
> >> libvpx
> >> + libvvenc
> >> libwebp
> >> libxevd
> >> libxeve
> >> @@ -3558,6 +3560,7 @@ libvpx_vp8_decoder_deps="libvpx"
> >> libvpx_vp8_encoder_deps="libvpx"
> >> libvpx_vp9_decoder_deps="libvpx"
> >> libvpx_vp9_encoder_deps="libvpx"
> >> +libvvenc_encoder_deps="libvvenc"
> >> libwebp_encoder_deps="libwebp"
> >> libwebp_anim_encoder_deps="libwebp"
> >> libx262_encoder_deps="libx262"
> >> @@ -7025,6 +7028,7 @@ enabled libvpx && {
> >> die "libvpx enabled but no supported decoders found"
> >> fi
> >> }
> >> +enabled libvvenc && require_pkg_config libvvenc "libvvenc >=
> 1.6.1" "vvenc/vvenc.h" vvenc_get_version
> >>
> >> enabled libwebp && {
> >> enabled libwebp_encoder && require_pkg_config libwebp "libwebp
> >= 0.2.0" webp/encode.h WebPGetEncoderVersion
> >> diff --git a/doc/encoders.texi b/doc/encoders.texi
> >> index c82f316f94..92aab17c49 100644
> >> --- a/doc/encoders.texi
> >> +++ b/doc/encoders.texi
> >> @@ -2378,6 +2378,71 @@ Indicates frame duration
> >> For more information about libvpx see:
> >> @url{http://www.webmproject.org/}
> >>
> >> + at section libvvenc
> >> +
> >> +VVenC H.266/VVC encoder wrapper.
> >> +
> >> +This encoder requires the presence of the libvvenc headers and library
> >> +during configuration. You need to explicitly configure the build with
> >> + at option{--enable-libvvenc}.
> >> +
> >> +The VVenC project website is at
> >> + at url{https://github.com/fraunhoferhhi/vvenc}.
> >> +
> >> + at subsection Supported Pixel Formats
> >> +
> >> +VVenC supports only 10-bit color spaces as input. But the internal
> (encoded)
> >> +bit depth can be set to 8-bit or 10-bit at runtime.
> >> +
> >> + at subsection Options
> >> +
> >> + at table @option
> >> + at item b
> >> +Sets target video bitrate.
> >> +
> >> + at item g
> >> +Set the GOP size. Currently support for g=1 (Intra only) or default.
> >> +
> >> + at item preset
> >> +Set the VVenC preset.
> >> +
> >> + at item levelidc
> >> +Set level idc.
> >> +
> >> + at item tier
> >> +Set vvc tier.
> >> +
> >> + at item qp
> >> +Set constant quantization parameter.
> >> +
> >> + at item subopt @var{boolean}
> >> +Set subjective (perceptually motivated) optimization. Default is 1
> (on).
> >> +
> >> + at item bitdepth8 @var{boolean}
> >> +Set 8bit coding mode instead of using 10bit. Default is 0 (off).
> >> +
> >> + at item period
> >> +set (intra) refresh period in seconds.
> >> +
> >> + at item vvenc-params
> >> +Set vvenc options using a list of @var{key}=@var{value} couples
> separated
> >> +by ":". See @command{vvencapp --fullhelp} or @command{vvencFFapp
> --fullhelp} for a list of options.
> >> +
> >> +For example, the options might be provided as:
> >> +
> >> + at example
> >> +intraperiod=64:decodingrefreshtype=idr:poc0idr=1:internalbitdepth=8
> >> + at end example
> >> +
> >> +For example the encoding options for 2-pass encoding might be provided
> with @option{-vvenc-params}:
> >> +
> >> + at example
> >> +ffmpeg -i input -c:v libvvenc -b 1M -vvenc-params
> passes=2:pass=1:rcstatsfile=stats.json output.mp4
> >> +ffmpeg -i input -c:v libvvenc -b 1M -vvenc-params
> passes=2:pass=2:rcstatsfile=stats.json output.mp4
> >> + at end example
> >> +
> >> + at end table
> >> +
> >> @section libwebp
> >>
> >> libwebp WebP Image encoder wrapper
> >> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> >> index 2443d2c6fd..5d7349090e 100644
> >> --- a/libavcodec/Makefile
> >> +++ b/libavcodec/Makefile
> >> @@ -1153,6 +1153,7 @@ OBJS-$(CONFIG_LIBVPX_VP8_DECODER) +=
> libvpxdec.o
> >> OBJS-$(CONFIG_LIBVPX_VP8_ENCODER) += libvpxenc.o
> >> OBJS-$(CONFIG_LIBVPX_VP9_DECODER) += libvpxdec.o
> >> OBJS-$(CONFIG_LIBVPX_VP9_ENCODER) += libvpxenc.o
> >> +OBJS-$(CONFIG_LIBVVENC_ENCODER) += libvvenc.o
> >> OBJS-$(CONFIG_LIBWEBP_ENCODER) += libwebpenc_common.o
> libwebpenc.o
> >> OBJS-$(CONFIG_LIBWEBP_ANIM_ENCODER) += libwebpenc_common.o
> libwebpenc_animencoder.o
> >> OBJS-$(CONFIG_LIBX262_ENCODER) += libx264.o
> >> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> >> index b102a8069e..59d36dbd56 100644
> >> --- a/libavcodec/allcodecs.c
> >> +++ b/libavcodec/allcodecs.c
> >> @@ -800,6 +800,7 @@ extern const FFCodec ff_libvpx_vp8_encoder;
> >> extern const FFCodec ff_libvpx_vp8_decoder;
> >> extern FFCodec ff_libvpx_vp9_encoder;
> >> extern const FFCodec ff_libvpx_vp9_decoder;
> >> +extern const FFCodec ff_libvvenc_encoder;
> >> /* preferred over libwebp */
> >> extern const FFCodec ff_libwebp_anim_encoder;
> >> extern const FFCodec ff_libwebp_encoder;
> >> diff --git a/libavcodec/libvvenc.c b/libavcodec/libvvenc.c
> >> new file mode 100644
> >> index 0000000000..78d4f55a2a
> >> --- /dev/null
> >> +++ b/libavcodec/libvvenc.c
> >> @@ -0,0 +1,566 @@
> >> +/*
> >> + * H.266 encoding using the VVenC library
> >> + *
> >> + * Copyright (C) 2022, Thomas Siedel
> >> + *
> >> + * 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 "config_components.h"
> >
> > Seems unneeded.
> >
> Thanks, I will remove it.
>
> >> +
> >> +#include <vvenc/vvenc.h>
> >> +#include <vvenc/vvencCfg.h>
> >> +#include <vvenc/version.h>
> >> +
> >> +#include "avcodec.h"
> >> +#include "codec_internal.h"
> >> +#include "encode.h"
> >> +#include "internal.h"
> >> +#include "packet_internal.h"
> >> +#include "profiles.h"
> >> +
> >> +#include "libavutil/avutil.h"
> >> +#include "libavutil/mem.h"
> >> +#include "libavutil/pixdesc.h"
> >> +#include "libavutil/opt.h"
> >> +#include "libavutil/common.h"
> >> +#include "libavutil/imgutils.h"
> >> +#include "libavutil/frame.h"
> >> +#include "libavutil/log.h"
> >> +
> >> +typedef struct VVenCOptions {
> >> + int preset; // preset 0: faster 4: slower
> >> + int qp; // quantization parameter 0-63
> >> + int subjectiveOptimization; // perceptually motivated QP
> adaptation, XPSNR based
> >> + int flag8bitCoding; // encode in 8bit instead of 10bit
> >> + int intraRefreshSec; // intra period/refresh in seconds
> >> + int levelIdc; // vvc level_idc
> >> + int tier; // vvc tier
> >> + AVDictionary *vvenc_opts;
> >> +} VVenCOptions;
> >> +
> >> +typedef struct VVenCContext {
> >> + AVClass *av_class;
> >> + VVenCOptions options; // encoder options
> >> + vvencEncoder *vvencEnc;
> >> + vvencAccessUnit *pAU;
> >> + bool encodeDone;
> >
> > We do not use CamelCase for variable and struct member names.
> >
> Thanks, I will change the members.
>
> >> +} VVenCContext;
> >> +
> >> +
> >> +static av_cold void ff_vvenc_log_callback(void *ctx, int level,
> >> + const char *fmt, va_list
> args)
> >
> > Remove the ff_ prefix from static functions (this is the prefix for
> > non-static functions).
> >
> Thanks. I was not aware of this.
>
> >> +{
> >> + vvenc_config params;
> >> + vvencEncoder *vvencEnc = (vvencEncoder *)ctx;
> >> + if (vvencEnc){
> >> + vvenc_config_default(¶ms);
> >> + vvenc_get_config(vvencEnc, ¶ms);
> >> + if ((int)params.m_verbosity >= level)
> >> + vfprintf(level == 1 ? stderr : stdout, fmt, args);
> >> + }
> >> +}
> >> +
> >> +static void ff_vvenc_set_verbository(vvenc_config* params )
> >
> > What's the reason for the space before the closing parentheses?
> > Moreover, we prefer to put the * to the parameter, not the type.
> >
> Thanks, it´s a type. I´ll fix it.
>
> >> +{
> >> + params->m_verbosity = VVENC_VERBOSE;
> >> + if (av_log_get_level() >= AV_LOG_DEBUG)
> >
> > av_log_get_level() can be changed at any time by someone else. So better
> > just call it once and cache the value for simplicity and consistency.
> >
> Good point. I´ll change it.
>
> >> + params->m_verbosity = VVENC_DETAILS;
> >> + else if (av_log_get_level() >= AV_LOG_VERBOSE)
> >> + params->m_verbosity = VVENC_NOTICE; // output per picture
> info
> >> + else if (av_log_get_level() >= AV_LOG_INFO)
> >> + params->m_verbosity = VVENC_WARNING; // ffmpeg default
> ffmpeg loglevel
> >> + else
> >> + params->m_verbosity = VVENC_SILENT;
> >> +}
> >> +
> >> +static int ff_vvenc_set_pic_format(AVCodecContext *avctx,
> vvenc_config* params )
> >> +{
> >> + VVenCContext *s =(VVenCContext *) avctx->priv_data;
> >
> > This cast is unnecessary in C.
> >
> Thanks. I´ll remove it.
>
> >> +
> >> + params->m_internChromaFormat = VVENC_CHROMA_420;
> >> + params->m_inputBitDepth[0] = 10;
> >> +
> >> + if (avctx->pix_fmt != AV_PIX_FMT_YUV420P10LE){
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "unsupported pixel format %s, currently only support
> for yuv420p10le\n",
> >> + av_get_pix_fmt_name(avctx->pix_fmt));
> >> + return AVERROR(EINVAL);
> >
> > 1. This whole block of code is dead: For encoders that set
> > AVCodec.pix_fmts, it is checked generically that the pixel format set is
> > one of the pixel formats in AVCodec.pix_fmts.
> > 2. Why only LE? Is this also true on BE systems?
> >
> It was just for double check. I change it to AV_PIX_FMT_YUV420P10
> and remove the check.
>
> >> + }
> >> +
> >> + if (s->options.flag8bitCoding) {
> >> +#if VVENC_VERSION_MAJOR > 1 || (VVENC_VERSION_MAJOR == 1 &&
> VVENC_VERSION_MINOR > 9) || (VVENC_VERSION_MAJOR == 1 &&
> VVENC_VERSION_MINOR >= 9 && VVENC_VERSION_PATCH >= 1)
> >> + params->m_internalBitDepth[0] = 8;
> >> +#else
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "unsupported 8bit coding mode. 8bit coding needs at
> least vvenc version >= 1.9.1 "
> >> + "(current version %s)\n", vvenc_get_version() );
> >> + return AVERROR(EINVAL);
> >> +#endif
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static void ff_vvenc_set_color_format(AVCodecContext *avctx,
> vvenc_config* params )
> >> +{
> >> + if (avctx->color_primaries != AVCOL_PRI_UNSPECIFIED)
> >> + params->m_colourPrimaries = (int) avctx->color_primaries;
> >> + if (avctx->colorspace != AVCOL_SPC_UNSPECIFIED)
> >> + params->m_matrixCoefficients = (int) avctx->colorspace;
> >> + if (avctx->color_trc != AVCOL_TRC_UNSPECIFIED) {
> >> + params->m_transferCharacteristics = (int) avctx->color_trc;
> >> +
> >> + if (avctx->color_trc == AVCOL_TRC_SMPTE2084)
> >> + params->m_HdrMode = (avctx->color_primaries ==
> AVCOL_PRI_BT2020) ?
> >> + VVENC_HDR_PQ_BT2020 : VVENC_HDR_PQ;
> >> + else if (avctx->color_trc == AVCOL_TRC_BT2020_10
> >> + || avctx->color_trc == AVCOL_TRC_ARIB_STD_B67)
> >> + params->m_HdrMode = (avctx->color_trc ==
> AVCOL_TRC_BT2020_10 ||
> >> + avctx->color_primaries ==
> AVCOL_PRI_BT2020 ||
> >> + avctx->colorspace ==
> AVCOL_SPC_BT2020_NCL ||
> >> + avctx->colorspace ==
> AVCOL_SPC_BT2020_CL) ?
> >> + VVENC_HDR_HLG_BT2020 : VVENC_HDR_HLG;
> >> + }
> >> +
> >> + if (params->m_HdrMode == VVENC_HDR_OFF
> >> + && (avctx->color_primaries != AVCOL_PRI_UNSPECIFIED
> >> + || avctx->colorspace != AVCOL_SPC_UNSPECIFIED)) {
> >> + params->m_vuiParametersPresent = 1;
> >> + params->m_colourDescriptionPresent = true;
> >> + }
> >> +}
> >> +
> >> +static void ff_vvenc_set_framerate(AVCodecContext *avctx,
> vvenc_config* params )
> >> +{
> >> + params->m_FrameRate = avctx->time_base.den;
> >> + params->m_FrameScale = avctx->time_base.num;
> >> +
> >> +FF_DISABLE_DEPRECATION_WARNINGS
> >> +
> >> +#if FF_API_TICKS_PER_FRAME
> >> + if (avctx->ticks_per_frame == 1) {
> >> +#endif
> >> + params->m_TicksPerSecond = -1; // auto mode for ticks per
> frame = 1
> >> +#if FF_API_TICKS_PER_FRAME
> >> + } else {
> >> + params->m_TicksPerSecond =
> >> + ceil((avctx->time_base.den / (double)
> avctx->time_base.num) *
> >> + (double) avctx->ticks_per_frame);
> >> + }
> >> +#endif
> >> +FF_ENABLE_DEPRECATION_WARNINGS
> >> +}
> >> +
> >> +static int ff_vvenc_parse_vvenc_params(AVCodecContext *avctx,
> vvenc_config* params, char* statsfile )
> >> +{
> >> + int parse_ret, ret;
> >> + VVenCContext *s;
> >> + AVDictionaryEntry *en = NULL;
> >
> > const
> >
> >> + s =(VVenCContext *) avctx->priv_data;
> >
> > Can be initialized directly and without the cast.
> >
> Thanks. I´ll remove it.
>
> >> + ret = 0;
> >> +
> >> + while ((en = av_dict_get(s->options.vvenc_opts, "", en,
> >> + AV_DICT_IGNORE_SUFFIX))) {
> >
> > av_dict_iterate()
>
> I was using libx264,libx265,libaomenc as reference and aligned to
> the existing code.
> I will change it to av_dict_iterate()
>
> >
> >> + av_log(avctx, AV_LOG_DEBUG, "vvenc_set_param: '%s:%s'\n",
> en->key,
> >> + en->value);
> >> + parse_ret = vvenc_set_param(params, en->key, en->value);
> >> + switch (parse_ret) {
> >> + case VVENC_PARAM_BAD_NAME:
> >> + av_log(avctx, AV_LOG_ERROR, "Unknown vvenc option: %s.\n",
> >> + en->key);
> >> + ret = AVERROR(EINVAL);
> >> + break;
> >> + case VVENC_PARAM_BAD_VALUE:
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "Invalid vvenc value for %s: %s.\n", en->key,
> en->value);
> >> + ret = AVERROR(EINVAL);
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> +
> >> + if (memcmp(en->key, "rcstatsfile", 11) == 0 ||
> >> + memcmp(en->key, "RCStatsFile", 11) == 0) {
> >
> > This presumes that en->key is at least 11 byte long (and it would accept
> > keys only prefixed by rcstatsfile); this need not be true at all. How
> > about using av_strcasecmp()?
>
> Good point. I´ll remove the memcmp.
>
> >
> >> + strncpy(statsfile, en->value, strlen(statsfile));
> >> + statsfile[strlen(statsfile)] = '\0';
> >
> > Did you test this at all?
> > 1. Using strlen(statsfile) means that the filename of the statsfile is
> > bounded by strlen("vvenc-rcstats.json"); more precisely, every iteration
> > of this code block can grow strlen() of the buffer by one and there is
> > no real bounds check, so this could lead to a stack buffer overflow
> > (when using a dictionary that contains rcstatsfile entries multiple
> times).
> > 2. We have av_strlcpy() for this.
> > 3. But a better approach is to just not copy the string at all, avoiding
> > any length restriction on the statsfile: Use const char **statsfile
> > instead of the char *statsfile parameter; in encode_init below you just
> > initialize statsfile via 'const char *statsfile = "vvenc-rcstats.json";'.
> >
> > The typical way to pass the statistics of a first pass to an encoder is
> > via an allocated buffer, not via a filename for the encoder to read
> > from/write to (see AVCodecContext.stats_out, stats_in). Is this possible
> > with libvvenc?
> >
> The 2pass approach was quite some of a hack by using the vvenc-
> params.
> Thanks for the review and hints to improve it.
> I will change the code and align to x264 behavior by using the
> 'passlogfile'/'stats' options and use 2pass flags AV_CODEC_FLAG_PASS1 |
> AV_CODEC_FLAG_PASS2 instead of using vvenc-params.
> I also will use the default ffmpeg 2pass logfile 'ffmpeg2pass-0.log' as
> default as libx264 is using it.
> Currently there is no way in vvenc to use an allocated buffer for
> write/read statistics.
>
> >> + }
> >> + }
> >> + return ret;
> >> +}
> >> +
> >> +static int ff_vvenc_set_rc_mode(AVCodecContext *avctx, vvenc_config*
> params)
> >> +{
> >> + if (params->m_RCPass != -1 && params->m_RCNumPasses == 1)
> >> + params->m_RCNumPasses = 2; /* enable 2pass mode */
> >
> > We actually have AV_CODEC_FLAG_PASS1 and AV_CODEC_FLAG_PASS2 that you
> > completely ignore.
> >
> I´ll change the 2pass behavior as mentioned above.
>
> >> +
> >> + if(avctx->rc_max_rate) {
> >> +#if VVENC_VERSION_MAJOR > 1 || (VVENC_VERSION_MAJOR == 1 &&
> VVENC_VERSION_MINOR > 8)
> >
> > Maybe you should map all of the major, minor and patch versions to one
> > VVENC_VERSION?
> >
> Thanks I will use a integer version for better readability.
>
> >> + params->m_RCMaxBitrate = avctx->rc_max_rate;
> >> +#endif
> >> +
> >> +#if VVENC_VERSION_MAJOR == 1 && VVENC_VERSION_MINOR < 11
> >> + /* rc_max_rate without a bit_rate enables capped CQF mode.
> >> + (QP + subj. optimization + max. bitrate) */
> >> + if(!avctx->bit_rate) {
> >> + av_log( avctx, AV_LOG_ERROR,
> >> + "Capped Constant Quality Factor mode (capped CQF)
> needs at "
> >> + "least vvenc version >= 1.11.0 (current version %s)\n",
> >> + vvenc_get_version());
> >> + return AVERROR(EINVAL);
> >> + }
> >> +#endif
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static int ff_vvenc_init_extradata(AVCodecContext *avctx, VVenCContext
> *s)
> >> +{
> >> + int ret;
> >> + if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> >> + ret = vvenc_get_headers(s->vvencEnc, s->pAU);
> >> + if (0 != ret) {
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "cannot get headers (SPS,PPS) from vvc
> encoder(vvenc): %s\n",
> >> + vvenc_get_last_error(s->vvencEnc));
> >> + vvenc_encoder_close(s->vvencEnc);
> >> + return AVERROR(EINVAL);
> >> + }
> >> +
> >> + if (s->pAU->payloadUsedSize <= 0) {
> >> + vvenc_encoder_close(s->vvencEnc);
> >> + return AVERROR_INVALIDDATA;
> >> + }
> >> +
> >> + avctx->extradata_size = s->pAU->payloadUsedSize;
> >> + avctx->extradata =
> >> + av_mallocz(avctx->extradata_size +
> AV_INPUT_BUFFER_PADDING_SIZE);
> >> + if (!avctx->extradata) {
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "Cannot allocate VVC header of size %d.\n",
> >> + avctx->extradata_size);
> >
> > Pointless error message, like so many of your allocation errors.
> >
> Sorry, but I was using libx265 as reference where all these error
> messages are implemented in the same way.
> Should I just remove the error messages and return an error?
>
1.
>
> >> + vvenc_encoder_close(s->vvencEnc);
> >> + return AVERROR(ENOMEM);
> >> + }
> >> +
> >> + memcpy(avctx->extradata, s->pAU->payload,
> avctx->extradata_size);
> >> + memset(avctx->extradata + avctx->extradata_size, 0,
> >> + AV_INPUT_BUFFER_PADDING_SIZE);
> >
> > Unnecessary given that you use av_mallocz().
> >
> Indeed. I´ll remove it.
>
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static av_cold int ff_vvenc_encode_init(AVCodecContext *avctx)
> >> +{
> >> + int ret;
> >> + int framerate, qp;
> >> + VVenCContext *s;
> >> + vvenc_config params;
> >> + vvencPresetMode preset;
> >> + char statsfile[1024] = "vvenc-rcstats.json";
> >> +
> >> + s = (VVenCContext *) avctx->priv_data;
> >> + qp = s->options.qp;
> >> + preset = (vvencPresetMode) s->options.preset;
> >> +
> >> + if (avctx->flags & AV_CODEC_FLAG_INTERLACED_DCT) {
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "ff_vvenc_encode_init::init() interlaced encoding not
> supported yet\n");
> >> + return AVERROR_INVALIDDATA;
> >
> > This is not invalid data; and there is no need to add the name of the
> > function to the error message.
> > (And what does "not supported yet" even mean? Does VVC even have
> > specific interlaced coding methods? Will this ever be supported?)
> >
> VVC has interlaces support as HEVC does. There are no special tools
> but only signalizing in high-level syntax.
> Currently there is no support in vvenc to signalize fields, so I added
> an error.
> What would be the best way to do it?
> Just throw a warning or using an error and return?
> I would change it to:
> av_log(avctx, AV_LOG_ERROR, "interlaced encoding not supported\n");
> return AVERROR(EINVAL);
>
2.
>
> >> + }
> >> +
> >> + vvenc_config_default(¶ms);
> >> +
> >> + framerate = avctx->time_base.den / avctx->time_base.num;
> >
> > You are aware that the division performed here is an integer division?
> > And are you aware that non-cfr content does not really have a framerate?
> > And that for cfr content, AVCodecContext.framerate contains the
> framerate?
> >
> The interface call 'vvenc_init_default' is expecting an int
> framerate and is setting the the ntsc timing depending on the
> framerate internally (e.g. 59 is mapped to 60000/1001).
> The correct time base is set later in the init call in function
> 'vvenc_set_framerate'
> ..
> 'params->m_FrameRate = avctx->time_base.den;'
> 'params->m_FrameScale = avctx->time_base.num;'
>
> But indeed I was aware that AVCodecContext.framerate and
> AVCodecContext.time_base are handled differently.
> I will change it, thanks.
>
> >> + vvenc_init_default(¶ms, avctx->width, avctx->height, framerate,
> >> + (qp >= 0) ? 0 : avctx->bit_rate, (qp < 0) ? 32
> : qp, preset);
> >> +
> >> + ff_vvenc_set_verbository(¶ms);
> >> +
> >> + if (avctx->thread_count > 0)
> >> + params.m_numThreads = avctx->thread_count;
> >> +
> >> + /* GOP settings (IDR/CRA) */
> >> + if (avctx->flags & AV_CODEC_FLAG_CLOSED_GOP)
> >> + params.m_DecodingRefreshType = VVENC_DRT_IDR;
> >> +
> >> + if (avctx->gop_size == 1) {
> >> + params.m_GOPSize = 1;
> >> + params.m_IntraPeriod = 1;
> >> + } else {
> >> + params.m_IntraPeriodSec = s->options.intraRefreshSec;
> >> + }
> >> +
> >> + params.m_AccessUnitDelimiter = true;
> >> + params.m_RCNumPasses = 1;
> >> +
> >> + params.m_usePerceptQPA = s->options.subjectiveOptimization;
> >> + params.m_level = (vvencLevel) s->options.levelIdc;
> >> + params.m_levelTier = (vvencTier) s->options.tier;
> >> +
> >> + ff_vvenc_set_framerate(avctx, ¶ms);
> >> +
> >> + ret = ff_vvenc_set_pic_format(avctx, ¶ms);
> >> + if( ret != 0 )
> >> + return ret;
> >> +
> >> + ff_vvenc_set_color_format(avctx, ¶ms);
> >> +
> >> + ret = ff_vvenc_parse_vvenc_params(avctx, ¶ms, &statsfile[0]);
> >> + if( ret != 0 )
> >> + return ret;
> >> +
> >> +
> >> + ret = ff_vvenc_set_rc_mode(avctx, ¶ms);
> >> + if( ret != 0 )
> >> + return ret;
> >> +
> >> + s->vvencEnc = vvenc_encoder_create();
> >> + if (NULL == s->vvencEnc) {
> >
> > if (!s->vvencEnc) is the common check here.
> >
> Thanks.
>
> >> + av_log(avctx, AV_LOG_ERROR, "cannot create vvc encoder
> (vvenc)\n");
> >> + return AVERROR(ENOMEM);
> >> + }
> >> +
> >> + vvenc_set_msg_callback(¶ms, s->vvencEnc,
> ff_vvenc_log_callback);
> >> + ret = vvenc_encoder_open(s->vvencEnc, ¶ms);
> >> + if (0 != ret) {
> >> + av_log(avctx, AV_LOG_ERROR, "cannot open vvc encoder (vvenc):
> %s\n",
> >> + vvenc_get_last_error(s->vvencEnc));
> >> + vvenc_encoder_close(s->vvencEnc);
> >> + return AVERROR(EINVAL);
> >> + }
> >> +
> >> + vvenc_get_config(s->vvencEnc, ¶ms); /* get the adapted
> config */
> >> +
> >> + av_log(avctx, av_log_get_level(), "vvenc version: %s\n",
> vvenc_get_version());
> >> + av_log(avctx, av_log_get_level(), "%s\n",
> >> + vvenc_get_config_as_string(¶ms, params.m_verbosity));
> >> +
> >> + if (params.m_RCNumPasses == 2) {
> >> + ret = vvenc_init_pass(s->vvencEnc, params.m_RCPass - 1,
> &statsfile[0]);
> >> + if (0 != ret) {
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "cannot init pass %d for vvc encoder (vvenc): %s\n",
> >> + params.m_RCPass, vvenc_get_last_error(s->vvencEnc));
> >> + vvenc_encoder_close(s->vvencEnc);
> >> + return AVERROR(EINVAL);
> >> + }
> >> + }
> >> +
> >> + s->pAU = vvenc_accessUnit_alloc();
> >> + if( !s->pAU ){
> >> + av_log(avctx, AV_LOG_FATAL, "cannot allocate memory for AU
> payload\n");
> >
> > 1. There is a memory leak here, as you don't close the encoder.
> > 2. This should be fixed by setting the FF_CODEC_CAP_INIT_CLEANUP
> > cap_internal; then you should (in fact, need to) remove all the
> > vvenc_encoder_close() calls.
> > (This also fixes leaks of pAU (it gets never freed on init errors at all
> > and leaks e.g. on ff_vvenc_init_extradata() errors.)
> >
> Thanks. I will return an error.
> I´ll add FF_CODEC_CAP_INIT_CLEANUP and remove all vvenc_encoder_close()
> calls.
>
> >> + return AVERROR(ENOMEM);
> >> + }
> >> + vvenc_accessUnit_alloc_payload(s->pAU, avctx->width *
> avctx->height);
> >> + if( !s->pAU ){
> >
> > This is a nonsense check: s->pAU is already set and can't be unset by
> > the call, so one has to check this somehow else.
> >
> Thanks for seeing this. It have to be
> if (!s->pAU->payload){
> I´ll change it.
>
> >> + av_log(avctx, AV_LOG_FATAL, "cannot allocate payload memory of
> size %d\n",
> >
> > AV_LOG_ERROR, not AV_LOG_FATAL
> >
> >> + avctx->width * avctx->height );
> >> + return AVERROR(ENOMEM);
> >> + }
> >> +
> >> + ret = ff_vvenc_init_extradata(avctx, s);
> >> + if( ret != 0 )
> >> + return ret;
> >> +
> >> + s->encodeDone = false;
> >> + return 0;
> >> +}
> >> +
> >> +static av_cold int ff_vvenc_encode_close(AVCodecContext * avctx)
> >> +{
> >> + VVenCContext *s = (VVenCContext *) avctx->priv_data;
> >> + if (s->vvencEnc) {
> >> + if (av_log_get_level() >= AV_LOG_VERBOSE)
> >> + vvenc_print_summary(s->vvencEnc);
> >> +
> >> + if (0 != vvenc_encoder_close(s->vvencEnc)) {
> >> + av_log(avctx, AV_LOG_ERROR, "cannot close vvenc\n");
> >> + return -1;
> >
> > You should free the access unit even if vvenc_encoder_close() returned
> > an error.
>
> Thanks. I´ll move the vvenc_accessUnit_free to the top before
> calling vvenc_encoder_close.
>
> >
> >> + }
> >> + }
> >> +
> >> + vvenc_accessUnit_free(s->pAU, true);
> >
> > Can this handle the case in which s->pAU is NULL?
> >
> Yes, the function vvenc_accessUnit_free will check this internally
> as well. However I add a null-check to make it clear.
>
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static av_cold int ff_vvenc_encode_frame(AVCodecContext *avctx,
> AVPacket *pkt,
> >> + const AVFrame *frame, int
> *got_packet)
> >> +{
> >> + VVenCContext *s = (VVenCContext *) avctx->priv_data;
> >> + vvencYUVBuffer *pyuvbuf;
> >> + vvencYUVBuffer yuvbuf;
> >> + int pict_type;
> >> + int ret;
> >> +
> >> + pyuvbuf = NULL;
> >> + if (frame) {
> >> + if (avctx->pix_fmt == AV_PIX_FMT_YUV420P10LE) {
> >> + vvenc_YUVBuffer_default(&yuvbuf);
> >> + yuvbuf.planes[0].ptr = (int16_t *) frame->data[0];
> >> + yuvbuf.planes[1].ptr = (int16_t *) frame->data[1];
> >> + yuvbuf.planes[2].ptr = (int16_t *) frame->data[2];
> >> +
> >> + yuvbuf.planes[0].width = frame->width;
> >> + yuvbuf.planes[0].height = frame->height;
> >> + /* stride is used in 16bitsamples (16bit) in vvenc, ffmpeg
> uses stride in bytes */
> >> + yuvbuf.planes[0].stride = frame->linesize[0] >> 1;
> >> +
> >> + yuvbuf.planes[1].width = frame->width >> 1;
> >> + yuvbuf.planes[1].height = frame->height >> 1;
> >> + yuvbuf.planes[1].stride = frame->linesize[1] >> 1;
> >> +
> >> + yuvbuf.planes[2].width = frame->width >> 1;
> >> + yuvbuf.planes[2].height = frame->height >> 1;
> >> + yuvbuf.planes[2].stride = frame->linesize[2] >> 1;
> >> +
> >> + yuvbuf.cts = frame->pts;
> >> + yuvbuf.ctsValid = true;
> >> + pyuvbuf = &yuvbuf;
> >> + } else {
> >> + av_log(avctx, AV_LOG_ERROR,
> >> + "unsupported input colorspace! input must be
> yuv420p10le");
> >> + return AVERROR(EINVAL);
> >
> > Checks like these do not belong in a single encoder; they are simply
> > allowed to presume that avctx->pix_fmt does not change during an encode
> > session and that all frames reaching the decoder actually have this
> > pixel format.
> >
> Thanks, I will remove the check.
>
> >> + }
> >> + }
> >> +
> >> + if (!s->encodeDone) {
> >> + ret = vvenc_encode(s->vvencEnc, pyuvbuf, s->pAU,
> &s->encodeDone);
> >> + if (ret != 0) {
> >> + av_log(avctx, AV_LOG_ERROR, "error in vvenc::encode -
> ret:%d\n",
> >> + ret);
> >> + return AVERROR(EINVAL);
> >
> > This is not EINVAL. It is AVERROR_EXTERNAL.
> >
> Thanks. I´ll change it.
>
> >> + }
> >> + } else {
> >> + *got_packet = 0;
> >
> > Unecessary: *got_packet is already zero before we enter this function.
> >
> good point. I´ll remove it.
>
> >> + return 0;
> >> + }
> >> +
> >> + if (s->pAU->payloadUsedSize > 0) {
> >> + ret = ff_get_encode_buffer(avctx, pkt,
> s->pAU->payloadUsedSize, 0);
> >> + if (ret < 0) {
> >> + av_log(avctx, AV_LOG_ERROR, "Error getting output
> packet.\n");
> >
> > Unnecessary, as ff_get_encode_buffer() already emits its own error
> messages.
> >
> Good to know. I´ll remove it.
>
> >> + return ret;
> >> + }
> >> +
> >> + memcpy(pkt->data, s->pAU->payload, s->pAU->payloadUsedSize);
> >> +
> >> + if (s->pAU->ctsValid)
> >> + pkt->pts = s->pAU->cts;
> >> + if (s->pAU->dtsValid)
> >> + pkt->dts = s->pAU->dts;
> >> + pkt->flags |= AV_PKT_FLAG_KEY * s->pAU->rap;
> >> +
> >> + switch (s->pAU->sliceType) {
> >> + case VVENC_I_SLICE:
> >> + pict_type = AV_PICTURE_TYPE_I;
> >> + break;
> >> + case VVENC_P_SLICE:
> >> + pict_type = AV_PICTURE_TYPE_P;
> >> + break;
> >> + case VVENC_B_SLICE:
> >> + pict_type = AV_PICTURE_TYPE_B;
> >> + break;
> >> + default:
> >> + av_log(avctx, AV_LOG_ERROR, "Unknown picture type
> encountered.\n");
> >> + return AVERROR_EXTERNAL;
> >> + }
> >> +
> >> + ff_side_data_set_encoder_stats(pkt, 0, NULL, 0, pict_type);
> >
> > Pointless given that you do not really have meaningful statistics to set.
> >
> vvenc currently only returns the picture type as statistics.
> I added this code to call all basic ffmpeg routines during an encoding
> step.
> However a picture type is also a statistic value.
> Shall I remove this code block or leave it to add more statistics in
> upcoming versions?
>
3.
>
> >> +
> >> + *got_packet = 1;
> >> +
> >> + return 0;
> >> + } else {
> >> + *got_packet = 0;
> >> + return 0;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const enum AVPixelFormat pix_fmts_vvenc[] = {
> >> + AV_PIX_FMT_YUV420P10LE,
> >> + AV_PIX_FMT_NONE
> >> +};
> >> +
> >> +#define OFFSET(x) offsetof(VVenCContext, x)
> >> +#define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
> >> +static const AVOption libvvenc_options[] = {
> >> + {"preset", "set encoding preset(0: faster - 4: slower", OFFSET(
> options.preset), AV_OPT_TYPE_INT, {.i64 = 2} , 0 , 4 , VE, "preset"},
> >> + { "faster", "0", 0, AV_OPT_TYPE_CONST, {.i64 = VVENC_FASTER},
> INT_MIN, INT_MAX, VE, "preset" },
> >> + { "fast", "1", 0, AV_OPT_TYPE_CONST, {.i64 = VVENC_FAST},
> INT_MIN, INT_MAX, VE, "preset" },
> >> + { "medium", "2", 0, AV_OPT_TYPE_CONST, {.i64 = VVENC_MEDIUM},
> INT_MIN, INT_MAX, VE, "preset" },
> >> + { "slow", "3", 0, AV_OPT_TYPE_CONST, {.i64 = VVENC_SLOW},
> INT_MIN, INT_MAX, VE, "preset" },
> >> + { "slower", "4", 0, AV_OPT_TYPE_CONST, {.i64 = VVENC_SLOWER},
> INT_MIN, INT_MAX, VE, "preset" },
> >> + { "qp" , "set quantization", OFFSET(options.qp),
> AV_OPT_TYPE_INT, {.i64 = -1}, -1 , 63 ,VE, "qp_mode" },
> >
> > The "qp_mode" is unnecessary: AVOption.unit exists to link
> > AV_OPT_TYPE_CONST options to the actual options.
> >
> Thanks, I will remove it.
>
> >> + { "period" , "set (intra) refresh period in seconds",
> OFFSET(options.intraRefreshSec), AV_OPT_TYPE_INT, {.i64 = 1}, 1 , INT_MAX
> ,VE,"irefreshsec" },
> >
> > We actually have AVCodecContext.gop_size, which is in frames
>
> I know, but 'period' is in seconds for convenience and not in
> frames.
> In vvenc GOP is a hierarchically group of pictures (size 16 or 32).
> The decoding refresh (which you are calling gop) is independent and can
> be set by using vvenc-params intraperiod=N, where N=frames
>
> >
> >> + { "subjopt", "set subjective (perceptually motivated)
> optimization", OFFSET(options.subjectiveOptimization), AV_OPT_TYPE_BOOL,
> {.i64 = 1}, 0 , 1, VE},
> >> + { "bitdepth8", "set 8bit coding mode",
> OFFSET(options.flag8bitCoding), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0 , 1, VE},
> >
> > Why are these separate options and not simply part of vvenc-params?
> >
> There are vvenc-params options. It´s just for convenience. It´s
> also possible to use
> -vvenc-params qpa=1:internalbitdepth=8
>
> Shall I remove these options?
>
Yes, please remove it. It's an implementation detail that may be removed in
the future. There's no need to expose it explicitly
>
> >> + { "vvenc-params", "set the vvenc configuration using a :-separated
> list of key=value parameters", OFFSET(options.vvenc_opts),
> AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE },
> >> + { "levelidc", "vvc level_idc", OFFSET( options.levelIdc),
> AV_OPT_TYPE_INT, {.i64 = 0}, 0, 105, VE, "levelidc"},
> >> + { "0", "auto", 0, AV_OPT_TYPE_CONST, {.i64 = 0}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "1", "1" , 0, AV_OPT_TYPE_CONST, {.i64 = 16}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "2", "2" , 0, AV_OPT_TYPE_CONST, {.i64 = 32}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "2.1", "2.1" , 0, AV_OPT_TYPE_CONST, {.i64 = 35}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "3", "3" , 0, AV_OPT_TYPE_CONST, {.i64 = 48}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "3.1", "3.1" , 0, AV_OPT_TYPE_CONST, {.i64 = 51}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "4", "4" , 0, AV_OPT_TYPE_CONST, {.i64 = 64}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "4.1", "4.1" , 0, AV_OPT_TYPE_CONST, {.i64 = 67}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "5", "5" , 0, AV_OPT_TYPE_CONST, {.i64 = 80}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "5.1", "5.1" , 0, AV_OPT_TYPE_CONST, {.i64 = 83}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "5.2", "5.2" , 0, AV_OPT_TYPE_CONST, {.i64 = 86}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "6", "6" , 0, AV_OPT_TYPE_CONST, {.i64 = 96}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "6.1", "6.1" , 0, AV_OPT_TYPE_CONST, {.i64 = 99}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "6.2", "6.2" , 0, AV_OPT_TYPE_CONST, {.i64 = 102}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >> + { "6.3", "6.3" , 0, AV_OPT_TYPE_CONST, {.i64 = 105}, INT_MIN,
> INT_MAX, VE, "levelidc"},
> >
> > We have a generic level option, so there is no need for this list above.
> >
> Thanks, I will have a look into that.
>
> >> + { "tier", "set vvc tier", OFFSET( options.tier), AV_OPT_TYPE_INT,
> {.i64 = 0}, 0 , 1 , VE, "tier"},
> >> + { "main", "main", 0, AV_OPT_TYPE_CONST, {.i64 = 0}, INT_MIN,
> INT_MAX, VE, "tier"},
> >> + { "high", "high", 0, AV_OPT_TYPE_CONST, {.i64 = 1}, INT_MIN,
> INT_MAX, VE, "tier"},
> >> + {NULL}
> >> +};
> >> +
> >> +static const AVClass class_libvvenc = {
> >> + .class_name = "libvvenc-vvc encoder",
> >> + .item_name = av_default_item_name,
> >> + .option = libvvenc_options,
> >> + .version = LIBAVUTIL_VERSION_INT,
> >> +};
> >> +
> >> +static const FFCodecDefault vvenc_defaults[] = {
> >> + { "b", "0" },
> >> + { "g", "-1" },
> >> + { NULL },
> >> +};
> >> +
> >> +FFCodec ff_libvvenc_encoder = {
> >
> > Missing const
>
> It´s not consistent as some codecs are const other not.
> e.g. ff_libaom_av1_encoder, ff_libx265_encoder
>
> I´ll fix it.
>
> >
> >> + .p.name = "libvvenc",
> >> + CODEC_LONG_NAME("H.266 / VVC Encoder VVenC"),
> >> + .p.type = AVMEDIA_TYPE_VIDEO,
> >> + .p.id <http://p.id/> = AV_CODEC_ID_VVC,
> >> + .p.capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_OTHER_THREADS,
> >
> > You can add AV_CODEC_CAP_DR1
> >
> Thanks. I´ll add it.
>
> >> + .p.profiles = NULL_IF_CONFIG_SMALL(ff_vvc_profiles),
> >> + .p.priv_class = &class_libvvenc,
> >> + .p.wrapper_name = "libvvenc",
> >> + .priv_data_size = sizeof(VVenCContext),
> >> + .p.pix_fmts = pix_fmts_vvenc,
> >> + .init = ff_vvenc_encode_init,
> >> + FF_CODEC_ENCODE_CB(ff_vvenc_encode_frame),
> >> + .close = ff_vvenc_encode_close,
> >> + .defaults = vvenc_defaults,
> >> + .caps_internal = FF_CODEC_CAP_AUTO_THREADS,
> >> +};
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at ffmpeg.org>
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org <mailto:ffmpeg-devel-request at ffmpeg.org>
> with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list