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

This key & value API can greatly help with users who wants to try
libaom-av1 specific options that are not supported by ffmpeg options.

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@google.com>
---
 doc/encoders.texi      | 11 +++++++++++
 libavcodec/libaomenc.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

Thanks for the review! Indeed the buf string does not play any role here. I
have removed it in the new patch.

Bohan

On Thu, Feb 4, 2021 at 1:02 PM Bohan Li <bohanli@google.com> 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.
>
> 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@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.
>
> +@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}:
> +
> +@example
> +ffmpeg -i input -c:v libaom-av1 -b:v 500K -libaom-params
> tune=psnr:enable-tpl-model=1 output.mp4
> +@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);
> +
> +    res = aom_codec_set_option(&ctx->encoder, key, value);
> +    if (res != AOM_CODEC_OK) {
> +        log_encoder_error(avctx, key);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    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
> +    {
> +      AVDictionaryEntry *en = NULL;
> +      while ((en = av_dict_get(ctx->extra_params, "", en,
> AV_DICT_IGNORE_SUFFIX))) {
> +        codec_set_option(avctx, en->key, en->value);
> +      }
> +    }
> +#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 },
>      { NULL },
>  };
>
> --
> 2.30.0.365.g02bc693789-goog
>
>

Gentle ping :)

On Thu, Feb 4, 2021 at 1:45 PM Bohan Li <bohanli@google.com> wrote:

> Thanks for the review! Indeed the buf string does not play any role here.
> I have removed it in the new patch.
>
> Bohan
>
> On Thu, Feb 4, 2021 at 1:02 PM Bohan Li <bohanli@google.com> 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.
>>
>> 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@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.
>>
>> +@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}:
>> +
>> +@example
>> +ffmpeg -i input -c:v libaom-av1 -b:v 500K -libaom-params
>> tune=psnr:enable-tpl-model=1 output.mp4
>> +@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);
>> +
>> +    res = aom_codec_set_option(&ctx->encoder, key, value);
>> +    if (res != AOM_CODEC_OK) {
>> +        log_encoder_error(avctx, key);
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    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
>> +    {
>> +      AVDictionaryEntry *en = NULL;
>> +      while ((en = av_dict_get(ctx->extra_params, "", en,
>> AV_DICT_IGNORE_SUFFIX))) {
>> +        codec_set_option(avctx, en->key, en->value);
>> +      }
>> +    }
>> +#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 },
>>      { NULL },
>>  };
>>
>> --
>> 2.30.0.365.g02bc693789-goog
>>
>>

Quick question as a user that is not entirely related to this patch, would
it be possible to add something like a `ffmpeg ... -libaom-params help -f
null -` or someother way that would print out the options available in the
compiled libaom? (In the case where we do not have aomenc compiled or it's
compiled using a different libaom version)

On Mon, Feb 8, 2021 at 10:40 PM Christopher Degawa <ccom@randomderp.com> wrote:
>
> Quick question as a user that is not entirely related to this patch, would
> it be possible to add something like a `ffmpeg ... -libaom-params help -f
> null -` or someother way that would print out the options available in the
> compiled libaom? (In the case where we do not have aomenc compiled or it's
> compiled using a different libaom version)

I don't think this is currently possible, but I did hack up a quick
PoC for dynamic help entries once after someone #ffmpeg raised that
it'd be nice to have a listing of things.

https://github.com/jeeb/ffmpeg/commits/avoption_dynamic_help

This PoC just added the presets from libx265 into a dynamic help text
for the preset option, visible with `ffmpeg -h encoder=libx265`. Of
course this just goes into the explanation field, so not like the
alternatives in some other fields where it iterates over things.

Jan

On Thu, Feb 4, 2021 at 11:02 PM Bohan Li
<bohanli-at-google.com@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@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.
>
> +@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}:
> +
> +@example
> +ffmpeg -i input -c:v libaom-av1 -b:v 500K -libaom-params tune=psnr:enable-tpl-model=1 output.mp4
> +@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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@gmail.com> wrote:

> On Thu, Feb 4, 2021 at 11:02 PM Bohan Li
> <bohanli-at-google.com@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@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.
> >
> > +@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}:
> > +
> > +@example
> > +ffmpeg -i input -c:v libaom-av1 -b:v 500K -libaom-params
> tune=psnr:enable-tpl-model=1 output.mp4
> > +@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@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".

Bohan Li <bohanli-at-google.com@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@gmail.com> wrote:
>
> > On Thu, Feb 4, 2021 at 11:02 PM Bohan Li
> > <bohanli-at-google.com@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@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.
> > >
> > > +@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}:
> > > +
> > > +@example
> > > +ffmpeg -i input -c:v libaom-av1 -b:v 500K -libaom-params
> > tune=psnr:enable-tpl-model=1 output.mp4
> > > +@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@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@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-request@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-request@ffmpeg.org with subject "unsubscribe".