Closed Bug 1631615 Opened 4 months ago Closed 3 months ago

windy.com - colors missing from map layers

Categories

(Core :: GFX: Color Management, defect, P2)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
mozilla78
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- unaffected
firefox76 --- unaffected
firefox77 + verified
firefox78 --- verified

People

(Reporter: miketaylr, Assigned: aosmond)

References

(Regressed 1 open bug, Regression, )

Details

(Keywords: regression)

Attachments

(5 files)

Attached image 76 left vs 77 right

(Sorry for the poor title, I don't know how to describe this)

Originally reported @ https://github.com/webcompat/web-bugs/issues/51892

STR:

  1. load windy.com

Expected: You see a colorful map image with greens, yellows, reds, etc
Actual: None of that, just a blue and gray rain layer.

This gets SUPER obvious if you click on the temperature control on the top right.

In the console:

WebGL warning: texSubImage: Texture has not been initialized prior to a partial upload, forcing the browser to clear it. This may be slow. gl-particles.js:2:10223
WebGL warning: texSubImage: Tex image TEXTURE_2D level 0 is incurring lazy initialization.

11:04.45 INFO: Narrowed integration regression window from [ad3f58c9, 5f2454ac] (3 builds) to [ad3f58c9, 6392325e] (2 builds) (~1 steps left)
11:04.45 INFO: No more integration revisions, bisection finished.
11:04.45 INFO: Last good revision: ad3f58c996e59bc5ca0a0e6aac61c8f6a7e51096
11:04.45 INFO: First bad revision: 6392325ec39e5cb10edd4aaaa4afb7a9f061b0b2
11:04.45 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ad3f58c996e59bc5ca0a0e6aac61c8f6a7e51096&tochange=6392325ec39e5cb10edd4aaaa4afb7a9f061b0b2

[Tracking Requested - why for this release]: It's unclear how this change affects other sites, it breaks this one pretty much.

Flags: needinfo?(aosmond)
OS: Unspecified → macOS
Hardware: Unspecified → Desktop

Can't reproduce on linux or windows, so might be mac specific.

@Mike: would you also be able to add your about:support information

Severity: -- → critical
Priority: -- → P2

Also yeah, I can't reproduce on Windows either (on a VM)

I confirmed that setting gfx.color_management.mode=2 fixes the problem

I can reproduce on Linux if I force the browser to use the same color profile as the user.

Assignee: nobody → aosmond
Status: NEW → ASSIGNED
OS: macOS → All

Adding nsLayoutUtils::SFE_NO_COLORSPACE_CONVERSION to CanvasRenderingContext2D::DrawImage's nsLayoutUtils::SurfaceFromElement call resolves the problem:

https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/dom/canvas/CanvasRenderingContext2D.cpp#4493

Before my pref change, it would have ignored any colorspace conversion because the JPEG has no ICC profile. Not sure what the actual fix should be yet.

As far as I can tell Chrome doesn't ask for any special behaviour for canvas. As such, it will enter this branch to try to load the ICC profile:

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/image-decoders/jpeg/jpeg_image_decoder.cc;l=574;drc=d685ea3c9ffcb18c781bc3a0bdbb92eb88842b1b;bpv=0;bpt=1?originalUrl=https:%2F%2Fcs.chromium.org%2F

It doesn't have one so it calls Decoder()->ColorTransform():

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/image-decoders/image_decoder.cc;drc=d685ea3c9ffcb18c781bc3a0bdbb92eb88842b1b;bpv=0;bpt=1;l=716?originalUrl=https:%2F%2Fcs.chromium.org%2F

source_to_target_color_transform_needs_update_ is false because we never added the ICC profile, so it doesn't actually fallback to assuming sRGB like we do. This explains why we see different behaviour (assuming I followed the lines correctly ;)).

Okay, I'll update the decoders to not assume sRGB if there is no reasonable colorspace indication to match Chrome's behaviour.

Flags: needinfo?(aosmond)

Andrew, will this 77 regression get fixed before the merge next Monday or should we expect an uplift in beta maybe? Thanks

Flags: needinfo?(aosmond)

I wrote the patch, but needed to fix some gtest related failures. Should be in before the merge but if not, it will be very upliftable/small change.

It appears some websites assume we will not color manage untagged images
and encode metadata in the image's surface data. Chrome matches this
behaviour. We should probably do the same for webcompat.

Pushed by aosmond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf2757bbcf71
Don't color manage PNG/JPG/WebP if not tagged. r=tnikkel
Flags: needinfo?(aosmond)
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

Comment on attachment 9145590 [details]
Bug 1631615 - Don't color manage PNG/JPG/WebP if not tagged.

Beta/Release Uplift Approval Request

  • User impact if declined: We have too aggressively color manage images which content authors did not intend to be color managed; if storing data like windy.com, it is hard to predict what would happen; it is also a webcompat issue with Chrome
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch is small well contained within imagelib and the individual decoders. We run a lot of tests through this path and the code coverage is high. We shipped for a very long time not color managing most untagged images.
  • String changes made/needed:
Attachment #9145590 - Flags: approval-mozilla-beta?

Comment on attachment 9145590 [details]
Bug 1631615 - Don't color manage PNG/JPG/WebP if not tagged.

Looks low risk for early betas and has tests, approved for 77 beta 2, thanks.

Attachment #9145590 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]
Attached image Old behavior

I reproduced the initial issue on Ubuntu 18.04 64bit using an old Nightly from 2020-04-20 where the color was completely off (full red map), but on Windows 10 64bit and MacOS 10.15.5 it reproduced a bit differently, the map was divided in different squares with different color tones (attached image).
On all platforms I can confirm that setting gfx.color_management.mode on 2 fixes the problem on that old Nightly build and also on latest Nightly 78.0a1 from today and Firefox 77.0b3.

Attached image New issue

What I've noticed now (Windows 10 and Ubuntu 18.04 only) that there are a few lines flashing when zooming in/out, same lines I saw 1 comment above where the map was devided in different squares, should I log a new issue on this or should this issue be reopen here?
I did not notice the same behavior on Mac.

Flags: needinfo?(aosmond)

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #21)

Created attachment 9146758 [details]
New issue

What I've noticed now (Windows 10 and Ubuntu 18.04 only) that there are a few lines flashing when zooming in/out, same lines I saw 1 comment above where the map was devided in different squares, should I log a new issue on this or should this issue be reopen here?
I did not notice the same behavior on Mac.

Sounds like a different bug. Does it happen when color management is set to 2? If so, I would be confident it is unrelated. Please attach your about:support as well, since it will contain the color profile which may be useful for reproducing.

Flags: needinfo?(aosmond) → needinfo?(bogdan.maris)

(In reply to Andrew Osmond [:aosmond] from comment #22)

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #21)

Created attachment 9146758 [details]
New issue

What I've noticed now (Windows 10 and Ubuntu 18.04 only) that there are a few lines flashing when zooming in/out, same lines I saw 1 comment above where the map was devided in different squares, should I log a new issue on this or should this issue be reopen here?
I did not notice the same behavior on Mac.

Sounds like a different bug. Does it happen when color management is set to 2? If so, I would be confident it is unrelated. Please attach your about:support as well, since it will contain the color profile which may be useful for reproducing.

Yes, this does still happen with gfx.color_management.mode set to 2. I logged bug 1636856 on this issue and posted there the about:support content. I'll go ahead and close this one as verified fixed.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Flags: needinfo?(bogdan.maris)
Regressions: 1639584
Blocks: 1645042
You need to log in before you can comment on or make changes to this bug.