Closed
Bug 1103328
Opened 10 years ago
Closed 9 years ago
Fire HAS_TRANSPARENCY for animated images that use kDisposeClear, for PNGs with first-frame padding, and for raw icons
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(5 files, 4 obsolete files)
2.32 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
2.09 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 1•10 years ago
|
||
Here are the patches.
Attachment #8527150 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8527151 -
Flags: review?(tnikkel)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8527152 -
Flags: review?(tnikkel)
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.
Attachment #8527153 -
Flags: review?(tnikkel)
Assignee | ||
Comment 5•10 years ago
|
||
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=766e04d4ff90
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.
Attachment #8527155 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8527153 -
Attachment is obsolete: true
Attachment #8527153 -
Flags: review?(tnikkel)
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
Assignee | ||
Comment 8•9 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.
Assignee | ||
Comment 9•9 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.
Attachment #8527242 -
Flags: review?(tnikkel)
Assignee | ||
Updated•9 years ago
|
Attachment #8527152 -
Attachment is obsolete: true
Attachment #8527152 -
Flags: review?(tnikkel)
Assignee | ||
Comment 10•9 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.
Attachment #8527243 -
Flags: review?(tnikkel)
Assignee | ||
Updated•9 years ago
|
Attachment #8527155 -
Attachment is obsolete: true
Attachment #8527155 -
Flags: review?(tnikkel)
Assignee | ||
Comment 11•9 years ago
|
||
Here's a new try job: https://tbpl.mozilla.org/?tree=Try&rev=8d005554de8f
Updated•9 years ago
|
Attachment #8527150 -
Flags: review?(tnikkel) → review+
Updated•9 years ago
|
Attachment #8527151 -
Flags: review?(tnikkel) → review+
Updated•9 years ago
|
Attachment #8527242 -
Flags: review?(tnikkel) → review+
Comment 12•9 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).
Assignee | ||
Comment 13•9 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.
Assignee | ||
Comment 14•9 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.
Attachment #8527899 -
Flags: review?(tnikkel)
Assignee | ||
Comment 15•9 years ago
|
||
Renumbered to part 5.
Attachment #8527901 -
Flags: review?(tnikkel)
Assignee | ||
Updated•9 years ago
|
Attachment #8527243 -
Attachment is obsolete: true
Attachment #8527243 -
Flags: review?(tnikkel)
Updated•9 years ago
|
Attachment #8527899 -
Flags: review?(tnikkel) → review+
Comment 16•9 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)
Attachment #8527901 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 17•9 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
Assignee | ||
Comment 18•9 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
Comment 19•9 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
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•