diff mbox series

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

Message ID 20240227221226.1377758-13-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel,v6,01/13] avutil/frame: split side data list wiping out to non-AVFrame function | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Jan Ekström Feb. 27, 2024, 10:12 p.m. UTC 
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.
---
 configure                    |  2 +
 libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak  |  5 +++
 tests/ref/fate/libx264-hdr10 | 15 +++++++
 4 files changed, 101 insertions(+)
 create mode 100644 tests/ref/fate/libx264-hdr10

Comments

Andreas Rheinhardt Feb. 27, 2024, 10:26 p.m. UTC | #1 
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.
> ---
>  configure                    |  2 +
>  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
>  tests/fate/enc_external.mak  |  5 +++
>  tests/ref/fate/libx264-hdr10 | 15 +++++++
>  4 files changed, 101 insertions(+)
>  create mode 100644 tests/ref/fate/libx264-hdr10
> 
> diff --git a/configure b/configure
> index bb5e630bad..f48cf46ffe 100755
> --- a/configure
> +++ b/configure
> @@ -2527,6 +2527,7 @@ CONFIG_EXTRA="
>      jpegtables
>      lgplv3
>      libx262
> +    libx264_hdr10
>      llauddsp
>      llviddsp
>      llvidencdsp
> @@ -6927,6 +6928,7 @@ enabled libx264           && require_pkg_config libx264 x264 "stdint.h x264.h" x
>                               require_cpp_condition libx264 x264.h "X264_BUILD >= 122" && {
>                               [ "$toolchain" != "msvc" ] ||
>                               require_cpp_condition libx264 x264.h "X264_BUILD >= 158"; } &&
> +                             check_cpp_condition libx264_hdr10 x264.h "X264_BUILD >= 163" &&

Why don't you just check X264_BUILD in libx264.c instead of adding this
to configure?

