> On 14. May 2024, at 18:49, Andreas Rheinhardt > <andreas.rheinha...@outlook.com> wrote: > > Christian Bartnik: >> From: Thomas Siedel <thomas...@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 chris1031...@gmail.com >> Signed-off-by: Christian Bartnik <chris1031...@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/} >> >> +@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 >> +@option{--enable-libvvenc}. >> + >> +The VVenC project website is at >> +@url{https://github.com/fraunhoferhhi/vvenc}. >> + >> +@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. >> + >> +@subsection Options >> + >> +@table @option >> +@item b >> +Sets target video bitrate. >> + >> +@item g >> +Set the GOP size. Currently support for g=1 (Intra only) or default. >> + >> +@item preset >> +Set the VVenC preset. >> + >> +@item levelidc >> +Set level idc. >> + >> +@item tier >> +Set vvc tier. >> + >> +@item qp >> +Set constant quantization parameter. >> + >> +@item subopt @var{boolean} >> +Set subjective (perceptually motivated) optimization. Default is 1 (on). >> + >> +@item bitdepth8 @var{boolean} >> +Set 8bit coding mode instead of using 10bit. Default is 0 (off). >> + >> +@item period >> +set (intra) refresh period in seconds. >> + >> +@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: >> + >> +@example >> +intraperiod=64:decodingrefreshtype=idr:poc0idr=1:internalbitdepth=8 >> +@end example >> + >> +For example the encoding options for 2-pass encoding might be provided with >> @option{-vvenc-params}: >> + >> +@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 >> +@end example >> + >> +@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? >> + 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); >> + } >> + >> + 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? >> + >> + *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? >> + { "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@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org <mailto:ffmpeg-devel-requ...@ffmpeg.org> with > subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".