[FFmpeg-devel] [PATCH] Add support for the new key & value API in libaom.
Bohan Li
bohanli at google.com
Tue Feb 9 02:23:39 EET 2021
Thank you very much for the reviews and the fixes, Jan!
I went over the fixup commit and I do agree with the patches you proposed.
It looks good to me.
Just to make sure I don't misunderstand, should I submit a new patch with
the modifications you mentioned, or leave the patch as is and you would
modify and apply it?
Thanks a lot!
Bohan
On Mon, Feb 8, 2021 at 3:47 PM Jan Ekström <jeebjp at gmail.com> wrote:
> 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".
> _______________________________________________
> 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