Closed Bug 403364 Opened 12 years ago Closed 12 years ago

APNG animations sometimes loop incorrectly

Categories

(Core :: ImageLib, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: Dolske, Assigned: alfredkayser)

References

Details

Attachments

(2 files)

While verifying the test changes in bug 399542, I found that that the test APNGs there displayed/animated properly when I viewed them all sequentially. But if I then viewed them again, sometimes they wouldn't loop properly (dropped frames).

I managed to get a reproducible, if sucky, test case (attached). Load it, and click the buttons left to right and then right to left (1A, 1B, 1C, 2A, 2B, 3, 2B, 2A, 1C, 1B, 1A). You need to wait 10+ seconds between button clicks or the bug doesn't manifest itself. This makes me suspect bug 296818 triggers the problem.

Expected:

1A is a R->G->B loop.
1B is a R->G->B loop.
1C is a G->B loop.
2A is a R->G->B loop with gradients
2B is a R->G->B loop with gradients
3  is a diagonal line drawn pixel-by-pixel.
[The images are from modules/libpr0n/test/unit/test_encoder_apng.js.]

Actual:

When clicking 1B (sometimes 1A?) again at the end of the sequence, instead of a R->G->B loop I get a R->G loop.
Flags: blocking1.9?
Status: NEW → ASSIGNED
Marking blocking +P2 since it is a new feature that is broken.  Change prio if you disagree..
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Wondering if Stuart or Vlad can provide hints on this.  I can repro and am looking for P1/P2 bugs to pitch in on, if needed.
Crowder, sounds like you could step through/printf-debug nsPNGDecoder::SetAnimFrameInfo... probably it's doing incorrect state manipulations

If you don't have time I might be able to take this one as well
andrew, any idea here?
I've also noticed this bug with the new throbber in Proto, which is an APNG now.
Blocks: proto
I don't have a mac to test it on.
There is something fishy in the imageContainer restore image code when the image is an animated PNG/GIF. Either the 'restore' stuff needs to be disable for animated images, or we need to think more thorough about the logic for discarding data there. At least discarding images should be done as long as the animation runs (some animations only go one time...).

Secondly, the reuse of image containers could cause some issues here as possible not all animated data is reset good enough.
The problem: The decoders (that implement 'Discardable') use 'GetImage' to get the image related to the 'ContainerObserver'. This is either imgILoad (in normal case) or a ContainerLoader (implementing a fake imgILoad) (when the image is reloaded).
That is causing all kinds of confusion, because the decoder then reuses the image object even if is not 'reloading' the image. Possible multiple ways to fix: make imgRequest to not re-use the imageContainer, or make another 'Init' for the decoders that take not a 'ContainerObserver' but a 'Image' object directly (in this way the decoder also know whether it is an initial (incremental) load, or a reload (non-incremental).

So, when loading another image (which happens to have the same size) make the PNG decoder to re-use the image object (even if not really relevant for that object).
Blocks: 399925
This is valid issue for all platforms!
OS: Mac OS X → All
Hardware: PC → All
What at least would help if the 'Init' of imgContainer also resets all the frames and the discardable/restore data stuff (as they are no longer valid!).

Also 'mDiscarded' needs to be reset to FALSE before ReloadImages, to prevent that during the 'ReloadImages', via GetCurrentFrameNoRef, RestoreDiscardedData, ReloadImages is called again (this generates a lot of assertions now).
Duplicate of this bug: 406009
The main thing of this patch is to reset mDiscarded to PR_FALSE before ReloadImages (which should have been called 'RestoreImages'), to prevent that ReloadImages is called multiple times (this happens with animated images, where the timer ask for another frame, while ReloadImages is still restoring...).
Assignee: nobody → alfredkayser
Attachment #297317 - Flags: review?(pavlov)
Comment on attachment 297317 [details] [diff] [review]
V1: Cleanup the discard logic to be more robust espec. for animated images


>+  // If image is not discarble, don't start discard timer
>+  if (!mDiscardable)
>     return NS_OK;

Nit: typo in "discardable".
Attachment #297317 - Flags: review?(pavlov) → review+
Checking in modules/libpr0n/src/imgContainer.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgContainer.cpp,v  <--  imgContainer.cpp
new revision: 1.64; previous revision: 1.63
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.