Closed
Bug 1191114
Opened 6 years ago
Closed 6 years ago
Detect HAS_TRANSPARENCY during the metadata decode
Categories
(Core :: ImageLib, defect)
Core
ImageLib
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(4 files, 4 obsolete files)
3.52 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
2.94 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
13.66 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
14.84 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
We frequently run into intermittent orange issues stemming from HAS_TRANSPARENCY being fired too late. Often changing timing in some unrelated part of the code is enough to expose them. The issue, fundamentally, is that the rendering of image documents changes depending on whether HAS_TRANSPARENCY has been fired (because we add a white noise background behind the image if it has) and it's very tricky to ensure that we never mess this up when taking the reftest snapshot. It'd be drastically less trouble all around if we just required that HAS_TRANSPARENCY be fired during the metadata decode for non-animated images. This would ensure that we'd never race, because HAS_TRANSPARENCY would always have been fired before we even *started* a full decode of the image. In addition, when combined with an upcoming patch to detect animation during the metadata decode, this should let us treat images as opaque earlier (before they have been fully decoded), which should make for cheaper paints while images are loading. Right now we can't do that because we don't know if HAS_TRANSPARENCY will fire or if we'll find out the image is animated until the full decode is complete. The one case where we can't do this accurately is BMPs, because they can have implicit transparency in various awkward ways. (To see what I mean, take a look at the places where we call PostHasTransparency() in nsBMPDecoder and nsICODecoder's BMP codepath.) Given the low importance of BMPs to today's web, the obvious solution here is to just always fire HAS_TRANSPARENCY for BMPs.
Assignee | ||
Comment 1•6 years ago
|
||
This patch actually makes the change described above. The rest of the patches are just for testing. Detecting HAS_TRANSPARENCY during the metadata decode is actually extremely easy for non-BMP formats because the information we need is right there in the header. Generally we only have to run a little bit more code before the metadata decode early return. For BMP, as discussed in comment 0, it's not possible to detect HAS_TRANSPARENCY from just the header, so we take a pessimistic approach and fire HAS_TRANSPARENCY all the time for BMP.
Attachment #8643357 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•6 years ago
|
||
Comment on attachment 8643357 [details] [diff] [review] (Part 1) - Always detect HAS_TRANSPARENCY during the metadata decode Glenn, you're the expert on the PNG decoder, so I'd appreciate any feedback you might have.
Attachment #8643357 -
Flags: feedback?(glennrp+bmo)
Assignee | ||
Comment 3•6 years ago
|
||
This part adds a DecoderFactory function to allow creating a metadata decoder without an associated image, which will make it possible to write unit tests for metadata decoders.
Attachment #8643361 -
Flags: review?(tnikkel)
Assignee | ||
Comment 4•6 years ago
|
||
When writing the next part, I realized that ImageTestCase was starting to grow lots of boolean flags, and it became clear that a bitfield would make the code cleaner. This patch switches ImageTestCase over to using a bitfield for its flags.
Attachment #8643365 -
Flags: review?(tnikkel)
Assignee | ||
Comment 5•6 years ago
|
||
This part actually adds the tests for part 1. (Finally, whew!) As long as I was at it, the tests also check other metadata decode-related things. There'll be even more stuff to put in here once we start detecting animation during the metadata decode, as mentioned in comment 0. This test covers all the cases that the mochitest test_has_transparency.html covers, with the exception of SVGs, since we're testing decoders here and not RasterImage/VectorImage.
Attachment #8643369 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a368feb529a
Updated•6 years ago
|
Attachment #8643361 -
Flags: review?(tnikkel) → review+
Comment 7•6 years ago
|
||
Comment on attachment 8643357 [details] [diff] [review] (Part 1) - Always detect HAS_TRANSPARENCY during the metadata decode Looks OK.
Attachment #8643357 -
Flags: feedback?(glennrp+bmo) → feedback+
Comment 8•6 years ago
|
||
We don't need the color chunks (cHRM and iCCP) during metadata decode, so we could add " || IsMetaDataDecode()" in nsPNGDecoder.cpp, like this: // Ignore unused chunks if (mCMSMode == eCMSMode_Off || IsMetaDataDecode() { png_set_keep_unknown_chunks(mPNG, 1, color_chunks, 2); }
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Glenn Randers-Pehrson from comment #8) > We don't need the color chunks (cHRM and iCCP) during metadata decode, so > we could add " || IsMetaDataDecode()" in nsPNGDecoder.cpp, like this: > > // Ignore unused chunks > if (mCMSMode == eCMSMode_Off || IsMetaDataDecode() { > png_set_keep_unknown_chunks(mPNG, 1, color_chunks, 2); > } Thanks for the feedback. I'll make that change!
Assignee | ||
Comment 10•6 years ago
|
||
I fixed several bugs that were detected in the last try job: - dom/workers/test/serviceworkers/test_fetch_event.html had a bug that was causing the image it was using to be corrupted by garbage bytes. It just coincidentally passed before; now that we look at a little bit more of the header, decoding fails. In bug 1193125 I fixed the test. - image/test/mochitest/test_has_transparency.html was failing because of the change to assume BMPs are always transparent. I could've just updated the test, but I decided to take another look at the issue, and I was able to modify the patch to treat a large class of BMPs as opaque, including the ones used in test_has_transparency.html. I've added tests in part 4 to make sure that we correctly handle the class of BMPs that we're forced to assume are transparent. These are basically BMPs that use RLE4 or RLE8 encoding, or 32bpp BMPs when BMP alpha data is enabled. - image/test/crashtests/856616.gif was failing because it tests the case where a GIF has a header but no actual frame data, and we were hitting the |done| label in nsGIFDecoder::WriteInternal(). That triggers an assertion that we don't run that code for metadata decodes. I fixed this by detecting this case and exiting early for metadata decodes. I'm expecting try to be green now that I've made these changes.
Attachment #8646141 -
Flags: review?(tnikkel)
Assignee | ||
Comment 11•6 years ago
|
||
This updated version of part 4 adds new BMP tests to verify that we detect the categories of BMPs mentioned in the previous comment as transparent.
Attachment #8646142 -
Flags: review?(tnikkel)
Assignee | ||
Comment 12•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bddfcebcd73
Assignee | ||
Updated•6 years ago
|
Attachment #8643357 -
Attachment is obsolete: true
Attachment #8643357 -
Flags: review?(tnikkel)
Assignee | ||
Updated•6 years ago
|
Attachment #8643369 -
Attachment is obsolete: true
Attachment #8643369 -
Flags: review?(tnikkel)
Assignee | ||
Comment 13•6 years ago
|
||
Try looks nice and green now.
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 8646142 [details] [diff] [review] (Part 4) - Add tests for metadata decoding, including that we always deliver HAS_TRANSPARENCY during the metadata decode Review of attachment 8646142 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/test/gtest/Common.cpp @@ +135,5 @@ > } > > ImageTestCase GreenICOTestCase() > { > + // This ICO contains a BMP, so it's always considered transparent as well. I need to update this comment; missed this when rebasing the patch. We don't always consider BMPs transparent anymore.
Assignee | ||
Comment 15•6 years ago
|
||
One more tweak: I forgot to include Glenn's suggested change of ignoring color chunks during the metadata decode.
Attachment #8646520 -
Flags: review?(tnikkel)
Assignee | ||
Updated•6 years ago
|
Attachment #8646141 -
Attachment is obsolete: true
Attachment #8646141 -
Flags: review?(tnikkel)
Assignee | ||
Comment 16•6 years ago
|
||
This version just fixes the out-of-date comment mentioned in comment 14.
Attachment #8646528 -
Flags: review?(tnikkel)
Assignee | ||
Updated•6 years ago
|
Attachment #8646142 -
Attachment is obsolete: true
Attachment #8646142 -
Flags: review?(tnikkel)
Updated•6 years ago
|
Attachment #8646520 -
Flags: review?(tnikkel) → review+
Updated•6 years ago
|
Attachment #8643365 -
Flags: review?(tnikkel) → review+
Updated•6 years ago
|
Attachment #8646528 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 17•6 years ago
|
||
Thanks for the reviews!
Assignee | ||
Comment 18•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db1ac46dacc https://hg.mozilla.org/integration/mozilla-inbound/rev/fce34ae03523 https://hg.mozilla.org/integration/mozilla-inbound/rev/721ecdee33a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f849cfa409
Comment 19•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3db1ac46dacc https://hg.mozilla.org/mozilla-central/rev/fce34ae03523 https://hg.mozilla.org/mozilla-central/rev/721ecdee33a7 https://hg.mozilla.org/mozilla-central/rev/f7f849cfa409
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•