Message ID | 20210204210220.219884-1-bohanli@google.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel] Add support for the new key & value API in libaom. | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
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".
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 }, };
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(+)