Closed Bug 900200 Opened 6 years ago Closed 6 years ago
(apng) Strange transparent areas appear in some APNG files
1.03 KB, image/png
4.05 KB, patch
|Details | Diff | Splinter Review|
840 bytes, image/png
660 bytes, image/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.
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
Joe, passing this over to you for investigation given this may be a fallout from 867774
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?
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?
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.
2nd proposed patch. Includes a test.
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. =)
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 ?
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
[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
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
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.
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.