[FFmpeg-devel] [PATCH] Add support for the new key & value API in libaom.

Steven Liu lingjiujianke at gmail.com
Tue Feb 9 04:31:41 EET 2021 

Bohan Li <bohanli-at-google.com at ffmpeg.org> 于2021年2月9日周二 上午8:24写道：
>
> 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?

You should remake a new version patch use git format-patch -v 2 -s -1
and use git send-email --to with --in-reply-to jeeb's comments mail
message-id.

>
> 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".
> _______________________________________________
> 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