[FFmpeg-devel] [PATCH 2/2] libavcodec/libaomenc.c: Add super-resolution options to libaom wrapper

[Wang Cao]

On Wed, Mar 4, 2020 at 1:38 AM Moritz Barsnick <barsnick at gmx.net> wrote:

> On Wed, Mar 04, 2020 at 05:59:03 +0800, Wang Cao wrote:
> > Signed-off-by: Wang Cao <wangcao at google.com>
> > ---
> >  doc/encoders.texi      | 39 +++++++++++++++++++++++++++++++++++++++
> >  libavcodec/libaomenc.c | 38 ++++++++++++++++++++++++++++++++++++++
>
> Again, I suggest bumping MICRO once more.
>
Done.

> > + at item superres_denominator
> > +The denominator for superres to use when @option{superres-mode} is
> @option{fixed}. Valid value
> > +ranges from 8 to 16.
> > +
> > + at item superres_kf_denominator
> > +The denominator for superres to use on key frames is fixed when
> > + at option{superres-mode} is @option{fixed}. Valid value ranges from 8 to
> 16.
>
> I believe "is fixed " is not needed in this sentence, or even
> confusing.
>
Done.

> >      [AOME_SET_TUNING]           = "AOME_SET_TUNING",
> > +    [AV1E_SET_ENABLE_SUPERRES] = "AV1E_SET_ENABLE_SUPERRES",
>
> The other '=' in this block are aligned.
>
> > +    if (ctx->superres_mode >= 0)
> > +      enccfg.rc_superres_mode = ctx->superres_mode;
> > +    if (ctx->superres_qthresh > 0)
> > +      enccfg.rc_superres_qthresh = ctx->superres_qthresh;
> > +    if (ctx->superres_kf_qthresh > 0)
> > +      enccfg.rc_superres_kf_qthresh = ctx->superres_kf_qthresh;
> > +    if (ctx->superres_denominator >= 0)
> > +      enccfg.rc_superres_denominator = ctx->superres_denominator;
> > +    if (ctx->superres_kf_denominator >= 0)
> > +      enccfg.rc_superres_kf_denominator = ctx->superres_kf_denominator;
>
> It looks like indentation is off - ffmpeg uses four spaces.
>
> (Is this struct zero-initialized / memset()'d?)

Yes it is initialized before through aom_codec_enc_config_default

>
>
>      { "ssim",            "SSIM as distortion metric",         0,
> AV_OPT_TYPE_CONST, {.i64 = 1}, 0, 0, VE, "tune"},
> > +    { "enable-superres", "Enable super-resolution mode",
> OFFSET(enable_superres), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> > +    { "superres-mode",   "Select super-resultion mode",
> OFFSET(superres_mode), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 4, VE,
> "superres_mode"},
> > +    { "none",            "No frame superres allowed",         0,
> AV_OPT_TYPE_CONST, {.i64 = 0}, 0, 0, VE, "superres_mode"},
> > +    { "fixed",           "All frames are coded at the specified scale
> and super-resolved", 0, AV_OPT_TYPE_CONST, {.i64 = 1}, 0, 0, VE,
> "superres_mode"},
> > +    { "random",          "All frames are coded at a random scale and
> super-resolved.",     0, AV_OPT_TYPE_CONST, {.i64 = 2}, 0, 0, VE,
> "superres_mode"},
> > +    { "qthresh",         "Superres scale for a frame is determined
> based on q_index",      0, AV_OPT_TYPE_CONST, {.i64 = 3}, 0, 0, VE,
> "superres_mode"},
> > +    { "auto",            "Automatically select superres for appropriate
> frames",           0, AV_OPT_TYPE_CONST, {.i64 = 4}, 0, 0, VE,
> "superres_mode"},
>
> Several remarks:
> - The "none" entry should also be aligned, just like the entry "fixed"
>   and following... (starting at   "0, AV_OPT_TYPE_CONST", if you see
>   what I mean).
> - Is "auto" a value given by the library? I'm asking because
>   we tend to use "-1" for our internal "auto", and use that as a
>   default, if desired.
>   (From looking at libaom, they indeed use 4 for "auto".)
> - Can you directly use the enumerations provided as SUPERRES_MODE by
>   libaom, or are they not public?
> - If you cannot reuse said enum (and its resulting range
>   [-1, SUPERRES_MODES - 1]), you should define your own as enum
>   SuperresModes { AOM_SUPERRES_MODE_NONE, AOM_SUPERRES_MODE_FIXED, ....,
>   AOM_SUPERRES_MODE_NB }, use these in the options definition above,
>   and set the allowed range to [-1, AOM_SUPERRES_MODE_NB - 1].
>
> This is a good approach to improve the readability. Done.

> Cheers,
> Moritz
>
Please find the changes in the updated patch. Thanks!
-- 

Wang Cao |  Software Engineer |  wangcao at google.com |  650-203-7807