mbox series

[FFmpeg-devel,0/3] libx264 configure check clean-up, removal of libx264rgb

Message ID 20210701215447.34169-1-jeebjp@gmail.com
Headers show
Series libx264 configure check clean-up, removal of libx264rgb
Related show

Message

Jan Ekström July 1, 2021, 9:54 p.m. UTC
Done in three separate change sets, as this way if the last change
is deemed too controversial, the earlier changes should be applicable
by themselves.

- 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.

- The third and last change is probably the most controversial,
  as in the removal of the separate libx264rgb wrapper.

  This is due to no other encoder wrapper in libavcodec (such as
  libx265 for example) being given such treatment, as well as due to
  the default behavior with ffmpeg.c, RGB input and the libx264 wrapper
  now being not something commonly supported - such as 4:2:0 YCbCr - but
  rather 4:4:4 YCbCr.

  Thus, it is clear that users should rather just define what they
  require (which with RGB input they already seem to be required to do),
  rather than trying to make the separation "do the right thing",
  which it does not currently seem to be leading to.

  Of course, I might be completely incorrect with regards to why the
  split was originally done, but I would expect that if it was for
  more supported color spaces, that would be 4:2:0 and not just
  "normal" 4:4:4 (which in H.264 coding-wise only differs by its
  metadata to BGR).

Best regards,
Jan

Jan Ekström (3):
  configure: move x264_csp_bgr check under general libx264 checks
  {configure,avcodec/libx264}: remove separate x264_csp_bgr check
  avcodec/libx264: remove separate libx264rgb RGB wrapper

 configure              |  3 ---
 doc/encoders.texi      |  7 +++---
 libavcodec/allcodecs.c |  1 -
 libavcodec/libx264.c   | 48 ++++++------------------------------------
 libavcodec/version.h   |  2 +-
 5 files changed, 11 insertions(+), 50 deletions(-)

Comments

Jan Ekström July 1, 2021, 10:06 p.m. UTC | #1
On Fri, Jul 2, 2021 at 12:54 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> Done in three separate change sets, as this way if the last change
> is deemed too controversial, the earlier changes should be applicable
> by themselves.
>
> - 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.
>
> - The third and last change is probably the most controversial,
>   as in the removal of the separate libx264rgb wrapper.
>
>   This is due to no other encoder wrapper in libavcodec (such as
>   libx265 for example) being given such treatment, as well as due to
>   the default behavior with ffmpeg.c, RGB input and the libx264 wrapper
>   now being not something commonly supported - such as 4:2:0 YCbCr - but
>   rather 4:4:4 YCbCr.
>
>   Thus, it is clear that users should rather just define what they
>   require (which with RGB input they already seem to be required to do),
>   rather than trying to make the separation "do the right thing",
>   which it does not currently seem to be leading to.
>
>   Of course, I might be completely incorrect with regards to why the
>   split was originally done, but I would expect that if it was for
>   more supported color spaces, that would be 4:2:0 and not just
>   "normal" 4:4:4 (which in H.264 coding-wise only differs by its
>   metadata to BGR).
>
> Best regards,
> Jan
>
> Jan Ekström (3):
>   configure: move x264_csp_bgr check under general libx264 checks
>   {configure,avcodec/libx264}: remove separate x264_csp_bgr check
>   avcodec/libx264: remove separate libx264rgb RGB wrapper
>
>  configure              |  3 ---
>  doc/encoders.texi      |  7 +++---
>  libavcodec/allcodecs.c |  1 -
>  libavcodec/libx264.c   | 48 ++++++------------------------------------
>  libavcodec/version.h   |  2 +-
>  5 files changed, 11 insertions(+), 50 deletions(-)
>
> --
> 2.31.1
>

Just as an additional note, I just checked the referenced ticket in
the x264rgb addition (https://trac.ffmpeg.org/ticket/658), and as far
as I can tell the requested behavior was to make 4:2:0 get picked by
default.

This might have been the case when the wrapper was added, but looking
at f.ex. https://trac.ffmpeg.org/ticket/9132 the current behavior is
leading to 4:4:4, which is similarly supported by decoders as BGR (as
they differ only by the metadata).

Jan