[FFmpeg-devel] [PATCH 1/2] libavcodec/libaomenc.c: Add a libaom command-line opton 'tune'

[Moritz Barsnick]

Hi Wang,

On Wed, Mar 04, 2020 at 05:59:02 +0800, Wang Cao wrote:

> Subject: libavcodec/libaomenc.c: Add a libaom command-line opton 'tune'
                                                             ^
typo -> option

>  doc/encoders.texi      | 11 +++++++++++
>  libavcodec/libaomenc.c |  7 +++++++

I suggest also bumping libavcodec MICRO version with each addition of
options.

> + at item tune (@emph{tune})
> +Set the distortion metric tuned with for encoder. Default is PSNR.

The grammar sound a bit confusing to me. Perhaps:
  Set the distortion metric the encoder is tuned with.

Also, perhaps reference the default value, not the default behavior:
  Default is @code{psnr}.

> + at table @samp
> + at item psnr (@emph{0})
> +PSNR as distortion metric
> +
> + at item ssim (@emph{1})
> +SSIM as distortion metric
> + at end table

Probably no need to document the numerical values, but I don't really
mind.

> +    if (ctx->tune >= 0)
> +        codecctl_int(avctx, AOME_SET_TUNING, ctx->tune);
[...]
> +    { "tune",            "The metric that encoder tunes for", OFFSET(tune), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 1, VE, "tune"},
> +    { "psnr",            "PSNR as distortion metric",         0, AV_OPT_TYPE_CONST, {.i64 = 0}, 0, 0, VE, "tune"},
> +    { "ssim",            "SSIM as distortion metric",         0, AV_OPT_TYPE_CONST, {.i64 = 1}, 0, 0, VE, "tune"},

If "-1^" is the default, it's the encoder (library) that decides what
is default, right? Is this guaranteed to be PSNR? Or should we just
document "automatically chosen"?

Also, for consts, I suggest enums. I will comment on the second patch
(as there are only two values here, but the point may be just as
valid).

Cheers,
Moritz