[FFmpeg-devel] [PATCH v3 0/2] libx264 configure check clean-up

[Jan Ekström]

On Sat, Jul 10, 2021 at 8:55 PM James Almer <jamrial at gmail.com> wrote:
>
> On 7/10/2021 1:26 PM, Jan Ekström wrote:
> > On Wed, Jul 7, 2021, 22:01 Jan Ekström <jeebjp at gmail.com> wrote:
> >
> >> Changes compared to v2:
> >> - Kept the CONFIG_LIBX264RGB_ENCODER define check for ff_libx264rgb_encoder
> >>    and the AVClass for libx264rgb.
> >> - Removed the libx264rgb removal from this patch set since while I hoped I
> >>    would be getting the initial two fixups reviewed even if people would
> >> oppose
> >>    the libx264rgb removal, so at least those could get in - that didn't seem
> >>    to be happening. This way I hope people would be more likely to focus on
> >>    that bit at first.
> >>
> >> The patch set contains two improvements to the libx264rgb configure checks,
> >> as I found out that for all the time I had been building FFmpeg with a
> >> custom
> >> prefix and utilizing pkg-config - it never got enabled due to the configure
> >> check relying on the header being in the default include paths or in
> >> extra-cflags.
> >>
> >> - The first change fixes libx264rgb enablement without having x264.h
> >>    in the system default include path, such as with custom prefixes.
> >>
> >> - The second change removes the separate X264_CSP_BGR check as x264.h
> >>    has this define unconditionally defined with the required X264_BUILD
> >>    118 or newer (it was added a few X264_BUILD versions before).
> >>
> >>    This change was checked by bumping the require_cpp_condition
> >>    check to X264_BUILD >= 255 and checking with both pkg-config
> >>    as well as by not having PKG_CONFIG_PATH defined as well as
> >>    making the non-pkg-config check pass with
> >>    `--extra-cflags="-I/prefix/include" --extra-ldflags="-L/prefix/lib -ldl"`
> >>    So the X264_BUILD check should properly fail the enablement in
> >>    case X264_BUILD is older than the one requested in the relevant
> >>    require_cpp_condition.
> >>
> >> Best regards,
> >> Jan
> >>
> >> Jan Ekström (2):
> >>    configure: move x264_csp_bgr check under general libx264 checks
> >>    {configure,avcodec/libx264}: remove separate x264_csp_bgr check
> >>
> >>   configure            | 3 +--
> >>   libavcodec/libx264.c | 2 --
> >>   2 files changed, 1 insertion(+), 4 deletions(-)
> >>
> >> --
> >> 2.31.1
> >>
> >
> > Ping on this patch set.
> >
> > These should be relatively straightforward changes that enable x264rgb when
> > it is searched through pkg-config, and testable by installing x264 into a
> > specific prefix and not having its headers in the default search path (but
> > setting PKG_CONFIG_PATH accordingly).
> >
> > Jan
>
> Should be ok.
>

Thanks, applied as 25d28f297b755d3cb6a3e036a1e251148d0e4d5c and
f32f56468c6caa03f4ebbf6cf58b2bb7bc775216 .

> And removing libx264rgb altogether should be done at some point if you
> can get rgb output with the normal wrapper.

We purely limit it by the accessible color spaces defined for the
AVCodec, so by removing that limitation you most definitely can get
RGB output from the normal wrapper. I had that patch as part of v1/2
of this set. I will now post a separate patch for it.

If there are some steps that have to be done for it, I'm fine with
that (like enabling libx264 to handle RGB in a first commit, then
marking libx264rgb for deprecation etc). But the only comments I got
last time were:
- "-c:v libx264rgb" no longer works. (well yes, the idea of the patch
was to remove it)
- the current libx264 default is more supported by non-libavcodec
decoders (which generally with RGB input at the moment is and probably
always was 4:4:4 YCbCr - which I am not sure if it is in any way or
form a major difference against 4:4:4 + RGB color space metadata?).
The original ticket against the backdrop of which libx264rgb was
originally split was 100% talking about yuv420p as the default for HW
support (https://trac.ffmpeg.org/ticket/658), which is not what the
split is doing.

Jan