Closed Bug 1191114 Opened 5 years ago Closed 5 years ago

Detect HAS_TRANSPARENCY during the metadata decode

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

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.
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)
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)
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)
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)
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)
Attachment #8643361 - Flags: review?(tnikkel) → review+
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+
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);
  }
(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!
Depends on: 1193125
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)
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)
Attachment #8643357 - Attachment is obsolete: true
Attachment #8643357 - Flags: review?(tnikkel)
Attachment #8643369 - Attachment is obsolete: true
Attachment #8643369 - Flags: review?(tnikkel)
Try looks nice and green now.
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.
One more tweak: I forgot to include Glenn's suggested change of ignoring color
chunks during the metadata decode.
Attachment #8646520 - Flags: review?(tnikkel)
Attachment #8646141 - Attachment is obsolete: true
Attachment #8646141 - Flags: review?(tnikkel)
This version just fixes the out-of-date comment mentioned in comment 14.
Attachment #8646528 - Flags: review?(tnikkel)
Attachment #8646142 - Attachment is obsolete: true
Attachment #8646142 - Flags: review?(tnikkel)
Attachment #8646520 - Flags: review?(tnikkel) → review+
Attachment #8643365 - Flags: review?(tnikkel) → review+
Attachment #8646528 - Flags: review?(tnikkel) → review+
Thanks for the reviews!
Blocks: 1194059
Blocks: 1210553
Blocks: 1231951
You need to log in before you can comment on or make changes to this bug.