On Wed, Mar 4, 2020 at 1:38 AM Moritz Barsnick <barsn...@gmx.net> wrote:
> On Wed, Mar 04, 2020 at 05:59:03 +0800, Wang Cao wrote:
> > Signed-off-by: Wang Cao <wang...@google.com>
> > ---
> > doc/encoders.texi | 39 +++++++++++++++++++++++++++++++++++++++
> > libavcodec/libaomenc.c | 38 ++++++++++++++++++++++++++++++++++++++
>
> Again, I suggest bumping MICRO once more.
>
Done.
> > +@item superres_denominator
> > +The denominator for superres to use when @option{superres-mode} is
> @option{fixed}. Valid value
> > +ranges from 8 to 16.
> > +
> > +@item superres_kf_denominator
> > +The denominator for superres to use on key frames is fixed when
> > +@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 | wang...@google.com | 650-203-7807
_______________________________________________
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".
