Closed Bug 1103328 Opened 5 years ago Closed 5 years ago

Fire HAS_TRANSPARENCY for animated images that use kDisposeClear, for PNGs with first-frame padding, and for raw icons

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(5 files, 4 obsolete files)

This bug is a followup for bug 1089880 that corrects a couple of oversights I noticed after a change in bug 1102617 to not mark all animated images as non-opaque in nsDisplayImage.

- Since the 'clear' disposal methods can expose the background under the image, they need to be considered transparency.

- PNGs can also have first-frame padding and we need to fire HAS_TRANSPARENCY in that situation, just like with GIFs.

- Our 'raw' icon decoder (nsIconDecoder) should always fire HAS_TRANSPARENCY, since any such icon could be transparent. (We always mark the frames from that decoder as having alpha, so this is consistent with what we were doing before.)

I also added a comment to clarify the relationship between PostHasTransparency and PostFrameStop's aFrameAlpha argument, which is used to decide whether to call imgFrame::SetHasNoAlpha. It's important that HAS_TRANSPARENCY fire as early as possible, so the key difference is that it usually relies on information from headers, while PostFrameStop may examine the frame content itself. That means that HAS_TRANSPARENCY is conservative, while PostFrameStop may be more precise. (Whether that additional precision buys us anything is questionable, but we need imgFrame::SetHasNoAlpha until we can be sure that preallocated frames are always correct, so now's not the time to think about removing it.)
Summary: Fire HAS_TRANSPARENCY for animated images that use kDisposeClear and for raw icons → Fire HAS_TRANSPARENCY for animated images that use kDisposeClear, for PNGs with first-frame padding, and for raw icons
Here are the patches.
Attachment #8527150 - Flags: review?(tnikkel)
Attachment #8527152 - Flags: review?(tnikkel)
In this part I add an assertion that whenever we report that a frame has alpha in PostFrameStop, we also already sent a HAS_TRANSPARENCY notification. That should make sure we catch any further issues like the ones in this bug. To make that true I had to also fire HAS_TRANSPARENCY in a couple of error cases, which is also handled in this part.
Attachment #8527153 - Flags: review?(tnikkel)
Sigh... Apparently I didn't introduce ProgressTracker::GetProgress until a later patch, so reordering my patch queue busted this. It's trivial, so I'll just add it here instead.
Attachment #8527155 - Flags: review?(tnikkel)
Attachment #8527153 - Attachment is obsolete: true
Attachment #8527153 - Flags: review?(tnikkel)
Here's a new try job that will hopefully actually build:

https://tbpl.mozilla.org/?tree=Try&rev=03483635169c
(In reply to Seth Fowler [:seth] from comment #4)
> In this part I add an assertion that whenever we report that a frame has
> alpha in PostFrameStop, we also already sent a HAS_TRANSPARENCY
> notification.

Alright, after investigating those assertion failures in the previous try job, I've concluded that I don't actually want to do this. It'd require firing HAS_TRANSPARENCY for all animated images, for example. That's something I explicitly want to avoid if I can.

I'm running some tests locally, but I'm probably going to remove everything from part 4 but the comment change.
In this part we fire HAS_TRANSPARENCY during the size decode, so I needed to change the constraints in CheckProgressConsistency. This actually makes a lot of sense; it'd be ideal if we could always fire HAS_TRANSPARENCY during the size decode.
Attachment #8527242 - Flags: review?(tnikkel)
Attachment #8527152 - Attachment is obsolete: true
Attachment #8527152 - Flags: review?(tnikkel)
Alright, I'm just going to add the new comment in part 4 now. As I mentioned in comment 8, I don't think that assertion is the right way to go.
Attachment #8527243 - Flags: review?(tnikkel)
Attachment #8527155 - Attachment is obsolete: true
Attachment #8527155 - Flags: review?(tnikkel)
Attachment #8527150 - Flags: review?(tnikkel) → review+
Attachment #8527151 - Flags: review?(tnikkel) → review+
Attachment #8527242 - Flags: review?(tnikkel) → review+
(In reply to Seth Fowler [:seth] from comment #9)
> it'd be ideal if we could always fire HAS_TRANSPARENCY during
> the size decode.

Size decoding would get a little more complex.  Right now we just look
at the width and height words that are near the beginning of the PNG.
But to find out if we have transparency we have to look at the color type
from the IHDR chunk (easy) but then we must scan up to the first IDAT
looking for a tRNS chunk.  Neither of those tell us whether there actually
is transparency in the image, but they do tell us when there surely is
no transparency (PNG color type 0, 2, or 3 and no tRNS chunk).
(In reply to Glenn Randers-Pehrson from comment #12)
> Size decoding would get a little more complex.

Absolutely, but I think there are good reasons to make it a little more complex. In addition to finding out whether the first frame has alpha, we also want to know whether it has padding. That will let us make sure that the frame we preallocate when we start decoding is always of the correct type, and we can totally get rid of the logic for replacing frames.

I'd also love if we could determine if the image is animated during a size decode, and it seems like we can probably do that for animated GIF by checking if a delay is specified for the first frame. I haven't looked into APNG to see if there are any problems there.

> Neither of those tell us whether there actually
> is transparency in the image, but they do tell us when there surely is
> no transparency (PNG color type 0, 2, or 3 and no tRNS chunk).

That's true. It's OK if HAS_TRANSPARENCY is a bit pessimistic, though.
I saw a failure in test_has_transparency once that I haven't been able to reproduce. My best guess is that it was caused because we're not forcing decoding in this test. Even if that's not the cause, it'd be nice to eliminate it conclusively so that if the failure pops up again, we know that it's something else. So this patch makes us force decoding in test_has_transparency by drawing the images into a canvas.
Attachment #8527899 - Flags: review?(tnikkel)
Renumbered to part 5.
Attachment #8527901 - Flags: review?(tnikkel)
Attachment #8527243 - Attachment is obsolete: true
Attachment #8527243 - Flags: review?(tnikkel)
Attachment #8527899 - Flags: review?(tnikkel) → review+
Comment on attachment 8527901 [details] [diff] [review]
(Part 5) - Add better documentation for PostHasTransparency

Can you spell out what pessimistic means here specifically? (I mean I know, but it might not be clear)
Attachment #8527901 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #16)
> Can you spell out what pessimistic means here specifically? (I mean I know,
> but it might not be clear)

I messed up and didn't address this before pushing, so I pushed a followup that spells out more clearly what I mean by "pessimistic":

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5dd8eb32a32
You need to log in before you can comment on or make changes to this bug.