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

[Moritz Barsnick]

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.

> + 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.

>      [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?)

>      { "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].

Cheers,
Moritz