JPEGImageDecoder: use libjpeg-turbo RGBA output path even for Adobe transform=0 JPEGs

• Component: New Bugs
• Status: RESOLVED FIXED

[Loïc Yhuel]

JPEGImageDecoder: use libjpeg-turbo RGBA output path even for Adobe transform=0 JPEGs

[Loïc Yhuel]

Created attachment 374964 [details]
Patch

[Fujii Hironori]

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

[Fujii Hironori]

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

[Loïc Yhuel]

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.

[Michael Catanzaro]

Comment on attachment 374964 [details]
Patch

I think this is fine, but yeah, a simpler check not requiring regex would be even better.

[Loïc Yhuel]

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

[Fujii Hironori]

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

[Loïc Yhuel]

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

[Fujii Hironori]

I think we should restrict libjpeg-turbo version by CMake and remove the workaround.

find_package(JPEG 1.2.1 REQUIRED)

[Michael Catanzaro]

Yeah that's good, 1.2.1 is pretty old.

[Loïc Yhuel]

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

[Konstantin Tokarev]

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

[Konstantin Tokarev]

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

[Alex Christensen]

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?

[Loïc Yhuel]

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

[Fujii Hironori]

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();

[Fujii Hironori]

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.

[Loïc Yhuel]

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

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 379020 [details]
Patch

[Loïc Yhuel]

Sorry, I forgot this patch.

So, without the test, we just assume no one will really use an old libjpeg-turbo.

[Fujii Hironori]

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?

[Fujii Hironori]

Comment on attachment 379020 [details]
Patch

Clearing flags on attachment: 379020

Committed r250029: <https://trac.webkit.org/changeset/250029>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55472267>