863123 - APNG frame attributes are set one frame behind

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Justin Dolske [:Dolske]]

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

[Mayank Bansal]

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.

Comment 2

[Justin Dolske [:Dolske]]

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

[Mayank Bansal]

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

Comment 4

[Alice0775 White]

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

Comment 5

[Justin Dolske [:Dolske]]

joedrew: why do you hate robots? WHY?

Comment 6

[Joe Drew (not getting mail)]

Attachment: 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.

Comment 7

[Joe Drew (not getting mail)]

https://tbpl.mozilla.org/?tree=Try&rev=7a0b1c929496

Comment 8

[Joe Drew (not getting mail)]

Attachment: diff ignoring whitespace

Yikes; that patch looks a lot scarier than it is. Here's a whitespace-ignoring patch.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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 10

[Seth Fowler [:seth] [:s2h]]

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?

Comment 11

[Seth Fowler [:seth] [:s2h]]

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

Comment 12

[Joe Drew (not getting mail)]

(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

Comment 13

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7950bce752cc

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7950bce752cc

Comment 15

[Joe Drew (not getting mail)]

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

Comment 16

[Joe Drew (not getting mail)]

 https://hg.mozilla.org/releases/mozilla-aurora/rev/931bd53af21e