1352282 - fix png decoder to set loop count even for first frame only decodes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Timothy Nikkel (:tnikkel)]

First frame only decodes of pngs do not parse the loop count, so the loop count gets a default value of 0. 0 is actually treated like a finite loop in FrameAnimator. (-1 is infinite loop). If you get the right order of events when there is an animated and first frame only decode both in flight at the same time the wrong value of 0 can overwrite the correct value of -1, and then FrameAnimator treats this as a finite loop and decides its finished animating and stops the animation. test_discardAnimatedImages.html actually caught this but it didn't make the test turn red cause we don't check for that specifically (ie if you ran it locally you saw the problem).

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: improve the test to catch this

I verified that this would have caught this bug.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8853209 [details] [diff] [review]
patch

Review of attachment 8853209 [details] [diff] [review]:
-----------------------------------------------------------------

Couldn't we just fix the decoder to always post the frame loop count when it is finished like the GIF decoder (looks like I did WebP animations to match the GIF decoder behaviour too)? I would prefer the behaviour of decoders be consistent, unless there is obvious overhead we want to avoid, and avoid comments in the code which suggest the metadata isn't trustworthy when it really is. While some animated parameters are conditional on a full decode, it makes a lot of sense why those particular parameters are unknown (refresh area, loop length in milliseconds) unless we do a full decode.

The animation control chunk stores the number of frames and the loop count -- we know it has been encountered already because we use that in the metadata decoding (and according to the spec, guaranteed to be before any frame data). I think it amounts to moving this chunk of code into nsPNGDecoder::FinishInternal:

http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/image/decoders/nsPNGDecoder.cpp#1028

We can safely ignore the nsPNGDecoder::FinishInternalWithError method because nsPNGDecoder::end_callback ends the decoding successfully immediately after (and even asserts to make sure we don't have an error set). The updated test case is still good though I imagine :).

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8853318 [details] [diff] [review]
improve the test to catch this

Review of attachment 8853318 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/mochitest/test_discardAnimatedImage.html
@@ +99,4 @@
>      gNumDiscards++;
>      ok(true, "got image discard");
> +    if (arrayIndex >= 2) {
> +      // The last two images are finite, so we don't expect any frame updates,

Shouldn't we expect at least two frame updates? At least one update while/just after the first frame decodes, from the decoder itself via Decoder::PostInvalidation (>= 1 for APNG, == 1 for GIF), and one update from the refresh request which it switches frames via RasterImage::RequestRefresh (>= 1, no matter how many frames as it could skip them)? The gNumFrameUpdates could count down from the number of expected updates to simplify the logic (since there are multiple thresholds if you do this).

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #4)
> Shouldn't we expect at least two frame updates? At least one update
> while/just after the first frame decodes, from the decoder itself via
> Decoder::PostInvalidation (>= 1 for APNG, == 1 for GIF),

I think I'd consider that a bug. Since we aren't going to paint the first frame.

> and one update from
> the refresh request which it switches frames via RasterImage::RequestRefresh
> (>= 1, no matter how many frames as it could skip them)? The

It should skip them all actually. FrameAnimator::RequestRefresh should never be called because the animation is finished, so the RasterImage will have mAnimating = false. The composited frame still has the final frame from before the discard (we don't discard the composited frame yet).

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: improve test v2

This test checks that drawImage actually draws something. The required me to change the finite gif because the first frame of the previous finite gif was completely transparent. Since drawImage is first frame only I also take a snapshot of the two infinite gifs and check that they actually change in addition to getting frame update msgs.

I also added a couple setTimeout(..,0) because we weren't usually painting the images to the screen before we marked them display none. Paint suppression isn't always over when the load event fires, to be sure paint suppression is over you have to do a setTimeout from the load event. In addition I made sure layout was flushed before requesting discard: if we didn't flush layout it's possible we'd hold a lock to the images and so the discard request would be ignored. I didn't observe this however.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd86eea082a
Always fill in the number of loops when decoding an APNG file. r=aosmond
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb514a085715
Improve test_discardAnimatedImage.html to check that the animation continues when we re-decode. r=aosmond

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/5bd86eea082a
https://hg.mozilla.org/mozilla-central/rev/cb514a085715

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa7fb0cc00c
In test_discardAnimatedImage.html call drawWindow from a setTimeout from frame update notification.

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Pulsebot from comment #10)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa7fb0cc00c
> In test_discardAnimatedImage.html call drawWindow from a setTimeout from
> frame update notification.

I pushed a followup to the test based on an assert I was getting intermittently on try server.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/5aa7fb0cc00c