Closed Bug 1319025 Opened 3 years ago Closed 3 years ago

Optimized APNGs get misrendered

Categories

(Core :: ImageLib, defect, P3)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 + wontfix
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: s.brone, Assigned: aosmond)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(2 files, 1 obsolete file)

Attached image Animation as described
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021

Steps to reproduce:

The attached APNG is loaded in Firefox. This APNG was created with APNGASM.exe. 

The problem occurs both when the APNG is loaded by itself and when it is embedded in an arbitrary HTML page. I tried turning off hardware acceleration, but this had no effect.

I have tried this on 50.0 and today's (11-20-2016) nightly build. My co-worker did not have the problem this morning on 49.0.2. But ever since he upgraded to 50.0 he has been affected as well.


Actual results:

The APNG is created in such a way that its file size is as small as possible. Only the first frame contains the complete image, the other frames only contains deltas. However, the first frame is no longer used as the base frame, so only the delta frames are being shown.

This causes the animation to show a stack of books moving, however the entire background image is lost.


Expected results:

The animation attached should show a stack of books moving towards a monitor (which is on the background).

I have been using this APNG for over two years now and it has never given me any problems.
Confirming, the attached APNG does not render properly for me either. (I've seen other APNGs also affected)

Name: Firefox
Version: 50.0
Build ID: 20161114215630
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
OS: Linux 4.8.7-200.fc24.x86_64
Multiprocess Windows: 0/1 (Disabled by add-ons)
Safe Mode: false (but I also tried with safe mode enabled and the same problem occurs)
Component: Untriaged → ImageLib
Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0009a43ad49a316c86cdb52bcb44006b32011170&tochange=39ba4da73c6c040e291520ecbec2d61bdeb3d168
Blocks: 1255107
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Keywords: regression
Flags: needinfo?(seth.bugzilla)
Version: 53 Branch → 50 Branch
I'll take the NI.
Flags: needinfo?(seth.bugzilla) → needinfo?(aosmond)
At first it looked as if we were rendering it correctly (GIMP with the APNG is similar to what we do now) but I believe now this is caused by a spec violation. From https://wiki.mozilla.org/APNG_Specification :

>>             iv) Disposal Method - Indicates the way in which the graphic is to
>>            be treated after being displayed.
>>
>>            Values :    0 -   No disposal specified. The decoder is
>>                              not required to take any action.
>>                        1 -   Do not dispose. The graphic is to be left
>>                              in place.
>>                        2 -   Restore to background color. The area used by the
>>                              graphic must be restored to the background color.
>>                        3 -   Restore to previous. The decoder is required to
>>                              restore the area overwritten by the graphic with
>>                              what was there prior to rendering the graphic.
>>                     4-7 -    To be defined.

According to disposal method 2 (DisposalMethod::CLEAR), we should restore the "area used by the graphic". This used to be be bounded by the frame rect but we actually remove the frame rect using RemoveFrameRectFiltuer with the SurfacePipe changes. As a result, the frame size used by FrameAnimator::DoBlend is always the full frame, and that's what we use when we do the dispose operation. This should be changed back to use the frame rect area instead.

I believe this should also affect animated GIFs, although I may be mistaken given we have no reports of that (and they should be more common than APNG).
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Flags: needinfo?(aosmond)
Errr, mea culpa. That was the *GIF* spec which I was also confirming should do the same :). The relevant *PNG* specification section is:

>>    APNG_DISPOSE_OP_NONE: no disposal is done on this frame before rendering the next; the contents of the output buffer are left as is.
>>    APNG_DISPOSE_OP_BACKGROUND: the frame's region of the output buffer is to be cleared to fully transparent black before rendering the next frame.
>>    APNG_DISPOSE_OP_PREVIOUS: the frame's region of the output buffer is to be reverted to the previous contents before rendering the next frame.
Priority: -- → P3
Whiteboard: gfx-noted
Attached patch Fix disposal method handling, v1 (obsolete) — Splinter Review
First kick at the can. I'm not sure if we have the testing coverage to properly vet this, will look into crafting APNGs and GIFs for gtests.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18187383b9af8b3f7c8f1e1cc799a1794057e667
I have a nice APNG test suite here, covering a bunch of basic features:
http://littlesvr.ca/apng/test.html

And it seems one of the files there (apng10.png) triggers this bug.
You can use this file, or better yet, the whole suite.
Track 51+/52+/53+ as APNG does not render properly.
Added a mochitest to test this scenario (apng reftest do not work). Confirmed that GIFs are unaffected (due to the blend method I believe).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c07c30b294c8aba894262e0b5a6d72d1917fec25
Attachment #8813391 - Attachment is obsolete: true
Attachment #8814442 - Flags: review?(tnikkel)
Attachment #8814442 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d35e993149
Fix how animated images disposal method should use frame rect size instead of the image size as its bounds. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/74d35e993149
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi :aosmond, 
Is this worth uplifting to Beta51 & Aurora52?
Flags: needinfo?(aosmond)
Comment on attachment 8814442 [details] [diff] [review]
Fix disposal method handling, v2

Approval Request Comment
[Feature/Bug causing the regression]: 1255107
[User impact if declined]: Some animated PNGs may look wrong.
[Is this code covered by automated tests?]: Yes. Existing test cases covered most APNG operations, and a new test case was added for this regression / to verify the fix.
[Has the fix been verified in Nightly?]: Yes, on 20161216030207, I manually loaded the APNG attached to this test case. Added mochitest is passing.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It is small and localized to a single function. Problem is very well understood, we regressed and violated the APNG specification.
[String changes made/needed]: None.
Flags: needinfo?(aosmond)
Attachment #8814442 - Flags: approval-mozilla-aurora?
Comment on attachment 8814442 [details] [diff] [review]
Fix disposal method handling, v2

Approval Request Comment
[Feature/Bug causing the regression]: 1255107
[User impact if declined]: Some animated PNGs may look wrong.
[Is this code covered by automated tests?]: Yes. Existing test cases covered most APNG operations, and a new test case was added for this regression / to verify the fix.
[Has the fix been verified in Nightly?]: Yes, on 20161216030207, I manually loaded the APNG attached to this test case. Added mochitest is passing.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It is small and localized to a single function. Problem is very well understood, we regressed and violated the APNG specification.
[String changes made/needed]: None.
Attachment #8814442 - Flags: approval-mozilla-beta?
Comment on attachment 8814442 [details] [diff] [review]
Fix disposal method handling, v2

fix apng regression in aurora52
Attachment #8814442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8814442 [details] [diff] [review]
Fix disposal method handling, v2

Fix an APNG regression and was verified. Beta51+. Should be in 51 beta 9.
Attachment #8814442 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1328685
Duplicate of this bug: 1331051
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.