APNG frame attributes are set one frame behind

RESOLVED FIXED in Firefox 22

Status

()

P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Dolske, Assigned: joe)

Tracking

({regression})

unspecified
mozilla23
x86
All
regression
Points:
---

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ fixed, firefox23+ fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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

6 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

6 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

6 years ago
Resolve the bug, definitely.
Just consider making this fast flashing behavior intentional.

Comment 4

6 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

6 years ago
joedrew: why do you hate robots? WHY?
(Assignee)

Updated

6 years ago
Assignee: nobody → joe
Summary: favicon in about:robots has gone crazy → APNG frame attributes are set one frame behind
(Assignee)

Comment 6

6 years ago
Created attachment 739662 [details] [diff] [review]
Write down frame attributes when we want to create the frame, and then set it in StopFrame

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 8

6 years ago
Created attachment 739738 [details] [diff] [review]
diff ignoring whitespace

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.
status-firefox22: --- → affected
status-firefox23: --- → affected
tracking-firefox22: ? → +
tracking-firefox23: ? → +
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+
(Assignee)

Comment 12

6 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
https://hg.mozilla.org/mozilla-central/rev/7950bce752cc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox23: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 15

6 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?
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.