Closed
Bug 1089880
Opened 10 years ago
Closed 10 years ago
Add a HAS_TRANSPARENCY notification to imagelib and use it for image documents
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: seth, Assigned: seth)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 7 obsolete files)
6.43 KB,
patch
|
Details | Diff | Splinter Review | |
16.77 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
10.07 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
In bug 754133 we set the background of <img> elements in image documents to white, so that transparent images (which are often designed for light backgrounds) would be readable even though the background of image documents is dark. In bug 756419 it was noticed that this would cause a "white flash" when discarding images in background image document tabs were brought back to the foreground. To avoid this, code was added that only sets the white background when the image is decoded. Whether the image is decoded is inferred from the DECODE_COMPLETE and DISCARD image notifications. There are several problems with this design: (1) It does not correctly handle the case where the image takes a long time to download and decode. When you display a large transparent image on a slow network, no white background will appear until the image completely downloads, at which point it will suddenly flash into place. The same will happen if the image gets discarded and redecoded. So we still have a "white flash", it's just that now we see it on transparent images rather than opaque images! (2) It does not correctly handle the case where the image is stored in a volatile buffer and has been "discarded" by the OS. In that situation, there'll be no DISCARD notification, so the user will still see the white flash. (3) It relies on interpreting decode notifications which are going to become increasingly difficult to reason about in this way as we start supporting multiple decoders for a single image and downscale-during-decode. I'm currently planning to start delivering DECODE_COMPLETE only for the first decode to make this stuff easier to reason about, and am considering removing it altogether in the future. That means that, even if the current solution worked well, it won't continue to work in the long term. All of these problems can be solved by giving the responsibility for drawing the contrasting background to imagelib, which is what I plan to do in this bug.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
See also bug 793366, which added slight noise to the <img> tag background. To make the change proposed in this bug, we'd have to revert that. I think it's worth it to do so, especially since we may be able to get bigger gains (such as choosing a dark or light background for the transparent image based on its predominant luminosity) with the approach proposed in this bug, in the long term.
Assignee | ||
Comment 2•10 years ago
|
||
So dolske and I talked about this on IRC, and we ended up concluded that: - It would be nice to end up with a solution that has a clear path to turning into a CSS pseudoselector. - It's probably good enough to draw the background only if the image has transparency, and keep it there permanently once we know that. So, the new plan is: (1) In ImageLib, generate a new HAS_TRANSPARENCY notification when we detect that an image is transparent. (2) In ImageDocument.cpp, listen for HAS_TRANSPARENCY and set a "transparent" class on the image element if it fires. Get rid of the old code that deals with the "decoded" class. (3) Update the CSS for top-level image documents to use the new class.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 3•10 years ago
|
||
This patch implements HAS_TRANSPARENCY.
Attachment #8521966 -
Flags: review?(tnikkel)
Assignee | ||
Comment 4•10 years ago
|
||
This patch uses HAS_TRANSPARENCY to activate the special background behind transparent images, instead of DECODE_COMPLETE and DISCARD.
Attachment #8521968 -
Flags: review?(dolske)
Assignee | ||
Comment 5•10 years ago
|
||
This tweaked version of part 1 fixes a bug in handling ICO AND masks and adds HAS_TRANSPARENCY support for SVG-as-image. (We always report SVGs as having transparency; this aligns with the current practice of always returning false from VectorImage::FrameIsOpaque.)
Attachment #8522076 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8521966 -
Attachment is obsolete: true
Attachment #8521966 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•10 years ago
|
||
This new part 2 adds tests for HAS_TRANSPARENCY.
Attachment #8522079 -
Flags: review?(tnikkel)
Assignee | ||
Comment 7•10 years ago
|
||
The old part 2 is now part 3.
Attachment #8522080 -
Flags: review?(dolske)
Assignee | ||
Updated•10 years ago
|
Attachment #8521968 -
Attachment is obsolete: true
Attachment #8521968 -
Flags: review?(dolske)
Assignee | ||
Comment 8•10 years ago
|
||
Alright, there was orange on the previous patch in my patch queue but it should be green now, so here's a try job: https://tbpl.mozilla.org/?tree=Try&rev=a59f71280d71
Updated•10 years ago
|
Attachment #8522080 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 9•10 years ago
|
||
So images which need padding on the first frame need to generate HAS_TRANSPARENCY. This is consistent with RasterImage::FrameIsOpaque.
Attachment #8523346 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8522076 -
Attachment is obsolete: true
Attachment #8522076 -
Flags: review?(tnikkel)
Assignee | ||
Comment 10•10 years ago
|
||
Updated the tests to test for that situation and avoid race conditions when the test ends.
Attachment #8523347 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8522079 -
Attachment is obsolete: true
Attachment #8522079 -
Flags: review?(tnikkel)
Assignee | ||
Comment 11•10 years ago
|
||
There was a test that called |getElementByClassName('decoded')|. I switched it to use |getElementByTagName('img')|.
Assignee | ||
Updated•10 years ago
|
Attachment #8522080 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
New try job here: https://tbpl.mozilla.org/?tree=Try&rev=64777e1a711e
Assignee | ||
Comment 13•10 years ago
|
||
Further improve the efficiency of the ICO decoder transparency check.
Attachment #8523367 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8523346 -
Attachment is obsolete: true
Attachment #8523346 -
Flags: review?(tnikkel)
Assignee | ||
Comment 14•10 years ago
|
||
OK, so apparently the problem is that it's illegal to log any results after SimpleTest.finish - I misunderstood the source of the failure before. I've tweak the test to hopefully avoid this problem, though it doesn't happen locally so a try job will be necessary to verify.
Attachment #8523368 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8523347 -
Attachment is obsolete: true
Attachment #8523347 -
Flags: review?(tnikkel)
Assignee | ||
Comment 15•10 years ago
|
||
Here's a new, linux-only try job (everything but |oth| looks fine, so I don't think we need to run this against all platforms again). Hopefully the |oth| failures were resolved by the latest version of part 2: https://tbpl.mozilla.org/?tree=Try&rev=2a9ea028cab7
Updated•10 years ago
|
Attachment #8523367 -
Flags: review?(tnikkel) → review+
Updated•10 years ago
|
Attachment #8523368 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Thanks for the review! Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/72822bad1819 https://hg.mozilla.org/integration/mozilla-inbound/rev/2af9fb958b4e https://hg.mozilla.org/integration/mozilla-inbound/rev/f4eea7e2f94b
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/72822bad1819 https://hg.mozilla.org/mozilla-central/rev/2af9fb958b4e https://hg.mozilla.org/mozilla-central/rev/f4eea7e2f94b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•