[FFmpeg-devel] [PATCH] avcodec/libaomenc: fix build w/libaom v1.0.0

James Almer jamrial at gmail.com
Thu Jul 2 17:03:07 EEST 2020


On 7/2/2020 12:01 AM, James Zern wrote:
> broken since:
> aa5c6f382b avcodec/libaomenc: Add command-line options to control the use of partition tools
> 
> + remove control related options when it's unavailable
> 
> Signed-off-by: James Zern <jzern at google.com>
> ---
>  libavcodec/libaomenc.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
> index cb6558476c..fc9182003e 100644
> --- a/libavcodec/libaomenc.c
> +++ b/libavcodec/libaomenc.c
> @@ -145,16 +145,36 @@ static const char *const ctlidstr[] = {
>  #endif
>      [AV1E_SET_ENABLE_CDEF]      = "AV1E_SET_ENABLE_CDEF",
>      [AOME_SET_TUNING]           = "AOME_SET_TUNING",
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_1TO4_PARTITIONS
>      [AV1E_SET_ENABLE_1TO4_PARTITIONS] = "AV1E_SET_ENABLE_1TO4_PARTITIONS",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_AB_PARTITIONS
>      [AV1E_SET_ENABLE_AB_PARTITIONS]   = "AV1E_SET_ENABLE_AB_PARTITIONS",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_RECT_PARTITIONS
>      [AV1E_SET_ENABLE_RECT_PARTITIONS] = "AV1E_SET_ENABLE_RECT_PARTITIONS",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_ANGLE_DELTA
>      [AV1E_SET_ENABLE_ANGLE_DELTA]       = "AV1E_SET_ENABLE_ANGLE_DELTA",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_CFL_INTRA
>      [AV1E_SET_ENABLE_CFL_INTRA]         = "AV1E_SET_ENABLE_CFL_INTRA",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_FILTER_INTRA
>      [AV1E_SET_ENABLE_FILTER_INTRA]      = "AV1E_SET_ENABLE_FILTER_INTRA",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_INTRA_EDGE_FILTER
>      [AV1E_SET_ENABLE_INTRA_EDGE_FILTER] = "AV1E_SET_ENABLE_INTRA_EDGE_FILTER",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_PAETH_INTRA
>      [AV1E_SET_ENABLE_PAETH_INTRA]       = "AV1E_SET_ENABLE_PAETH_INTRA",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_SMOOTH_INTRA
>      [AV1E_SET_ENABLE_SMOOTH_INTRA]      = "AV1E_SET_ENABLE_SMOOTH_INTRA",
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_PALETTE
>      [AV1E_SET_ENABLE_PALETTE]           = "AV1E_SET_ENABLE_PALETTE",
> +#endif

Can't this be simplified by a AOM_ENCODER_ABI_VERSION check that ensures
all of these are present? For the sake of cleanness.

If they are in libaom 2.0.0, then that should be enough.

>  };
>  
>  static av_cold void log_encoder_error(AVCodecContext *avctx, const char *desc)
> @@ -718,26 +738,46 @@ static av_cold int aom_init(AVCodecContext *avctx,
>          codecctl_int(avctx, AV1E_SET_ENABLE_CDEF, ctx->enable_cdef);
>      if (ctx->enable_restoration >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_RESTORATION, ctx->enable_restoration);
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_RECT_PARTITIONS
>      if (ctx->enable_rect_partitions >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_RECT_PARTITIONS, ctx->enable_rect_partitions);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_1TO4_PARTITIONS
>      if (ctx->enable_1to4_partitions >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_1TO4_PARTITIONS, ctx->enable_1to4_partitions);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_AB_PARTITIONS
>      if (ctx->enable_ab_partitions >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_AB_PARTITIONS, ctx->enable_ab_partitions);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_ANGLE_DELTA
>      if (ctx->enable_angle_delta >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_ANGLE_DELTA, ctx->enable_angle_delta);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_CFL_INTRA
>      if (ctx->enable_cfl_intra >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_CFL_INTRA, ctx->enable_cfl_intra);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_FILTER_INTRA
>      if (ctx->enable_filter_intra >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_FILTER_INTRA, ctx->enable_filter_intra);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_INTRA_EDGE_FILTER
>      if (ctx->enable_intra_edge_filter >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_INTRA_EDGE_FILTER, ctx->enable_intra_edge_filter);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_PAETH_INTRA
>      if (ctx->enable_paeth_intra >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_PAETH_INTRA, ctx->enable_paeth_intra);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_SMOOTH_INTRA
>      if (ctx->enable_smooth_intra >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_SMOOTH_INTRA, ctx->enable_smooth_intra);
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_PALETTE
>      if (ctx->enable_palette >= 0)
>          codecctl_int(avctx, AV1E_SET_ENABLE_PALETTE, ctx->enable_palette);
> +#endif

Same.

>  
>      codecctl_int(avctx, AOME_SET_STATIC_THRESHOLD, ctx->static_thresh);
>      if (ctx->crf >= 0)
> @@ -1126,8 +1166,12 @@ static const AVOption options[] = {
>      { "crf",              "Select the quality for constant quality mode", offsetof(AOMContext, crf), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 63, VE },
>      { "static-thresh",    "A change threshold on blocks below which they will be skipped by the encoder", OFFSET(static_thresh), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
>      { "drop-threshold",   "Frame drop threshold", offsetof(AOMContext, drop_threshold), AV_OPT_TYPE_INT, {.i64 = 0 }, INT_MIN, INT_MAX, VE },
> +#ifdef AOM_CTRL_AV1E_SET_DENOISE_NOISE_LEVEL
>      { "denoise-noise-level", "Amount of noise to be removed", OFFSET(denoise_noise_level), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_DENOISE_BLOCK_SIZE
>      { "denoise-block-size", "Denoise block size ", OFFSET(denoise_block_size), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, VE},
> +#endif
>      { "undershoot-pct",   "Datarate undershoot (min) target (%)", OFFSET(rc_undershoot_pct), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 100, VE},
>      { "overshoot-pct",    "Datarate overshoot (max) target (%)", OFFSET(rc_overshoot_pct), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 1000, VE},
>      { "minsection-pct",   "GOP min bitrate (% of target)", OFFSET(minsection_pct), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 100, VE},
> @@ -1136,10 +1180,16 @@ static const AVOption options[] = {
>      { "tiles",            "Tile columns x rows", OFFSET(tile_cols), AV_OPT_TYPE_IMAGE_SIZE, { .str = NULL }, 0, 0, VE },
>      { "tile-columns",     "Log2 of number of tile columns to use", OFFSET(tile_cols_log2), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 6, VE},
>      { "tile-rows",        "Log2 of number of tile rows to use",    OFFSET(tile_rows_log2), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 6, VE},
> +#ifdef AOM_CTRL_AV1E_SET_ROW_MT
>      { "row-mt",           "Enable row based multi-threading",      OFFSET(row_mt),         AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
>      { "enable-cdef",      "Enable CDEF filtering",                 OFFSET(enable_cdef),    AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_GLOBAL_MOTION
>      { "enable-global-motion",  "Enable global motion",             OFFSET(enable_global_motion), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_INTRABC
>      { "enable-intrabc",  "Enable intra block copy prediction mode", OFFSET(enable_intrabc), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif

No, keep these as is. Changing them now is a breaking change for no real
gain.

>      { "enable-restoration", "Enable Loop Restoration filtering", OFFSET(enable_restoration), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
>      { "usage",           "Quality and compression efficiency vs speed trade-off", 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"},
> @@ -1148,16 +1198,36 @@ static const AVOption options[] = {
>      { "psnr",            NULL,         0, AV_OPT_TYPE_CONST, {.i64 = AOM_TUNE_PSNR}, 0, 0, VE, "tune"},
>      { "ssim",            NULL,         0, AV_OPT_TYPE_CONST, {.i64 = AOM_TUNE_SSIM}, 0, 0, VE, "tune"},
>      FF_AV1_PROFILE_OPTS
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_RECT_PARTITIONS
>      { "enable-rect-partitions", "Enable rectangular partitions", OFFSET(enable_rect_partitions), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_1TO4_PARTITIONS
>      { "enable-1to4-partitions", "Enable 1:4/4:1 partitions",     OFFSET(enable_1to4_partitions), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_AB_PARTITIONS
>      { "enable-ab-partitions",   "Enable ab shape partitions",    OFFSET(enable_ab_partitions),   AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_ANGLE_DELTA
>      { "enable-angle-delta",       "Enable angle delta intra prediction",                OFFSET(enable_angle_delta),       AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_CFL_INTRA
>      { "enable-cfl-intra",         "Enable chroma predicted from luma intra prediction", OFFSET(enable_cfl_intra),         AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_FILTER_INTRA
>      { "enable-filter-intra",      "Enable filter intra predictor",                      OFFSET(enable_filter_intra),      AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_INTRA_EDGE_FILTER
>      { "enable-intra-edge-filter", "Enable intra edge filter",                           OFFSET(enable_intra_edge_filter), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_SMOOTH_INTRA
>      { "enable-smooth-intra",      "Enable smooth intra prediction mode",                OFFSET(enable_smooth_intra),      AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_PAETH_INTRA
>      { "enable-paeth-intra",       "Enable paeth predictor in intra prediction",         OFFSET(enable_paeth_intra),       AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif
> +#ifdef AOM_CTRL_AV1E_SET_ENABLE_PALETTE
>      { "enable-palette",           "Enable palette prediction mode",                     OFFSET(enable_palette),           AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE},
> +#endif

So far we haven't wrapped options in pre processor checks, ensuring they
are always present regardless of what version of the library we link to.
This means they act as no-ops when the relevant feature is not present.
It's less of a headache for users that way.

Ideally a notice would be added in doc/encoders.texi to each of them,
stating the required minimum version, like it was done for row-mt. But
if you prefer to hide these new options, then i wont be against it. The
comment about a simpler check also applies in that case.


More information about the ffmpeg-devel mailing list