Closed Bug 900200 Opened 6 years ago Closed 6 years ago

(apng) Strange transparent areas appear in some APNG files

Categories

(Core :: ImageLib, defect)

24 Branch
x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 + verified
firefox25 + verified
firefox26 --- verified

People

(Reporter: newstop, Assigned: newstop)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached image test.png
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130731 Firefox/25.0 (Nightly/Aurora)
Build ID: 20130731030203

Steps to reproduce:

Attached animation has to transparency, yet Firefox decodes it with some strange transparent areas in bottom-right corner. Animation is simple, all blend ops are "source", all dispose ops are "none":
frame 1: [200x200]
frame 2: [100x100]
frame 3: [50x50]

Another example:
http://philip.html5.org/tests/apng/tests.html#8-bit-greyscale
This should be a solid grey rectangle containing a solid white rectangle.

The regression is quite recent, it first appeared in 2013-06-21 nightly, no bug in older builds.
Keywords: regression
Bug is in 2013-06-21 nightly. 
No bug in 2013-06-20 nightly or older.

No big when it's a (similar) GIF.
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5b3196ad66f4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130619 Firefox/24.0 ID:20130619143654
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f5f8e2d6d991
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130619 Firefox/24.0 ID:20130619144755
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5b3196ad66f4&tochange=f5f8e2d6d991

Regressed by: Bug 867774
Version: 25 Branch → 24 Branch
OS: Windows 7 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Joe, passing this over to you for investigation given this may be a fallout from 867774
Assignee: nobody → joe
Assignee: joe → nobody
Attached patch v01.patch (obsolete) — Splinter Review
Proposed patch.
Attachment #791844 - Flags: review?(joe)
Comment on attachment 791844 [details] [diff] [review]
v01.patch

AFAIK, Joe Drew is not working on Mozilla any more...
Attachment #791844 - Flags: review?(joe) → review?(seth)
One thing that's clear is that we need a test for this. Max, is that something you can take care of?
Flags: needinfo?(newstop)
I don't have much experience creating tests, but I guess we can 
take this one image from a test suite:
http://philip.html5.org/tests/apng/tests.html#8-bit-greyscale
and test it the most right-bottom pixel for transparency?
Or maybe to add the correct (reference) image and compare the second frame to it?
Flags: needinfo?(newstop)
The reference image is the way to go. Making the actual test is pretty easy; take a look here:

https://mxr.mozilla.org/mozilla-central/source/image/test/reftest/animated/reftest.list

You'd add a line to that file in the section that mentions "operators that only APNG supports" that would look like this:

"== delay-test.html?bug900200.png no-delay-test.html?bug900200-ref.png"

And you'd add two files to that directory, "bug900200.png" (an APNG which does not loop and exhibits that bug without your patch) and "bug900200-ref.png" (a non-animated PNG that looks the same as the other image after the animation has finished).

The real work is in the images themselves. There are two things to note here:

1. The licensing must be compatible with the W3C Document License and the BSD 3-clause license. Practically speaking, I would avoid using images you find in the wild unless they are explicitly public domain or CC0. It's better to create new images.

2. It may save a bit of time if the reference image is the same as one of the existing reference images. In particular, you could reuse 'green.png' or 'grey.png' in that directory instead of creating bug900200-ref.png if you construct bug900200.png right. This part is up to you, though.
OK, I'll try to make the tests.
Attached patch v02.patchSplinter Review
2nd proposed patch. Includes a test.
Attachment #791844 - Attachment is obsolete: true
Attachment #791844 - Flags: review?(seth)
Attachment #792791 - Flags: review?(seth)
Attached image bug900200.png
Attached image bug900200-ref.png
I patched today's mozilla-central repo with the v02 patch and it displays the test.png (first attachment, above) properly on Ubuntu.  I had also applied the libpng-1.6.3 v15 patch from bug #841734, but that should not matter.
Comment on attachment 792791 [details] [diff] [review]
v02.patch

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

Sorry for the delay, Max. I wanted to do some testing on this one before reviewing it and I didn't have a chance to do that until today.

Everything looks great! I really appreciate you putting a reftest together, too. r+!
Attachment #792791 - Flags: review?(seth) → review+
If try looks green this is ready for checkin.

https://tbpl.mozilla.org/?tree=Try&rev=db096778f8ba
Hopefully, delaytest works now?

Because a few years ago, I created a patch for bug 580182l and it was accepted, but Bobby Holley also made a test, but as you can see, he was unable to land it.
ISTR that Joe made changes to make these kinds of tests more reliable at some point in the past year. At any rate, things look good on try, so I think it's reasonable to take our chances and land this. =)
https://hg.mozilla.org/mozilla-central/rev/9986101468df
Assignee: nobody → newstop
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Can you please request approval nomination for aurora/beta asap, so this can land in time for our Beta going to build tomorrow morning PT ?
Attachment #792791 - Flags: approval-mozilla-beta?
Attachment #792791 - Flags: approval-mozilla-aurora?
We need to have this the form filled with approval request comment before I approve this :

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch: 

I am going to flag the dev/reviewer to help with that
Flags: needinfo?(seth)
Flags: needinfo?(newstop)
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Regressed by bug 867774
User impact if declined: Some APNG files will display with noticeable artifacts.
Testing completed (on m-c, etc.): tested locally, patch includes a reftest, also tryserver is green: https://tbpl.mozilla.org/?tree=Try&rev=db096778f8ba
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None
Flags: needinfo?(newstop)
Attachment #792791 - Flags: approval-mozilla-beta?
Attachment #792791 - Flags: approval-mozilla-beta+
Attachment #792791 - Flags: approval-mozilla-aurora?
Attachment #792791 - Flags: approval-mozilla-aurora+
Flags: needinfo?(seth)
Tested in Nightly, works fine!
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS 10.7; rv:24.0) Gecko/20100101 Firefox/24.0

Verified as fixed on Firefox 24 beta 10 (20130909203154) using the following pages:
https://bugzilla.mozilla.org/attachment.cgi?id=783987
https://bugzilla.mozilla.org/attachment.cgi?id=793448
http://philip.html5.org/tests/apng/tests.html#8-bit-greyscale

Does this still needs manual testing considering Comment 22? Thank you
Flags: needinfo?
QA Contact: petruta.rasa
Petruta, you need to set the need-info? flag with an email address otherwise no one will respond. Please test this again with Firefox 25 similar to what you did for comment 25.
Status: RESOLVED → VERIFIED
Flags: needinfo?
Keywords: verifyme
Thanks, Anthony.

Verified fixed using latest Aurora (20130910004002):
Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0
You need to log in before you can comment on or make changes to this bug.