diff mbox series

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

Message ID 20200303215903.46180-1-wangcao@google.com
State New
Headers show
Series [FFmpeg-devel,1/2] libavcodec/libaomenc.c: Add a libaom command-line opton 'tune' | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Wang Cao March 3, 2020, 9:59 p.m. UTC
Signed-off-by: Wang Cao <wangcao@google.com>
---
 doc/encoders.texi      | 11 +++++++++++
 libavcodec/libaomenc.c |  7 +++++++
 2 files changed, 18 insertions(+)

Comments

Moritz Barsnick March 4, 2020, 9:38 a.m. UTC | #1
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
diff mbox series

Patch

diff --git a/doc/encoders.texi b/doc/encoders.texi
index e23b6b32fe..4215f237bd 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -1508,6 +1508,17 @@  Complexity-based.
 Cyclic refresh.
 @end table
 
+@item tune (@emph{tune})
+Set the distortion metric tuned with for encoder. Default is PSNR.
+
+@table @samp
+@item psnr (@emph{0})
+PSNR as distortion metric
+
+@item ssim (@emph{1})
+SSIM as distortion metric
+@end table
+
 @item lag-in-frames
 Set the maximum number of frames which the encoder may keep in flight
 at any one time for lookahead purposes.  Defaults to the internal
diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index 096aadbe1c..7fd624c470 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -94,6 +94,7 @@  typedef struct AOMEncoderContext {
     int enable_intrabc;
     int enable_restoration;
     int usage;
+    int tune;
 } AOMContext;
 
 static const char *const ctlidstr[] = {
@@ -132,6 +133,7 @@  static const char *const ctlidstr[] = {
     [AV1E_SET_ENABLE_INTRABC]   = "AV1E_SET_ENABLE_INTRABC",
 #endif
     [AV1E_SET_ENABLE_CDEF]      = "AV1E_SET_ENABLE_CDEF",
+    [AOME_SET_TUNING]           = "AOME_SET_TUNING",
 };
 
 static av_cold void log_encoder_error(AVCodecContext *avctx, const char *desc)
@@ -699,6 +701,8 @@  static av_cold int aom_init(AVCodecContext *avctx,
     codecctl_int(avctx, AOME_SET_STATIC_THRESHOLD, ctx->static_thresh);
     if (ctx->crf >= 0)
         codecctl_int(avctx, AOME_SET_CQ_LEVEL,          ctx->crf);
+    if (ctx->tune >= 0)
+        codecctl_int(avctx, AOME_SET_TUNING, ctx->tune);
 
     codecctl_int(avctx, AV1E_SET_COLOR_PRIMARIES, avctx->color_primaries);
     codecctl_int(avctx, AV1E_SET_MATRIX_COEFFICIENTS, avctx->colorspace);
@@ -1096,6 +1100,9 @@  static const AVOption options[] = {
     { "usage",           "Quality and compression efficiency vs speed tradeof", OFFSET(usage), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, VE, "usage"},
     { "good",            "Good quality",      0, AV_OPT_TYPE_CONST, {.i64 = 0 /* AOM_USAGE_GOOD_QUALITY */}, 0, 0, VE, "usage"},
     { "realtime",        "Realtime encoding", 0, AV_OPT_TYPE_CONST, {.i64 = 1 /* AOM_USAGE_REALTIME */},     0, 0, VE, "usage"},
+    { "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"},
     { NULL },
 };