Bug 186272 - [GTK][WPE] Support JPEG 2000 images
Summary: [GTK][WPE] Support JPEG 2000 images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Critical
Assignee: Nobody
URL:
Keywords: InRadar
: 180995 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-06-04 09:22 PDT by Sergio Villar Senin
Modified: 2019-01-24 02:03 PST (History)
11 users (show)

See Also:


Attachments
Patch (29.30 KB, patch)
2019-01-22 10:07 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (29.91 KB, patch)
2019-01-23 04:00 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (32.30 KB, patch)
2019-01-23 06:23 PST, Carlos Garcia Campos
zan: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.58 MB, application/zip)
2019-01-23 07:38 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2018-06-04 09:22:11 PDT
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.
Comment 1 Michael Catanzaro 2018-06-04 09:41:33 PDT
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...?
Comment 2 Sergio Villar Senin 2018-06-05 08:24:43 PDT
(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.
Comment 3 Carlos Garcia Campos 2018-06-05 09:16:34 PDT
Could it be that dell is serving jpx for us because of the user agent?
Comment 4 Michael Catanzaro 2018-06-05 10:03:17 PDT
(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.
Comment 5 Sergio Villar Senin 2018-06-06 03:15:59 PDT
(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).
Comment 6 Michael Catanzaro 2018-06-06 08:29:40 PDT
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.
Comment 7 Xabier Rodríguez Calvar 2018-06-06 23:01:18 PDT
(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.
Comment 8 Carlos Garcia Campos 2018-06-20 06:35:08 PDT
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.
Comment 9 Sergio Villar Senin 2018-06-20 09:56:22 PDT
(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...
Comment 10 Michael Catanzaro 2018-06-20 09:57:10 PDT
(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.
Comment 11 Carlos Garcia Campos 2018-06-21 01:42:39 PDT
(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.
Comment 12 Carlos Garcia Campos 2018-06-21 02:17:47 PDT
I don't see any broken image in kmart.com
Comment 13 Michael Catanzaro 2018-06-21 08:14:43 PDT
(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.
Comment 14 Michael Catanzaro 2018-12-18 13:15:39 PST
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.
Comment 15 Said Abou-Hallawa 2019-01-07 10:30:59 PST
*** Bug 180995 has been marked as a duplicate of this bug. ***
Comment 16 Andres Gomez Garcia 2019-01-18 05:37:30 PST
Ikea webpages (www.ikea.*) are affected by this. Quite annoying since images there are quite important ...

Also, Deutsche Bahn (www.bahn.de)
Comment 17 Carlos Garcia Campos 2019-01-22 10:07:01 PST
Created attachment 359745 [details]
Patch
Comment 18 Build Bot 2019-01-22 10:09:23 PST
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.
Comment 19 Zan Dobersek 2019-01-23 01:50:28 PST
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?
Comment 20 Carlos Garcia Campos 2019-01-23 03:06:24 PST
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.
Comment 21 Carlos Garcia Campos 2019-01-23 03:24:21 PST
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).
Comment 22 Carlos Garcia Campos 2019-01-23 04:00:25 PST
Created attachment 359870 [details]
Patch
Comment 23 Build Bot 2019-01-23 04:04:38 PST
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.
Comment 24 Carlos Garcia Campos 2019-01-23 05:23:57 PST
We need OpenJPEG 2.2.0, not available in debian stable, so we'll need to build it in jhbuild.
Comment 25 Carlos Garcia Campos 2019-01-23 06:23:35 PST
Created attachment 359873 [details]
Patch
Comment 26 Build Bot 2019-01-23 06:26:45 PST
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.
Comment 27 Build Bot 2019-01-23 07:38:20 PST
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
Comment 28 Build Bot 2019-01-23 07:38:22 PST
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
Comment 29 Michael Catanzaro 2019-01-23 09:58:32 PST
Amazing!
Comment 30 Carlos Garcia Campos 2019-01-24 02:02:40 PST
Committed r240428: <https://trac.webkit.org/changeset/240428>
Comment 31 Radar WebKit Bug Importer 2019-01-24 02:03:30 PST
<rdar://problem/47509724>