diff mbox series

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

Message ID 20210702112513.36348-3-jeebjp@gmail.com
State Superseded
Headers show
Series libx264 configure check clean-up, removal of libx264rgb | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Jan Ekström July 2, 2021, 11:25 a.m. UTC 
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(-)

Comments

Jan Ekström July 2, 2021, 12:10 p.m. UTC | #1 
On Fri, Jul 2, 2021 at 2:25 PM Jan Ekström <jeebjp@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).

Jan
Jan Ekström July 2, 2021, 12:27 p.m. UTC | #2 
On Fri, Jul 2, 2021 at 3:10 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 2:25 PM Jan Ekström <jeebjp@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
diff mbox series

Patch

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
+
 static const enum AVPixelFormat pix_fmts_8bit_rgb[] = {
     AV_PIX_FMT_BGR0,
     AV_PIX_FMT_BGR24,
     AV_PIX_FMT_RGB24,
     AV_PIX_FMT_NONE
 };
-#endif
 
 #if X264_BUILD < 153
 static av_cold void X264_init_static(AVCodec *codec)
@@ -1186,9 +1183,7 @@  AVCodec ff_libx264_encoder = {
                       ,
     .wrapper_name     = "libx264",
 };
-#endif
 
-#if CONFIG_LIBX264RGB_ENCODER
 static const AVClass rgbclass = {
     .class_name = "libx264rgb",
     .item_name  = av_default_item_name,