Seth Fowler [:seth] [:s2h] Assignee
	Description • 10 years ago

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.)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 1 • 10 years ago

Here are the patches.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 4 • 10 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 10 years ago

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=766e04d4ff90

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 6 • 10 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 7 • 10 years ago

Here's a new try job that will hopefully actually build:

https://tbpl.mozilla.org/?tree=Try&rev=03483635169c

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 10 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 9 • 10 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 10 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 11 • 10 years ago

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=8d005554de8f

	Glenn Randers-Pehrson
	Comment 12 • 10 years ago

(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).

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 13 • 10 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 14 • 10 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 15 • 10 years ago

Renumbered to part 5.

	Timothy Nikkel (:tnikkel)
	Comment 16 • 10 years ago

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)

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 17 • 10 years ago

Thanks for the review! Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4f24936bba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bec65d242677
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48a1c4550955
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d92d3a036dfd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b020dee629be

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 18 • 10 years ago

(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

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/9e4f24936bba
https://hg.mozilla.org/mozilla-central/rev/bec65d242677
https://hg.mozilla.org/mozilla-central/rev/48a1c4550955
https://hg.mozilla.org/mozilla-central/rev/d92d3a036dfd
https://hg.mozilla.org/mozilla-central/rev/b020dee629be
https://hg.mozilla.org/mozilla-central/rev/f5dd8eb32a32