>                               check_cpp_condition libx262 x264.h "X264_MPEG2"
>  enabled libx265           && require_pkg_config libx265 x265 x265.h x265_api_get &&
>                               require_cpp_condition libx265 x265.h "X265_BUILD >= 89"
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 10d646bd76..effdcfdb37 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"
> @@ -871,6 +872,82 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
>          return AVERROR(EINVAL);\
>      }
>  
> +#if CONFIG_LIBX264_HDR10
> +static void handle_mdcv(x264_param_t *params,
> +                        const AVMasteringDisplayMetadata *mdcv)
> +{
> +    if (!mdcv->has_primaries && !mdcv->has_luminance)
> +        return;
> +
> +    params->mastering_display.b_mastering_display = 1;
> +
> +    if (mdcv->has_primaries) {
> +        int *const 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
> +            },
> +        };
> +
> +        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 });
> +    }
> +
> +    if (mdcv->has_luminance) {
> +        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 // CONFIG_LIBX264_HDR10
> +
> +static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
> +{
> +#if CONFIG_LIBX264_HDR10
> +    const AVFrameSideData *cll_sd =
> +        av_frame_side_data_get(
> +            (const AVFrameSideData **)avctx->frame_side_data,
> +            avctx->nb_frame_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> +    const AVFrameSideData *mdcv_sd =
> +        av_frame_side_data_get(
> +            (const AVFrameSideData **)avctx->frame_side_data,
> +            avctx->nb_frame_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> +
> +    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 // CONFIG_LIBX264_HDR10
> +}
> +
>  static av_cold int X264_init(AVCodecContext *avctx)
>  {
>      X264Context *x4 = avctx->priv_data;
> @@ -1171,6 +1248,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..4095a4b51a 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, LIBX264_HDR10 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
Jan Ekström Feb. 27, 2024, 11:39 p.m. UTC | #2 
On Wed, Feb 28, 2024 at 12:24 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.
> > ---
> >  configure                    |  2 +
> >  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
> >  tests/fate/enc_external.mak  |  5 +++
> >  tests/ref/fate/libx264-hdr10 | 15 +++++++
> >  4 files changed, 101 insertions(+)
> >  create mode 100644 tests/ref/fate/libx264-hdr10
> >
> > diff --git a/configure b/configure
> > index bb5e630bad..f48cf46ffe 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2527,6 +2527,7 @@ CONFIG_EXTRA="
> >      jpegtables
> >      lgplv3
> >      libx262
> > +    libx264_hdr10
> >      llauddsp
> >      llviddsp
> >      llvidencdsp
> > @@ -6927,6 +6928,7 @@ enabled libx264           && require_pkg_config libx264 x264 "stdint.h x264.h" x
> >                               require_cpp_condition libx264 x264.h "X264_BUILD >= 122" && {
> >                               [ "$toolchain" != "msvc" ] ||
> >                               require_cpp_condition libx264 x264.h "X264_BUILD >= 158"; } &&
> > +                             check_cpp_condition libx264_hdr10 x264.h "X264_BUILD >= 163" &&
>
> Why don't you just check X264_BUILD in libx264.c instead of adding this
> to configure?
>

Most likely due to months ago it feeling like a cleaner option for
whatever reason (less magical numbers). Although in theory you can get
a similar named define by checking it within the module, yes.

Jan
Jan Ekström Feb. 28, 2024, 6:07 p.m. UTC | #3 
On Wed, Feb 28, 2024 at 1:39 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Wed, Feb 28, 2024 at 12:24 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.
> > > ---
> > >  configure                    |  2 +
> > >  libavcodec/libx264.c         | 79 ++++++++++++++++++++++++++++++++++++
> > >  tests/fate/enc_external.mak  |  5 +++
> > >  tests/ref/fate/libx264-hdr10 | 15 +++++++
> > >  4 files changed, 101 insertions(+)
> > >  create mode 100644 tests/ref/fate/libx264-hdr10
> > >
> > > diff --git a/configure b/configure
> > > index bb5e630bad..f48cf46ffe 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -2527,6 +2527,7 @@ CONFIG_EXTRA="
> > >      jpegtables
> > >      lgplv3
> > >      libx262
> > > +    libx264_hdr10
> > >      llauddsp
> > >      llviddsp
> > >      llvidencdsp
> > > @@ -6927,6 +6928,7 @@ enabled libx264           && require_pkg_config libx264 x264 "stdint.h x264.h" x
> > >                               require_cpp_condition libx264 x264.h "X264_BUILD >= 122" && {
> > >                               [ "$toolchain" != "msvc" ] ||
> > >                               require_cpp_condition libx264 x264.h "X264_BUILD >= 158"; } &&
> > > +                             check_cpp_condition libx264_hdr10 x264.h "X264_BUILD >= 163" &&
> >
> > Why don't you just check X264_BUILD in libx264.c instead of adding this
> > to configure?
> >
>
> Most likely due to months ago it feeling like a cleaner option for
> whatever reason (less magical numbers). Although in theory you can get
> a similar named define by checking it within the module, yes.
>

Finally remembered what the actual reason was while modifying the
commit to see if I could move to a simple X264_BUILD check.

The FATE test added requires this newer version, and not just any
enabled libx264. So the define from configure is utilized for that.
Not sure you can utilize simple x264.h checks for X264_BUILD >= 163
for that :) .

Jan
diff mbox series

Patch

diff --git a/configure b/configure
index bb5e630bad..f48cf46ffe 100755
--- a/configure
+++ b/configure
@@ -2527,6 +2527,7 @@  CONFIG_EXTRA="
     jpegtables
     lgplv3
     libx262
+    libx264_hdr10
     llauddsp
     llviddsp
     llvidencdsp
@@ -6927,6 +6928,7 @@  enabled libx264           && require_pkg_config libx264 x264 "stdint.h x264.h" x
                              require_cpp_condition libx264 x264.h "X264_BUILD >= 122" && {
                              [ "$toolchain" != "msvc" ] ||
                              require_cpp_condition libx264 x264.h "X264_BUILD >= 158"; } &&
+                             check_cpp_condition libx264_hdr10 x264.h "X264_BUILD >= 163" &&
                              check_cpp_condition libx262 x264.h "X264_MPEG2"
 enabled libx265           && require_pkg_config libx265 x265 x265.h x265_api_get &&
                              require_cpp_condition libx265 x265.h "X265_BUILD >= 89"
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 10d646bd76..effdcfdb37 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"
@@ -871,6 +872,82 @@  static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
         return AVERROR(EINVAL);\
     }
 
+#if CONFIG_LIBX264_HDR10
+static void handle_mdcv(x264_param_t *params,
+                        const AVMasteringDisplayMetadata *mdcv)
+{
+    if (!mdcv->has_primaries && !mdcv->has_luminance)
+        return;
+
+    params->mastering_display.b_mastering_display = 1;
+
+    if (mdcv->has_primaries) {
+        int *const 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
+            },
+        };
+
+        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 });
+    }
+
+    if (mdcv->has_luminance) {
+        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 // CONFIG_LIBX264_HDR10
+
+static void handle_side_data(AVCodecContext *avctx, x264_param_t *params)
+{
+#if CONFIG_LIBX264_HDR10
+    const AVFrameSideData *cll_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
+    const AVFrameSideData *mdcv_sd =
+        av_frame_side_data_get(
+            (const AVFrameSideData **)avctx->frame_side_data,
+            avctx->nb_frame_side_data, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
+
+    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 // CONFIG_LIBX264_HDR10
+}
+
 static av_cold int X264_init(AVCodecContext *avctx)
 {
     X264Context *x4 = avctx->priv_data;
@@ -1171,6 +1248,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..4095a4b51a 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, LIBX264_HDR10 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