Closed Bug 1089880 Opened 6 years ago Closed 6 years ago

Add a HAS_TRANSPARENCY notification to imagelib and use it for image documents

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

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.
Blocks: 910441
Depends on: 754133
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.
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.
No longer blocks: 910441
Depends on: 1084136
Summary: Add explicit support for drawing transparent images against a contrasting background to imgIContainer::Draw → Add a HAS_TRANSPARENCY notification to imagelib and use it for image documents
Depends on: 1089046
No longer depends on: 1084136
No longer depends on: 1089046
Blocks: 910441
This patch implements HAS_TRANSPARENCY.
Attachment #8521966 - Flags: review?(tnikkel)
This patch uses HAS_TRANSPARENCY to activate the special background behind transparent images, instead of DECODE_COMPLETE and DISCARD.
Attachment #8521968 - Flags: review?(dolske)
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)
Attachment #8521966 - Attachment is obsolete: true
Attachment #8521966 - Flags: review?(tnikkel)
This new part 2 adds tests for HAS_TRANSPARENCY.
Attachment #8522079 - Flags: review?(tnikkel)
The old part 2 is now part 3.
Attachment #8522080 - Flags: review?(dolske)
Attachment #8521968 - Attachment is obsolete: true
Attachment #8521968 - Flags: review?(dolske)
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
Attachment #8522080 - Flags: review?(dolske) → review+
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)
Attachment #8522076 - Attachment is obsolete: true
Attachment #8522076 - Flags: review?(tnikkel)
Updated the tests to test for that situation and avoid race conditions when the test ends.
Attachment #8523347 - Flags: review?(tnikkel)
Attachment #8522079 - Attachment is obsolete: true
Attachment #8522079 - Flags: review?(tnikkel)
There was a test that called |getElementByClassName('decoded')|. I switched it to use |getElementByTagName('img')|.
Attachment #8522080 - Attachment is obsolete: true
Further improve the efficiency of the ICO decoder transparency check.
Attachment #8523367 - Flags: review?(tnikkel)
Attachment #8523346 - Attachment is obsolete: true
Attachment #8523346 - Flags: review?(tnikkel)
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)
Attachment #8523347 - Attachment is obsolete: true
Attachment #8523347 - Flags: review?(tnikkel)
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
Attachment #8523367 - Flags: review?(tnikkel) → review+
Attachment #8523368 - Flags: review?(tnikkel) → review+
Blocks: 1102617
No longer blocks: 1102617
Blocks: 1103328
Blocks: 1112196
You need to log in before you can comment on or make changes to this bug.