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

[Jan Ekström]

On Wed, Feb 28, 2024 at 1:39 AM Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Wed, Feb 28, 2024 at 12:24 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.
> > > ---
> > >  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