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 <jee...@gmail.com> wrote: > On Thu, Feb 4, 2021 at 11:02 PM Bohan Li > <bohanli-at-google....@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 <boha...@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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
