Closed Bug 863123 Opened 7 years ago Closed 7 years ago

APNG frame attributes are set one frame behind

Categories

(Core :: ImageLib, defect, P1)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox21 --- unaffected
firefox22 + fixed
firefox23 + fixed

People

(Reporter: Dolske, Assigned: joe)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
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.
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.
Resolve the bug, definitely.
Just consider making this fast flashing behavior intentional.
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
Keywords: regression
OS: Mac OS X → All
joedrew: why do you hate robots? WHY?
Assignee: nobody → joe
Summary: favicon in about:robots has gone crazy → APNG frame attributes are set one frame behind
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)
Yikes; that patch looks a lot scarier than it is. Here's a whitespace-ignoring patch.
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.
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 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+
(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
https://hg.mozilla.org/mozilla-central/rev/7950bce752cc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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?
Attachment #739662 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.