Created attachment 374964 [details] Patch

Comment on attachment 374964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374964&action=review > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:303 > +#if defined(TURBO_JPEG_RGB_SWIZZLE) && !defined(LIBJPEG_TURBO_SUPPORTS_CONVERSIONS_FROM_RGB) libjpeg-turbo seems to have LIBJPEG_TURBO_VERSION_NUMBER. Can you use it? LIBJPEG_TURBO_VERSION_NUMBER < 1002001

Can we simply remove the code? Debian 9 (stretch) has libjpeg-turbo 1.5.1. Ubuntu 18.04 (bionic) has libjpeg-turbo 1.5.2. https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy https://packages.debian.org/source/stretch/libjpeg-turbo https://packages.ubuntu.com/source/bionic/libjpeg-turbo

LIBJPEG_TURBO_VERSION_NUMBER was added in 1.5.0, I wanted to do the test with the version which fixed the issue. We could remove the code, but we would have to test the version, either : - with a solution like the proposed patch - in the code : -#if defined(JCS_ALPHA_EXTENSIONS) && ASSUME_LITTLE_ENDIAN +#if defined(LIBJPEG_TURBO_VERSION_NUMBER) && ASSUME_LITTLE_ENDIAN +#if LIBJPEG_TURBO_VERSION_NUMBER >= 1002001 It would reference the 1.2.1 version, but only work with 1.5.0 and later, unless they are patched. Older libjpeg-turbo versions would be handled as libjpeg, so without BGRA output.

Comment on attachment 374964 [details] Patch I think this is fine, but yeah, a simpler check not requiring regex would be even better.

(In reply to Michael Catanzaro from comment #5) > Comment on attachment 374964 [details] > Patch > > I think this is fine, but yeah, a simpler check not requiring regex would be > even better. Without the regex, the only possible test is LIBJPEG_TURBO_VERSION_NUMBER (and can be done in the code), but it would mean only 1.5.0 and later would have the fix, not 1.2.1.

Which version of libjpeg-turbo are you using? When will you upgrade your libjpeg-turbo?

(In reply to Fujii Hironori from comment #7) > Which version of libjpeg-turbo are you using? When will you upgrade your > libjpeg-turbo? In our builds, we use 1.4.2 or never depending on the target, but we can upgrade. I just wanted to avoid requiring a more recent version that what is really needed. If we keep the code, but do the test with LIBJPEG_TURBO_VERSION_NUMBER, users of libjpeg-turbo 1.2.x/1.4.x would keep the current behavior, not taking advantage of the fast path with those specific JPEG files. I think that would be OK. Removing the code would mean slowing down those potential users for all other JPEGs, without any warning.

I think we should restrict libjpeg-turbo version by CMake and remove the workaround. find_package(JPEG 1.2.1 REQUIRED)

Yeah that's good, 1.2.1 is pretty old.

(In reply to Fujii Hironori from comment #9) > I think we should restrict libjpeg-turbo version by CMake and remove the > workaround. > > find_package(JPEG 1.2.1 REQUIRED) FindJPEG.cmake defines JPEG_VERSION from JPEG_LIB_VERSION (with a regexp) : it's the libjpeg ABI version chosen when configuring libjpeg (for example 62). libjpeg-turbo >= 1.5.0 has a libjpeg.pc, which has the libjpeg-turbo version. (In reply to Michael Catanzaro from comment #10) > Yeah that's good, 1.2.1 is pretty old. But does everyone use libjpeg-turbo ? The code is still compatible with the original libjpeg (IJG), obviously using only RGB output.

(In reply to Fujii Hironori from comment #9) > I think we should restrict libjpeg-turbo version by CMake and remove the > workaround. > > find_package(JPEG 1.2.1 REQUIRED) Problem is that CMake's bundled FindJPEG (which we are using) does not detect libjpeg version, so you can write e.g. find_package(JPEG 55 REQUIRED) and it will succeed.

(In reply to Loïc Yhuel from comment #11) > (In reply to Michael Catanzaro from comment #10) > > Yeah that's good, 1.2.1 is pretty old. > But does everyone use libjpeg-turbo ? Of course no.

Comment on attachment 374964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374964&action=review > Source/WebCore/ChangeLog:10 > + Covered by existing fast/images/rgb-jpeg-with-adobe-marker-only.html. If it's covered by this test, should we see an update of the test expectations with this change?

(In reply to Alex Christensen from comment #14) > Comment on attachment 374964 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374964&action=review > > > Source/WebCore/ChangeLog:10 > > + Covered by existing fast/images/rgb-jpeg-with-adobe-marker-only.html. > > If it's covered by this test, should we see an update of the test > expectations with this change? https://trac.webkit.org/changeset/108870/webkit added the test and the workaround (RGB output, converted to RGBA in WebKit, like with the IJG libjpeg), so the test always passed. My patch removes the workaround when we have libjpeg-turbo >= 1.2.1. The test would only fail if we force RGBA decoding with a libjpeg-turbo < 1.2.1.

Comment on attachment 374964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374964&action=review > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:302 > m_info.out_color_space = rgbOutputColorSpace(); It seems that JPEGImageDecoder::outputScanlines doesn't support the case out_color_space is BGRA and m_scaled is false. I think we should set out_color_space JCS_RGB if m_scaled. m_info.out_color_space = m_scaled ? JCS_RGB : rgbOutputColorSpace();

Comment on attachment 374964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374964&action=review >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:302 >> m_info.out_color_space = rgbOutputColorSpace(); > > It seems that JPEGImageDecoder::outputScanlines doesn't support the case out_color_space is BGRA and m_scaled is false. > I think we should set out_color_space JCS_RGB if m_scaled. > > m_info.out_color_space = m_scaled ? JCS_RGB : rgbOutputColorSpace(); It seems a dead code. Filed Bug 200498.

(In reply to Fujii Hironori from comment #16) > Comment on attachment 374964 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374964&action=review > > > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:302 > > m_info.out_color_space = rgbOutputColorSpace(); > > It seems that JPEGImageDecoder::outputScanlines doesn't support the case > out_color_space is BGRA and m_scaled is false. > I think we should set out_color_space JCS_RGB if m_scaled. > > m_info.out_color_space = m_scaled ? JCS_RGB : rgbOutputColorSpace(); Yes, r225091 left dead code which wasn't under the flag, but depended on the flag to work properly. Btw, the generic downsampling code doesn't really makes sense for JPEG, since the library support fractional M/8 scaling (which is faster than full size decoding, let alone the additional manual scaling loop here).

Comment on attachment 374964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374964&action=review > Source/WebCore/platform/ImageDecoders.cmake:71 > + set_property(SOURCE platform/image-decoders/jpeg/JPEGImageDecoder.cpp APPEND PROPERTY COMPILE_DEFINITIONS "LIBJPEG_TURBO_SUPPORTS_CONVERSIONS_FROM_RGB") Let's remove the workaround. Make it a fatal error at CMake phase by using message(FATAL_ERROR "") if you want.

Created attachment 379020 [details] Patch

Sorry, I forgot this patch. So, without the test, we just assume no one will really use an old libjpeg-turbo.

libjpeg-turbo 1.2.1 has been released 2012-06-30. https://sourceforge.net/p/libjpeg-turbo/mailman/message/29479885/ Who does want to use very old libjpeg-turbo with very new WebKit together?

Comment on attachment 379020 [details] Patch Clearing flags on attachment: 379020 Committed r250029: <https://trac.webkit.org/changeset/250029>

All reviewed patches have been landed. Closing bug.

<rdar://problem/55472267>