Bug 186272 – [GTK][WPE] Support JPEG 2000 images

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Sergio Villar Senin]

I tried to visit dell.com in MB. It redirects me to http://www1.euro.dell.com/content/default.aspx?c=es&l=es&s=&s=gen&~ck=cr

Images are not loaded, inspector reports an error retrieving resources.

[Michael Catanzaro]

I don't see any errors in the web inspector. If I inspect the individual resources, it just says "resource has no content." What error do you see...?

[Sergio Villar Senin]

(In reply to Michael Catanzaro from comment #1)
> I don't see any errors in the web inspector. If I inspect the individual
> resources, it just says "resource has no content." What error do you see...?

Go to "Network" tab and click on any of the failed jpgs. It says "An error ocurred trying to load the resource". Anyway that's not the point, the images are shown perfectly in FF or in Chrome in the same laptop.

[Carlos Garcia Campos]

Could it be that dell is serving jpx for us because of the user agent?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #3)
> Could it be that dell is serving jpx for us because of the user agent?

Hm, well Epiphany can download the images if I download them directly, but eog cannot display them because they are not JPEG images, even though they use the .jpg extension.

Maybe? Probably?

kmart.com is broken with the same problem, bug #180995. I closed that WONTFIX because I could not find a user agent quirk that would resolve the issue.

[Sergio Villar Senin]

(In reply to Michael Catanzaro from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> > Could it be that dell is serving jpx for us because of the user agent?
> 
> Hm, well Epiphany can download the images if I download them directly, but
> eog cannot display them because they are not JPEG images, even though they
> use the .jpg extension.
> 
> Maybe? Probably?
> 
> kmart.com is broken with the same problem, bug #180995. I closed that
> WONTFIX because I could not find a user agent quirk that would resolve the
> issue.

Not finding a fix does not imply closing it as WONTFIX, we need to find a solution for both issues (if they are not the same).

[Michael Catanzaro]

So to be clear: these are server bugs, we are never going to support this image format. Landing https://bugs.webkit.org/show_bug.cgi?id=178758 is probably the only way to move forward here, though it might break these websites in Safari.

Alternatively, if you can find a user agent quirk that works, we could use that. But it's certainly not a WebKit bug if we fail to develop a quirk to trick a remote server.

[Xabier Rodríguez Calvar]

(In reply to Michael Catanzaro from comment #6)
> So to be clear: these are server bugs, we are never going to support this
> image format. Landing https://bugs.webkit.org/show_bug.cgi?id=178758 is
> probably the only way to move forward here, though it might break these
> websites in Safari.
> 
> Alternatively, if you can find a user agent quirk that works, we could use
> that. But it's certainly not a WebKit bug if we fail to develop a quirk to
> trick a remote server.

I disagree. At least for some places it is not as easy as it is a server bug and off we go.

[Carlos Garcia Campos]

I confirm this is because the user agent, once again :-/ dell is sending us jpeg2000 images, using the chrome user agent we receive webp instead.

[Sergio Villar Senin]

(In reply to Carlos Garcia Campos from comment #8)
> I confirm this is because the user agent, once again :-/ dell is sending us
> jpeg2000 images, using the chrome user agent we receive webp instead.

OK it's then a matter of quirks isn't it?

This is the neverending story...

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #8)
> I confirm this is because the user agent, once again :-/ dell is sending us
> jpeg2000 images, using the chrome user agent we receive webp instead.

I'd use it for baseDomain == dell.com then.

Also check kmart.com just in case you have better luck than me, but I think we'll need to write that one off.

(In reply to Xabier Rodríguez Calvar from comment #7) 
> I disagree. At least for some places it is not as easy as it is a server bug
> and off we go.

How do you propose we fix it if a Chromium user agent quirk does not work (kmart.com)? Only solution I see is to work on removing JPEG 2000 support from Safari and wait until websites notice that they are broken.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #10)
> (In reply to Carlos Garcia Campos from comment #8)
> > I confirm this is because the user agent, once again :-/ dell is sending us
> > jpeg2000 images, using the chrome user agent we receive webp instead.
> 
> I'd use it for baseDomain == dell.com then.

But it also happens to me in dell.es, and I assume in dell.* too.

> Also check kmart.com just in case you have better luck than me, but I think
> we'll need to write that one off.

Did you remove the disk cache after changing the user agent and trying again? 

> (In reply to Xabier Rodríguez Calvar from comment #7) 
> > I disagree. At least for some places it is not as easy as it is a server bug
> > and off we go.
> 
> How do you propose we fix it if a Chromium user agent quirk does not work
> (kmart.com)? Only solution I see is to work on removing JPEG 2000 support
> from Safari and wait until websites notice that they are broken.

I'll check why changing the user agent doesn't work in makrt first.

[Carlos Garcia Campos]

I don't see any broken image in kmart.com

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #11)
> Did you remove the disk cache after changing the user agent and trying
> again? 

No, I probably should have.

(In reply to Carlos Garcia Campos from comment #12)
> I don't see any broken image in kmart.com

Looks like Kmart fixed it quite recently. That's nice.

[Michael Catanzaro]

See https://bugs.webkit.org/show_bug.cgi?id=178758#c10 for our latest discovery regarding dell.com. We're simply going to need to add support for JPEG 2000 images. This will affect all ports except Apple ports, because Apple ports do not use WebKit's image decoders.

[Said Abou-Hallawa]

*** Bug 180995 has been marked as a duplicate of this bug. ***

[Andres Gomez Garcia]

Ikea webpages (www.ikea.*) are affected by this. Quite annoying since images there are quite important ...

Also, Deutsche Bahn (www.bahn.de)

[Carlos Garcia Campos]

Created attachment 359745 [details]
Patch

[EWS Watchlist]

Attachment 359745 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:39:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:41:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 359745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359745&action=review

> Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:57
> +    *r = std::max(0, std::min(upb, y + static_cast<int>(1.402 * static_cast<float>(cr))));
> +    *g = std::max(0, std::min(upb, y - static_cast<int>(0.344 * static_cast<float>(cb) + 0.714 * static_cast<float>(cr))));
> +    *b = std::max(0, std::min(upb, y + static_cast<int>(1.772 * static_cast<float>(cb))));

Why not use matrix coefficients wholly, like it's done in the color_esycc_to_rgb() function in openjpeg?

Instead of std::min/std::max, you could consider using clampTo<>(), but keeping all of calculation in float.
So for r value, you'd have:

    r = static_cast<int>(clampTo<float>(y - 0.0000368 + 1.40199 * cr, 0, upb));

> Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:68
> +    size_t maxw = static_cast<size_t>(img->comps[0].w);
> +    size_t maxh = static_cast<size_t>(img->comps[0].h);
> +    size_t max = maxw * maxh;

Any way to impose sane limits on these sizes, and/or to protect from overflow?

[Carlos Garcia Campos]

Comment on attachment 359745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359745&action=review

>> Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:57
>> +    *b = std::max(0, std::min(upb, y + static_cast<int>(1.772 * static_cast<float>(cb))));
> 
> Why not use matrix coefficients wholly, like it's done in the color_esycc_to_rgb() function in openjpeg?
> 
> Instead of std::min/std::max, you could consider using clampTo<>(), but keeping all of calculation in float.
> So for r value, you'd have:
> 
>     r = static_cast<int>(clampTo<float>(y - 0.0000368 + 1.40199 * cr, 0, upb));

I have no idea, this code is copied from OpenJPEG. I just used min/max instead of the ifs in the original code. I'll use clamp as you suggest.

>> Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:68
>> +    size_t max = maxw * maxh;
> 
> Any way to impose sane limits on these sizes, and/or to protect from overflow?

Ditto. I guess we could use CheckedArithmetic.

[Carlos Garcia Campos]

Comment on attachment 359745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359745&action=review

>>> Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:57
>>> +    *b = std::max(0, std::min(upb, y + static_cast<int>(1.772 * static_cast<float>(cb))));
>> 
>> Why not use matrix coefficients wholly, like it's done in the color_esycc_to_rgb() function in openjpeg?
>> 
>> Instead of std::min/std::max, you could consider using clampTo<>(), but keeping all of calculation in float.
>> So for r value, you'd have:
>> 
>>     r = static_cast<int>(clampTo<float>(y - 0.0000368 + 1.40199 * cr, 0, upb));
> 
> I have no idea, this code is copied from OpenJPEG. I just used min/max instead of the ifs in the original code. I'll use clamp as you suggest.

color_esycc_to_rgb() is used when color space is OPJ_CLRSPC_EYCC. We don't support that one (yet).

[Carlos Garcia Campos]

Created attachment 359870 [details]
Patch

[EWS Watchlist]

Attachment 359870 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:39:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:41:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

We need OpenJPEG 2.2.0, not available in debian stable, so we'll need to build it in jhbuild.

[Carlos Garcia Campos]

Created attachment 359873 [details]
Patch

[EWS Watchlist]

Attachment 359873 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:39:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.cpp:41:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/image-decoders/jpeg2000/JPEG2000ImageDecoder.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 359873 [details]
Patch

Attachment 359873 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10857660

New failing tests:
imported/w3c/web-platform-tests/css/css-grid/abspos/orthogonal-positioned-grid-descendants-015.html
imported/w3c/web-platform-tests/css/css-grid/abspos/orthogonal-positioned-grid-descendants-013.html

[EWS Watchlist]

Created attachment 359881 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Michael Catanzaro]

Amazing!

[Carlos Garcia Campos]

Committed r240428: <https://trac.webkit.org/changeset/240428>

[Radar WebKit Bug Importer]

<rdar://problem/47509724>