900200 - (apng) Strange transparent areas appear in some APNG files

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Max Stepin]

Attachment: 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.

Comment 1

[Max Stepin]

Bug is in 2013-06-21 nightly. 
No bug in 2013-06-20 nightly or older.

No big when it's a (similar) GIF.

Comment 2

[Alice0775 White]

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

Comment 3

[bhavana bajaj [:bajaj]]

Joe, passing this over to you for investigation given this may be a fallout from 867774

Comment 4

[Max Stepin]

Attachment: v01.patch

Proposed patch.

Comment 5

[Alice0775 White]

Comment on attachment 791844 [details] [diff] [review]
v01.patch

AFAIK, Joe Drew is not working on Mozilla any more...

Comment 6

[Seth Fowler [:seth] [:s2h]]

One thing that's clear is that we need a test for this. Max, is that something you can take care of?

Comment 7

[Max Stepin]

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?

Comment 8

[Seth Fowler [:seth] [:s2h]]

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.

Comment 9

[Max Stepin]

OK, I'll try to make the tests.

Comment 10

[Max Stepin]

Attachment: v02.patch

2nd proposed patch. Includes a test.

Comment 11

[Max Stepin]

Attachment: bug900200.png

Comment 12

[Max Stepin]

Attachment: bug900200-ref.png

Comment 13

[Glenn Randers-Pehrson]

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 14

[Seth Fowler [:seth] [:s2h]]

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+!

Comment 15

[Seth Fowler [:seth] [:s2h]]

If try looks green this is ready for checkin.

https://tbpl.mozilla.org/?tree=Try&rev=db096778f8ba

Comment 16

[Max Stepin]

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.

Comment 17

[Seth Fowler [:seth] [:s2h]]

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. =)

Comment 18

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9986101468df

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9986101468df

Comment 20

[bhavana bajaj [:bajaj]]

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 ?

Comment 21

[bhavana bajaj [:bajaj]]

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

Comment 22

[Max Stepin]

[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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/507a9fc2d45b
https://hg.mozilla.org/releases/mozilla-beta/rev/dadfae804013

Comment 24

[Max Stepin]

Tested in Nightly, works fine!

Comment 25

[Petruta Horea [:phorea]]

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

Comment 26

[u279076]

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.

Comment 27

[Petruta Horea [:phorea]]

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