441971 - APNG_BLEND_OP_OVER not supported for apng's with indexed color (color type 3)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Reto Hoehener]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

Is it possible that the blend op 'OVER' is not supported for apngs with indexed color?

Reproducible: Always

Steps to Reproduce:
Try to open the attached file animated.png with firefox 3.
Actual Results:  
Firefox says: The image "animated.png" cannot be displayed, because it contains errors.

Expected Results:  
Not sure if this is expected behavior or not.

Comment 1

[Reto Hoehener]

Attachment: animated png created from 1-3.png (indexed color, blend op 'over')

Comment 2

[Reto Hoehener]

Attachment: frame 1 of animated.png

Comment 3

[Reto Hoehener]

Attachment: frame 2 of animated.png

Comment 4

[Reto Hoehener]

Attachment: frame 3 of animated.png

Comment 5

[Hadrien Nilsson]

Confirming with Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4

The attachment is a valid APNG file. Firefox fails to render APNG with uncommon pixel formats. Check the "Other depths and colour types" section on:

http://philip.html5.org/tests/apng/tests.html

Comment 6

[Glenn Randers-Pehrson]

Attachment: Eliminate too-stringent test for opaque images with blend_op==OVER (v0)

I think this patch will also fix (or partly fix) bug #455140

Comment 7

[Glenn Randers-Pehrson]

@joe, please make a triserver build with this patch.  You can combine it with the one I requested yesterday in bug #463465 or make a separate build -- your call.

Comment 8

[Glenn Randers-Pehrson]

Attachment: Eliminate too-stringent test for opaque images with blend_op==OVER (v1)

Fixed malformed comment that had an extra "*/"

Comment 9

[Glenn Randers-Pehrson]

I believe the purpose of the code that the patch removes was to save CPU cycles
in blending opaque images.  Throwing a png_error() seems an excessive response;
a png_warning() is more appropriate.  Also the test was incorrect because it
did not account for the possibility of a tRNS chunk in truecolor or grayscale
images.  Furthermore, the test rejects indexed-color PNGs but throws an error
saying it is rejecting a truecolor PNG.  Instead, we will simply ignore
PNG_BLEND_OP_OVER in opaque images and use the simpler PNG_BLEND_OP_SOURCE,
which produces the same result for opaque images, instead.

Comment 10

[Joe Drew (not getting mail)]

Glenn: Pushed a try build for you with the contents of bug 463465, bug 441971, and bug 397593. FYI, they don't apply perfectly cleanly in sequence, though it's not a problem in practice.

applying bug441971
applying bug397593
applying bug463465
patching file modules/libimg/png/MOZCHANGES
Hunk #1 succeeded at 6 with fuzz 2 (offset 3 lines).
Now at: bug463465

The builds will show up in https://build.mozilla.org/tryserver-builds/?C=M;O=D with userid [e-mail] and some random other ID (possibly a27690563c17).

Comment 11

[Glenn Randers-Pehrson]

The test case looks OK with the v1 patch.  It's tiny, so look carefully.  It's easier to see in favicons in the bookmark and the navigation window.

Comment 12

[Glenn Randers-Pehrson]

The v1 patch is incorporated in the libpng-1.2.37 patch (bug #492200)
If this one gets checked in before libpng-1.2.37 then I'll revise
the libpng-1.2.37 patch to avoid merge conflict.  If libpng patch gets
checked in first then this one will become obsolete.

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 380808 [details] [diff] [review]
Eliminate too-stringent test for opaque images with blend_op==OVER (v1)

Just delete the "following code deleted" code.

Comment 14

[Glenn Randers-Pehrson]

Attachment: Eliminate too-stringent test for opaque images with blend_op==OVER (v2)

Removed dead code

Comment 15

[Max Stepin]

Can this one be approved for check-in?

Comment 16

[Reed Loden [:reed]]

Fixed by landing of bug 504805.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 382889 [details] [diff] [review]
Eliminate too-stringent test for opaque images with blend_op==OVER (v2)

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 382889 [details] [diff] [review]
Eliminate too-stringent test for opaque images with blend_op==OVER (v2)

1.9.2.x is the place to fix this, or 1.9.2 final if you're willing to lobby the right folks (outside of bugzilla)

Comment 19

[Max Stepin]

Not fixed in Firefox 3.6.3

This bugfix is super safe. Could it be included into 3.6.x branch?

Comment 20

[Samuel Sidler (:ss)]

Comment on attachment 382889 [details] [diff] [review]
Eliminate too-stringent test for opaque images with blend_op==OVER (v2)

Only if you ask for approval on the patch. But note that you'll have to find someone to land this...

Comment 21

[LegNeato]

Comment on attachment 382889 [details] [diff] [review]
Eliminate too-stringent test for opaque images with blend_op==OVER (v2)

a=LegNeato for 1.9.2.5. Please ONLY land this on mozilla-1.9.2 default, as we
are still working on 1.9.2.4 on the relbranch

Comment 22

[Glenn Randers-Pehrson]

I need a "check-in buddy" to get this in to the 1.9.2 branch.

Please check the patch in and set the "status1.9.2" field to ".6-fixed".

Code-freeze for the 1.9.2.6 release is planned for the end of next
week. Please land soon and don't leave it to the last minute.

Comment 23

[Max Stepin]

Come on, Mozilla. The patch got review+ and superreview+ in June 2009. 

Nobody can use optimized APNG files on the web, the same way optimized GIFs are used, thanks to this bug.

Comment 24

[Bobby Holley (:bholley)]

(In reply to comment #23)
> Come on, Mozilla. The patch got review+ and superreview+ in June 2009. 

Fair enough. I'll land this tomorrow on mozilla-1.9.2 default assuming it applies cleanly.

Comment 25

[Bobby Holley (:bholley)]

pushed to 1.9.2 as http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cda37351e026

Resolving fixed. Let me know if this needs to be applied anywhere else.