On Thu, Jul 2, 2020 at 7:33 AM James Almer <jamr...@gmail.com> wrote:
>
> 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 <jz...@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.
>
Sure, that will work.
> > };
> >
> > 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.
>
Done.
> >
> > 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.
>
Reverted.
> > { "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.
>
Having an option do nothing in some versions can be a little confusing
too, but I agree being able to point out the difference in behavior in
documentation is enough.
_______________________________________________
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".
