Detect HAS_TRANSPARENCY during the metadata decode

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

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
Assignee

Description

4 years ago
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

4 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

4 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

4 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

4 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

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

Comment 9

4 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

Updated

4 years ago
Depends on: 1193125
Assignee

Comment 10

4 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

4 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

Updated

4 years ago
Attachment #8643357 - Attachment is obsolete: true
Attachment #8643357 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Attachment #8643369 - Attachment is obsolete: true
Attachment #8643369 - Flags: review?(tnikkel)
Assignee

Comment 13

4 years ago
Try looks nice and green now.
Assignee

Comment 14

4 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

4 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

4 years ago
Attachment #8646141 - Attachment is obsolete: true
Attachment #8646141 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
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+
Assignee

Comment 17

4 years ago
Thanks for the reviews!
Assignee

Updated

4 years ago
Blocks: 1194059
Assignee

Updated

4 years ago
Blocks: 1210553

Updated

4 years ago
Blocks: 1231951
You need to log in before you can comment on or make changes to this bug.