1191114 - Detect HAS_TRANSPARENCY during the metadata decode

Status: RESOLVED FIXED

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Always detect HAS_TRANSPARENCY during the metadata decode

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add support for creating an anonymous metadata decoder, for use in tests

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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add flags to image test cases

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add tests for metadata decoding, including that we always deliver HAS_TRANSPARENCY during the metadata decode

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a368feb529a

Comment 7

[Glenn Randers-Pehrson]

Comment on attachment 8643357 [details] [diff] [review]
(Part 1) - Always detect HAS_TRANSPARENCY during the metadata decode

Looks OK.

Comment 8

[Glenn Randers-Pehrson]

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);
  }

Comment 9

[Seth Fowler [:seth] [:s2h]]

(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!

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Always detect HAS_TRANSPARENCY during the metadata decode

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.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add tests for metadata decoding, including that we always deliver HAS_TRANSPARENCY during the metadata decode

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.

Comment 12

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bddfcebcd73

Comment 13

[Seth Fowler [:seth] [:s2h]]

Try looks nice and green now.

Comment 14

[Seth Fowler [:seth] [:s2h]]

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.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Always detect HAS_TRANSPARENCY during the metadata decode

One more tweak: I forgot to include Glenn's suggested change of ignoring color
chunks during the metadata decode.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add tests for metadata decoding, including that we always deliver HAS_TRANSPARENCY during the metadata decode

This version just fixes the out-of-date comment mentioned in comment 14.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Thanks for the reviews!

Comment 18

[Seth Fowler [:seth] [:s2h]]

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

[Ryan VanderMeulen [:RyanVM]]

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