[FFmpeg-devel] [PATCH] Add support for the new key & value API in libaom.
Jan Ekström
jeebjp at gmail.com
Tue Feb 9 01:47:10 EET 2021
On Thu, Feb 4, 2021 at 11:02 PM Bohan Li
<bohanli-at-google.com at ffmpeg.org> wrote:
>
> This key & value API can greatly help with users who wants to try
> libaom-av1 specific options that are not supported by ffmpeg options.
>
Excellent! Thank you for moving this forward :) .
I noticed various things which I will also notice here, but made a possible
fixup commit on a branch:
https://github.com/jeeb/ffmpeg/commits/libaom_key_value_api
If you think that is OK (in case I didn't mess anything up), I can
trim the commit message a bit and we should be done with this :) .
> As was previously discussed in this thread:
> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-October/271658.
>
> The commit that added the API to libaom:
> https://aomedia.googlesource.com/aom/+/c1d42fe6615c96fc929257
>
> The libaom issue tracker:
> https://bugs.chromium.org/p/aomedia/issues/detail?id=2875
>
> Signed-off-by: Bohan Li <bohanli at google.com>
> ---
> doc/encoders.texi | 11 +++++++++++
> libavcodec/libaomenc.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 41 insertions(+)
>
> diff --git a/doc/encoders.texi b/doc/encoders.texi
> index c2ba7d3e6f..9fab512892 100644
> --- a/doc/encoders.texi
> +++ b/doc/encoders.texi
> @@ -1684,6 +1684,17 @@ Enable interintra compound. Default is true.
> @item enable-smooth-interintra (@emph{boolean}) (Requires libaom >= v2.0.0)
> Enable smooth interintra mode. Default is true.
>
> + at item libaom-params
> +Set libaom options using a list of @var{key}=@var{value} pairs separated
> +by ":". For a list of supported options, see @command{aomenc --help} under the
> +section "AV1 Specific Options".
> +
> +For example to specify libaom encoding options with @option{-libaom-params}:
> +
> + at example
> +ffmpeg -i input -c:v libaom-av1 -b:v 500K -libaom-params tune=psnr:enable-tpl-model=1 output.mp4
> + at end example
> +
> @end table
>
> @section libsvtav1
> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> index 342d0883e4..c7a87e01cd 100644
> --- a/libavcodec/libaomenc.c
> +++ b/libavcodec/libaomenc.c
> @@ -124,6 +124,7 @@ typedef struct AOMEncoderContext {
> int enable_diff_wtd_comp;
> int enable_dist_wtd_comp;
> int enable_dual_filter;
> + AVDictionary *extra_params;
> } AOMContext;
>
> static const char *const ctlidstr[] = {
> @@ -318,6 +319,25 @@ static av_cold int codecctl_int(AVCodecContext *avctx,
> return 0;
> }
>
> +static av_cold int codec_set_option(AVCodecContext *avctx,
> + const char* key,
> + const char* value)
> +{
> + AOMContext *ctx = avctx->priv_data;
> + int width = -30;
> + int res;
> +
> + av_log(avctx, AV_LOG_DEBUG, " %*s: %s\n", width, key, value);
> +
My guess this was a left-over from some debugging. I think the width
and debug log line can be removed if log_encoder_error gives one a
useful error in the log?
> + res = aom_codec_set_option(&ctx->encoder, key, value);
> + if (res != AOM_CODEC_OK) {
> + log_encoder_error(avctx, key);
> + return AVERROR(EINVAL);
AVERROR_EXTERNAL seems to be utilized when something fails inside an
external library. To be fair, in this case maybe if we had separate
paths for AOM_CODEC_INVALID_PARAM and AOM_CODEC_ERROR we could utilize
EINVAL for one of them? But if we just handle both of them then
AVERROR_EXTERNAL might match both?
> + }
> +
> + return 0;
> +}
> +
> static av_cold int aom_free(AVCodecContext *avctx)
> {
> AOMContext *ctx = avctx->priv_data;
> @@ -874,6 +894,15 @@ static av_cold int aom_init(AVCodecContext *avctx,
> codecctl_int(avctx, AV1E_SET_ENABLE_INTRABC, ctx->enable_intrabc);
> #endif
>
> +#if AOM_ENCODER_ABI_VERSION >= 23
> + {
Indentation 2 VS 4 in this block here :) Probably just left-over
options from somewhere.
> + AVDictionaryEntry *en = NULL;
> + while ((en = av_dict_get(ctx->extra_params, "", en, AV_DICT_IGNORE_SUFFIX))) {
> + codec_set_option(avctx, en->key, en->value);
This function does return an error, yet it is not handled here and
passed on. I will guess this is just a forgotten case, and not that
you want to specifically ignore those errors.
Thus in the review notes commit I just moved the little code that was
in codec_set_option to within this loop.
> + }
> + }
> +#endif
> +
> // provide dummy value to initialize wrapper, values will be updated each _encode()
> aom_img_wrap(&ctx->rawimg, img_fmt, avctx->width, avctx->height, 1,
> (unsigned char*)1);
> @@ -1299,6 +1328,7 @@ static const AVOption options[] = {
> { "enable-masked-comp", "Enable masked compound", OFFSET(enable_masked_comp), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> { "enable-interintra-comp", "Enable interintra compound", OFFSET(enable_interintra_comp), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> { "enable-smooth-interintra", "Enable smooth interintra mode", OFFSET(enable_smooth_interintra), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> + { "libaom-params", "Extra parameters for libaom", OFFSET(extra_params), AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE },
I think we could follow the style of x264/x265/rav1e wrappers and skip
the "lib" bit and make it "aom-params". Also the help string could be
improved a bit, an example is in the review notes commit.
Additionally, we could limit the definition of this option to when the
feature is actually available. Having it exposed but not usable is not
really fun :) .
Jan
> { NULL },
> };
>
> --
> 2.30.0.365.g02bc693789-goog
>
> _______________________________________________
> 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