Optimized APNGs get misrendered

RESOLVED FIXED in Firefox 51

Status

()

Core
ImageLib
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: sbrone, Assigned: aosmond)

Tracking

({regression})

50 Branch
mozilla53
regression
Points:
---

Firefox Tracking Flags

(firefox50+ wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8812723 [details]
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.

Comment 1

a year ago
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)

Updated

a year ago
Component: Untriaged → ImageLib
(Assignee)

Comment 2

a year ago
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

Updated

a year ago
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Flags: needinfo?(seth.bugzilla)

Updated

a year ago
Version: 53 Branch → 50 Branch
(Assignee)

Comment 3

a year ago
I'll take the NI.
Flags: needinfo?(seth.bugzilla) → needinfo?(aosmond)
(Assignee)

Comment 4

a year ago
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)
(Assignee)

Comment 5

a year ago
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.
(Assignee)

Updated

a year ago
Priority: -- → P3
Whiteboard: gfx-noted
(Assignee)

Comment 6

a year ago
Created attachment 8813391 [details] [diff] [review]
Fix disposal method handling, v1

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

Comment 7

a year ago
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.
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox53: ? → +
(Assignee)

Comment 9

a year ago
Created attachment 8814442 [details] [diff] [review]
Fix disposal method handling, v2

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+

Comment 10

a year ago
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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74d35e993149
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi :aosmond, 
Is this worth uplifting to Beta51 & Aurora52?
Flags: needinfo?(aosmond)
(Assignee)

Comment 13

a year ago
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?
(Assignee)

Comment 14

a year ago
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+

Comment 17

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6a581f5ae0b
status-firefox52: affected → fixed

Comment 18

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b828b2c86ea9
status-firefox51: affected → fixed
Too late for 50.
status-firefox50: affected → wontfix
tracking-firefox50: ? → +

Updated

a year ago
Duplicate of this bug: 1328685

Updated

a year ago
Duplicate of this bug: 1331051
You need to log in before you can comment on or make changes to this bug.