Closed
Bug 863123
Opened 12 years ago
Closed 12 years ago
APNG frame attributes are set one frame behind
Categories
(Core :: Graphics: ImageLib, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla23
Tracking | Status | |
---|---|---|
firefox21 | --- | unaffected |
firefox22 | + | fixed |
firefox23 | + | fixed |
People
(Reporter: Dolske, Assigned: joe)
References
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
patch
|
seth
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
Oh, the inhumanity.
Our about:robots page includes a tiny robot favicon (see browser/base/content/aboutRobots.xhtml, it's a data: URI). Some people don't know that it's actually an APNG (let's keep this quiet, just between you and I). The eyes will flash at a slow interval... Once every 30 seconds, IIRC.
This isn't happening on trunk (4/15 OS X Nightly). Instead it's looping every second or so (incorrect timeout), and the flashing eyes _replace_ the image instead of being drawn on top of it (incorrect rendering).
This is a hueg disaster.
Exterminate.
Comment 1•12 years ago
|
||
Looks pretty good to me. The flashes match well with the theme of robots.. Think fast flashing lights on space ship control panels and eyes of robots....
When i first saw it, i thought it is an added geekiness to the about:robots page.
Also, since the page itself takes about 20-40 seconds to read, its unlikely that people remain on the page long enough to see the slow eye flashing. In fact, i never knew before that the favicon had any animation at all.
IMO, this should remain.
Reporter | ||
Comment 2•12 years ago
|
||
It's a bit of an easteregg. In any case, design of about:robots is outside the scope of this bug. The image isn't being displayed per spec, and we should fix that.
Comment 3•12 years ago
|
||
Resolve the bug, definitely.
Just consider making this fast flashing behavior intentional.
Comment 4•12 years ago
|
||
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c9c29cc97af0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320062640
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1407288d820b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320074043
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9c29cc97af0&tochange=1407288d820b
Suspected: Bug 716140
Blocks: 716140
status-firefox21:
--- → unaffected
tracking-firefox22:
--- → ?
tracking-firefox23:
--- → ?
Keywords: regression
OS: Mac OS X → All
Reporter | ||
Comment 5•12 years ago
|
||
joedrew: why do you hate robots? WHY?
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → joe
Summary: favicon in about:robots has gone crazy → APNG frame attributes are set one frame behind
Assignee | ||
Comment 6•12 years ago
|
||
The PNG decoder does things slightly differently, and actually gives you the *next* frame's attributes. So we need to write down those attributes before we start the frame, and then set them on the Decoder when we're done the frame as usual.
Attachment #739662 -
Flags: review?(seth)
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
Yikes; that patch looks a lot scarier than it is. Here's a whitespace-ignoring patch.
Comment 9•12 years ago
|
||
This has a good testcase, and it makes me wonder what other breakage we might hear about when this gets to Beta. Tracking and hoping joedrew, hater of robots, will verify a regression fix.
status-firefox22:
--- → affected
status-firefox23:
--- → affected
Comment 10•12 years ago
|
||
Comment on attachment 739738 [details] [diff] [review]
diff ignoring whitespace
Review of attachment 739738 [details] [diff] [review]:
-----------------------------------------------------------------
r+ if my concern below is unfounded. If you have to make substantial changes, please rerequest review.
::: image/decoders/nsPNGDecoder.cpp
@@ +160,5 @@
> +#ifdef PNG_APNG_SUPPORTED
> + if (png_get_valid(mPNG, mInfo, PNG_INFO_acTL)) {
> + mAnimInfo = AnimFrameInfo(mPNG, mInfo);
> + }
> +#endif
It's not immediately clear to me whether this works correctly for the first frame. Is the relevant info already available when CreateFrame() is called for the first time?
Attachment #739738 -
Flags: review+
Comment 11•12 years ago
|
||
Comment on attachment 739662 [details] [diff] [review]
Write down frame attributes when we want to create the frame, and then set it in StopFrame
Review of attachment 739662 [details] [diff] [review]:
-----------------------------------------------------------------
This is r+ iff the other version is r+. =)
Attachment #739662 -
Flags: review?(seth) → review+
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #10)
> It's not immediately clear to me whether this works correctly for the first
> frame. Is the relevant info already available when CreateFrame() is called
> for the first time?
Yep. In fact, this is just restoring the old behaviour: http://hg.mozilla.org/releases/mozilla-beta/file/cc63217713dc/image/decoders/nsPNGDecoder.cpp#l112
Assignee | ||
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 739662 [details] [diff] [review]
Write down frame attributes when we want to create the frame, and then set it in StopFrame
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716140
User impact if declined: Animated PNGs (including those we ship) display wrong
Testing completed (on m-c, etc.): on m-c for a few days
Risk to taking this patch (and alternatives if risky): Not terribly risky; restoring a code path we had before.
String or IDL/UUID changes made by this patch: None
Attachment #739662 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #739662 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 16•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•