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.
> +@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}.
> +@table @samp
> +@item psnr (@emph{0})
> +PSNR as distortion metric
> +
> +@item ssim (@emph{1})
> +SSIM as distortion metric
> +@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
_______________________________________________
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".
