[FFmpeg-devel] [PATCH v2 2/3] {configure, avcodec/libx264}: remove separate x264_csp_bgr check

[Jan Ekström]

On Fri, Jul 2, 2021 at 3:10 PM Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 2:25 PM Jan Ekström <jeebjp at gmail.com> wrote:
> >
> > We already require X264_BUILD >= 118, which includes an unconditional
> > definition of X264_CSP_BGR in itself, thus making this check
> > effectively always true.
> > ---
> >  configure            | 3 +--
> >  libavcodec/libx264.c | 7 +------
> >  2 files changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/configure b/configure
> > index ab27220688..b3b8065188 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3316,7 +3316,7 @@ libwebp_anim_encoder_deps="libwebp"
> >  libx262_encoder_deps="libx262"
> >  libx264_encoder_deps="libx264"
> >  libx264_encoder_select="atsc_a53"
> > -libx264rgb_encoder_deps="libx264 x264_csp_bgr"
> > +libx264rgb_encoder_deps="libx264"
> >  libx264rgb_encoder_select="libx264_encoder"
> >  libx265_encoder_deps="libx265"
> >  libxavs_encoder_deps="libxavs"
> > @@ -6529,7 +6529,6 @@ enabled libx264           && { check_pkg_config libx264 x264 "stdint.h x264.h" x
> >                                 { require libx264 "stdint.h x264.h" x264_encoder_encode "-lx264 $pthreads_extralibs $libm_extralibs" &&
> >                                   warn "using libx264 without pkg-config"; } } &&
> >                               require_cpp_condition libx264 x264.h "X264_BUILD >= 118" &&
> > -                             check_cpp_condition x264_csp_bgr x264.h "X264_CSP_BGR" &&
> >                               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 >= 70"
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index 4b905bf9da..fdb9e285a6 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -553,7 +553,6 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
> >      case AV_PIX_FMT_YUVJ444P:
> >      case AV_PIX_FMT_YUV444P9:
> >      case AV_PIX_FMT_YUV444P10: return X264_CSP_I444;
> > -#if CONFIG_LIBX264RGB_ENCODER
> >      case AV_PIX_FMT_BGR0:
> >          return X264_CSP_BGRA;
> >      case AV_PIX_FMT_BGR24:
> > @@ -561,7 +560,6 @@ static int convert_pix_fmt(enum AVPixelFormat pix_fmt)
> >
> >      case AV_PIX_FMT_RGB24:
> >          return X264_CSP_RGB;
> > -#endif
> >      case AV_PIX_FMT_NV12:      return X264_CSP_NV12;
> >      case AV_PIX_FMT_NV16:
> >      case AV_PIX_FMT_NV20:      return X264_CSP_NV16;
> > @@ -1018,14 +1016,13 @@ static const enum AVPixelFormat pix_fmts_all[] = {
> >  #endif
> >      AV_PIX_FMT_NONE
> >  };
> > -#if CONFIG_LIBX264RGB_ENCODER
> > +
>
> As much as I want to remove the x264rgb ifs for the AVClass and
> AVCodec, I guess at this point the separation should still be there to
> be 100% correct...
>
> 1. `--disable-encoder=libx264rgb` does actually make it unavailable
> through the API (even if the ff_ symbol most likely is still there
> built/exposed), and keeps the base x264 wrapper around.
> 2. `--disable-encoder=libx264` does disable both (which is kind of
> what the end result of this patch set is after merging, but
> technically I guess incorrect at the point where they're still
> separated).
>
> So I will slip these changes to the last commit for the AVClass and
> AVCodec definitions (and I guess pix_fmts_8bit_rgb since otherwise you
> will get unused warnings about it).

Well, after testing and then going d'oh as one looks at the dependency
chain, disabling libx264 and keeping only libx264rgb is not even
possible right now as libx264rgb depends on libx264. So as such
changing the ifs causes no change in behavior other than whether the
AVCodec ff_ symbol is exposed or not.

Jan