| Message ID | 20210701215447.34169-1-jeebjp@gmail.com |
|---|---|
| Headers | show |
| Series | libx264 configure check clean-up, removal of libx264rgb | expand |
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
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(-)