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

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

On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> 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

fate fails with X264_BUILD 152

The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
[vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
Failed to inject frame into filter network: Invalid argument
Error while filtering: Invalid argument
[out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
Conversion failed!
threads=1
tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
make: *** [fate-libx264-hdr10] Error 234


[...]

On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> > 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
>
> fate fails with X264_BUILD 152
>
> The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
> [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
> Failed to inject frame into filter network: Invalid argument
> Error while filtering: Invalid argument
> [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
> frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
> Conversion failed!
> threads=1
> tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
> make: *** [fate-libx264-hdr10] Error 234
>

Without having more information, that sounds more like a 8bit only
build rather than an X264_BUILD related issue, as the error seems to
come from a conversion from the input 10bit content to whatever not
being available.

Can you check if that is the case?

Jan

Jan Ekström:
> On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
>>
>> On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
>>> 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
>>
>> fate fails with X264_BUILD 152
>>
>> The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
>> [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
>> Failed to inject frame into filter network: Invalid argument
>> Error while filtering: Invalid argument
>> [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
>> frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
>> Conversion failed!
>> threads=1
>> tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
>> make: *** [fate-libx264-hdr10] Error 234
>>
> 
> Without having more information, that sounds more like a 8bit only
> build rather than an X264_BUILD related issue, as the error seems to
> come from a conversion from the input 10bit content to whatever not
> being available.
> 
> Can you check if that is the case?
> 

That is probably true, but your test nevertheless requires X264_BUILD >=
163, but the test requirements don't check for this.

- Andreas

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.

> +
> +    for (int i = 0; i < 3; i++) {
> +        const AVRational *src = mdcv->display_primaries[i];
> +        int *dst[2] = { points[i][0], points[i][1] };
> +
> +        *dst[0] = av_rescale_q(1, src[0], (AVRational){ 1, 50000 });
> +        *dst[1] = av_rescale_q(1, src[1], (AVRational){ 1, 50000 });
> +    }
> +
> +    params->mastering_display.i_white_x =
> +        av_rescale_q(1, mdcv->white_point[0], (AVRational){ 1, 50000 });
> +    params->mastering_display.i_white_y =
> +        av_rescale_q(1, mdcv->white_point[1], (AVRational){ 1, 50000 });
> +
> +skip_primaries:
> +    if (!mdcv->has_luminance)
> +        return;
> +
> +    params->mastering_display.i_display_max =
> +        av_rescale_q(1, mdcv->max_luminance, (AVRational){ 1, 10000 });
> +    params->mastering_display.i_display_min =
> +        av_rescale_q(1, mdcv->min_luminance, (AVRational){ 1, 10000 });
> +}
> +#endif // X264_BUILD >= 163
> +
> +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) { ... }

> +
> +    if (cll_sd) {
> +        const AVContentLightMetadata *cll =
> +            (AVContentLightMetadata *)cll_sd->data;
> +
> +        params->content_light_level.i_max_cll  = cll->MaxCLL;
> +        params->content_light_level.i_max_fall = cll->MaxFALL;
> +
> +        params->content_light_level.b_cll = 1;
> +    }
> +
> +    if (mdcv_sd) {
> +        handle_mdcv(params, (AVMasteringDisplayMetadata *)mdcv_sd->data);
> +    }
> +#endif // X264_BUILD >= 163
> +}
> +
>  static av_cold int X264_init(AVCodecContext *avctx)
>  {
>      X264Context *x4 = avctx->priv_data;
> @@ -1142,6 +1219,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      if (avctx->chroma_sample_location != AVCHROMA_LOC_UNSPECIFIED)
>          x4->params.vui.i_chroma_loc = avctx->chroma_sample_location - 1;
>  
> +    handle_side_data(avctx, &x4->params);
> +
>      if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER)
>          x4->params.b_repeat_headers = 0;
>  
> diff --git a/tests/fate/enc_external.mak b/tests/fate/enc_external.mak
> index d787941c16..90d8894d04 100644
> --- a/tests/fate/enc_external.mak
> +++ b/tests/fate/enc_external.mak
> @@ -7,5 +7,10 @@ FATE_ENC_EXTERNAL-$(call ENCDEC, LIBSVTAV1 HEVC, MOV, HEVC_DEMUXER LIBDAV1D_DECO
>  fate-libsvtav1-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
>      mp4 "-c:v libsvtav1" "-show_frames -show_entries frame=side_data_list -of flat"
>  
> +# test for x264 MDCV and CLL passthrough during encoding
> +FATE_ENC_EXTERNAL-$(call ENCDEC, LIBX264 HEVC, MOV, HEVC_DEMUXER H264_DECODER) += fate-libx264-hdr10
> +fate-libx264-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
> +    mp4 "-c:v libx264" "-show_frames -show_entries frame=side_data_list -of flat"
> +
>  FATE_SAMPLES_FFMPEG_FFPROBE += $(FATE_ENC_EXTERNAL-yes)
>  fate-enc-external: $(FATE_ENC_EXTERNAL-yes)
> diff --git a/tests/ref/fate/libx264-hdr10 b/tests/ref/fate/libx264-hdr10
> new file mode 100644
> index 0000000000..99c11677f0
> --- /dev/null
> +++ b/tests/ref/fate/libx264-hdr10
> @@ -0,0 +1,15 @@
> +frames.frame.0.side_data_list.side_data.0.side_data_type="H.26[45] User Data Unregistered SEI message"
> +frames.frame.0.side_data_list.side_data.1.side_data_type="Mastering display metadata"
> +frames.frame.0.side_data_list.side_data.1.red_x="13250/50000"
> +frames.frame.0.side_data_list.side_data.1.red_y="34500/50000"
> +frames.frame.0.side_data_list.side_data.1.green_x="7500/50000"
> +frames.frame.0.side_data_list.side_data.1.green_y="3000/50000"
> +frames.frame.0.side_data_list.side_data.1.blue_x="34000/50000"
> +frames.frame.0.side_data_list.side_data.1.blue_y="16000/50000"
> +frames.frame.0.side_data_list.side_data.1.white_point_x="15635/50000"
> +frames.frame.0.side_data_list.side_data.1.white_point_y="16450/50000"
> +frames.frame.0.side_data_list.side_data.1.min_luminance="50/10000"
> +frames.frame.0.side_data_list.side_data.1.max_luminance="10000000/10000"
> +frames.frame.0.side_data_list.side_data.2.side_data_type="Content light level metadata"
> +frames.frame.0.side_data_list.side_data.2.max_content=1000
> +frames.frame.0.side_data_list.side_data.2.max_average=200

On Sun, Aug 20, 2023 at 01:25:27AM +0300, Jan Ekström wrote:
> On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> > > 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
> >
> > fate fails with X264_BUILD 152
> >
> > The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
> > [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
> > Failed to inject frame into filter network: Invalid argument
> > Error while filtering: Invalid argument
> > [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
> > frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
> > Conversion failed!
> > threads=1
> > tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
> > make: *** [fate-libx264-hdr10] Error 234
> >
> 
> Without having more information, that sounds more like a 8bit only
> build rather than an X264_BUILD related issue, as the error seems to
> come from a conversion from the input 10bit content to whatever not
> being available.
> 
> Can you check if that is the case?

x264 152 was either 8bit or 10bit but not both. That box on which this
fails has x264 builds for both for 8bit and 10bit.
ffmpeg can at runtime be linked with either
for example
LD_PRELOAD=/usr/lib/x86_64-linux-gnu/x264-10bit/libx264.so.152 ./ffmpeg -i ~/videos/mm-short.mpg  test.mkv
produces a 10bit output

maybe you should just disable this test for 152 and prior to avoid this complexity

thx

[...]

On Sun, Aug 20, 2023 at 4:12 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sun, Aug 20, 2023 at 01:25:27AM +0300, Jan Ekström wrote:
> > On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
> > > > 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
> > >
> > > fate fails with X264_BUILD 152
> > >
> > > The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
> > > [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
> > > Failed to inject frame into filter network: Invalid argument
> > > Error while filtering: Invalid argument
> > > [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
> > > frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
> > > Conversion failed!
> > > threads=1
> > > tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
> > > make: *** [fate-libx264-hdr10] Error 234
> > >
> >
> > Without having more information, that sounds more like a 8bit only
> > build rather than an X264_BUILD related issue, as the error seems to
> > come from a conversion from the input 10bit content to whatever not
> > being available.
> >
> > Can you check if that is the case?
>
> x264 152 was either 8bit or 10bit but not both. That box on which this
> fails has x264 builds for both for 8bit and 10bit.
> ffmpeg can at runtime be linked with either
> for example
> LD_PRELOAD=/usr/lib/x86_64-linux-gnu/x264-10bit/libx264.so.152 ./ffmpeg -i ~/videos/mm-short.mpg  test.mkv
> produces a 10bit output
>
> maybe you should just disable this test for 152 and prior to avoid this complexity
>
> thx

Yup, before Vittorio allowed for both bit depths to be included that
indeed was the case, just didn't recall it was at X264_BUILD >= 153
where the work was done.

As for skipping, I was seemingly tired and focused more on the exact
error than the overall case. Indeed, >= 163 is required for the test
to pass, so some sort of disablement for older versions is required.

Is the simplest way to add a configure check for this, or can the
define be utilized in some manner from the test's Makefile?

Jan

Jan Ekström:
> On Sun, Aug 20, 2023 at 4:12 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
>>
>> On Sun, Aug 20, 2023 at 01:25:27AM +0300, Jan Ekström wrote:
>>> On Sat, Aug 19, 2023 at 7:53 PM Michael Niedermayer
>>> <michael@niedermayer.cc> wrote:
>>>>
>>>> On Fri, Aug 18, 2023 at 12:48:49AM +0300, Jan Ekström wrote:
>>>>> 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
>>>>
>>>> fate fails with X264_BUILD 152
>>>>
>>>> The filters 'Parsed_null_0' and 'format' do not have a common format and automatic conversion is disabled.
>>>> [vf#0:0 @ 0x55eddf8d4780] Error reinitializing filters!
>>>> Failed to inject frame into filter network: Invalid argument
>>>> Error while filtering: Invalid argument
>>>> [out#0/mp4 @ 0x55eddf87b980] Nothing was written into output file, because at least one of its streams received no packets.
>>>> frame=    0 fps=0.0 q=0.0 Lsize=       0kB time=N/A bitrate=N/A speed=N/A
>>>> Conversion failed!
>>>> threads=1
>>>> tests/Makefile:307: recipe for target 'fate-libx264-hdr10' failed
>>>> make: *** [fate-libx264-hdr10] Error 234
>>>>
>>>
>>> Without having more information, that sounds more like a 8bit only
>>> build rather than an X264_BUILD related issue, as the error seems to
>>> come from a conversion from the input 10bit content to whatever not
>>> being available.
>>>
>>> Can you check if that is the case?
>>
>> x264 152 was either 8bit or 10bit but not both. That box on which this
>> fails has x264 builds for both for 8bit and 10bit.
>> ffmpeg can at runtime be linked with either
>> for example
>> LD_PRELOAD=/usr/lib/x86_64-linux-gnu/x264-10bit/libx264.so.152 ./ffmpeg -i ~/videos/mm-short.mpg  test.mkv
>> produces a 10bit output
>>
>> maybe you should just disable this test for 152 and prior to avoid this complexity
>>
>> thx
> 
> Yup, before Vittorio allowed for both bit depths to be included that
> indeed was the case, just didn't recall it was at X264_BUILD >= 153
> where the work was done.
> 
> As for skipping, I was seemingly tired and focused more on the exact
> error than the overall case. Indeed, >= 163 is required for the test
> to pass, so some sort of disablement for older versions is required.
> 

Actually, x264 allows to select only eight or ten bit bitdepth (or both)
at configure-time. IIRC opening the encoder will fail if an eight-bit
only libx264 is used when one tries to use a 10bit pixel format.
(This is actually a regression since x264 version 153; with older
versions the codec's advertised pixel formats actually match the really
supported pixel formats.)

> Is the simplest way to add a configure check for this, or can the
> define be utilized in some manner from the test's Makefile?
>

On Sun, Aug 20, 2023 at 9:54 AM Andreas Rheinhardt
<andreas.rheinhardt@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

On Mon, Aug 21, 2023 at 9:49 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Actually, x264 allows to select only eight or ten bit bitdepth (or both)
> at configure-time. IIRC opening the encoder will fail if an eight-bit
> only libx264 is used when one tries to use a 10bit pixel format.
> (This is actually a regression since x264 version 153; with older
> versions the codec's advertised pixel formats actually match the really
> supported pixel formats.)
>

This changed a few years ago, x264 supports both bitdepths in a single
binary since 153 (hence why the codepath got updated like so)

Vittorio Giovara:
> On Mon, Aug 21, 2023 at 9:49 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
> 
>> Actually, x264 allows to select only eight or ten bit bitdepth (or both)
>> at configure-time. IIRC opening the encoder will fail if an eight-bit
>> only libx264 is used when one tries to use a 10bit pixel format.
>> (This is actually a regression since x264 version 153; with older
>> versions the codec's advertised pixel formats actually match the really
>> supported pixel formats.)
>>
> 
> This changed a few years ago, x264 supports both bitdepths in a single
> binary since 153 (hence why the codepath got updated like so)

But if you use an x264 build that only supports one bitdepth, you can
run into the regression I mentioned (and the test will fail if you use
an 8-bit only libx264).

- Andreas