[FFmpeg-devel] [PATCH v3 11/12] avcodec/libx264: add support for writing out CLL and MDCV

[Jan Ekström]

On Sun, Aug 20, 2023 at 9:54 AM Andreas Rheinhardt
<andreas.rheinhardt at outlook.com> wrote:
>
> Jan Ekström:
> > Both of these two structures were first available with X264_BUILD
> > 163, so make relevant functionality conditional on the version
> > being at least such.
> >
> > Keep handle_side_data available in all cases as this way X264_init
> > does not require additional version based conditions within it.
> >
> > Finally, add a FATE test which verifies that pass-through of the
> > MDCV/CLL side data is working during encoding.
> > ---
> >  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
> >  tests/fate/enc_external.mak  |  5 +++
> >  tests/ref/fate/libx264-hdr10 | 15 +++++++
> >  3 files changed, 99 insertions(+)
> >  create mode 100644 tests/ref/fate/libx264-hdr10
> >
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index 1a7dc7bdd5..30ea3dae4c 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -25,6 +25,7 @@
> >  #include "libavutil/eval.h"
> >  #include "libavutil/internal.h"
> >  #include "libavutil/opt.h"
> > +#include "libavutil/mastering_display_metadata.h"
> >  #include "libavutil/mem.h"
> >  #include "libavutil/pixdesc.h"
> >  #include "libavutil/stereo3d.h"
> > @@ -842,6 +843,82 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
> >          return AVERROR(EINVAL);\
> >      }
> >
> > +#if X264_BUILD >= 163
> > +static void handle_mdcv(x264_param_t *params,
> > +                        const AVMasteringDisplayMetadata *mdcv)
> > +{
> > +    int *points[][2] = {
> > +        {
> > +            &params->mastering_display.i_red_x,
> > +            &params->mastering_display.i_red_y
> > +        },
> > +        {
> > +            &params->mastering_display.i_green_x,
> > +            &params->mastering_display.i_green_y
> > +        },
> > +        {
> > +            &params->mastering_display.i_blue_x,
> > +            &params->mastering_display.i_blue_y
> > +        },
> > +    };
> > +
> > +    if (!mdcv->has_primaries && !mdcv->has_luminance)
> > +        return;
> > +
> > +    params->mastering_display.b_mastering_display = 1;
> > +
> > +    if (!mdcv->has_primaries)
> > +        goto skip_primaries;
>
> Normally we try to avoid gotos for non-error stuff. You are basically
> replacing an ordinary "if" here.
>

Yes, I think this was mostly me attempting to follow the "early exit
if possible" best practice, which mostly brings usefulness that the
following code no longer needs to be think about that condition (which
often then leads to less indentation etc).

You might be right that checking it the other way and just putting the
contents into the if might be better in this spot, will adjust and
check.


> > +static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
> > +{
> > +#if X264_BUILD >= 163
> > +    const AVFrameSideDataSet set = avctx->side_data_set;
> > +    const AVFrameSideData *cll_sd =
> > +        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> > +    const AVFrameSideData *mdcv_sd =
> > +        av_side_data_set_get_item(set, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
>
> You can improve code locality by not using two variables for the side
> data, but only one:
> side_data = av_side_data_set_get_item(set,
> AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> if (side_data) { ... }
> side_data = av_side_data_set_get_item(set,
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> if (side_data) { ... }

This is something where I wonder whether this actually brings
performance benefits, and whether those are worth it VS the separation
of the pre-optimization variable names?

Jan