463465 - Artifacts in APNG frames

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tracy Walker [:tracy]]

The test case for fixed bug 420416 has regressed in nightly trunk builds.

the second image has a smear

Comment 1

[Ria Klaassen (not reading all bugmail)]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-12+08%3A00%3A00&enddate=2008-09-13+09%3A00%3A00
tested on the presence of the stripes.

Comment 3

[Glenn Randers-Pehrson]

This is closely related to bug #433047 if not a dupe.

Comment 4

[Glenn Randers-Pehrson]

Attachment: Patch to clear the prev_row before each APNG frame

Comment 5

[Glenn Randers-Pehrson]

Attachment: Patch to clear the prev_row before each APNG frame (v1)

also takes care of non-progressive reading

Comment 6

[Glenn Randers-Pehrson]

Comment on attachment 377978 [details] [diff] [review]
Patch to clear the prev_row before each APNG frame (v1)

This does not need to wait for libpng-1.2.37, which does not address this problem.

Comment 7

[Glenn Randers-Pehrson]

Would someone with access to the tri-server please make a tri-server build with this patch?

Comment 8

[Joe Drew (not getting mail)]

Done - it's building now. I'll update when it's done.

Comment 9

[Joe Drew (not getting mail)]

https://build.mozilla.org/tryserver-builds/[e-mail]-glennrp-apng-v1/ - linux is done, others will be showing up. I haven't looked at unittest results, though.

Comment 10

[Glenn Randers-Pehrson]

The test case for bug #420416 still has the artifacts with the triserver Windows build.  I haven't tried the other two builds.  The test case for bug #433047 also still displays artifacts.

Comment 11

[Glenn Randers-Pehrson]

The test case for this bug looks OK, however.

Comment 12

[Glenn Randers-Pehrson]

This (v1) patch will be included in the upcoming libpng-1.2.37 release that's due out June 4th.

Comment 13

[Glenn Randers-Pehrson]

Attachment: Patch to clear the prev_row before each APNG frame (v2)

Moved memset of prev_row into png_read_reinit()

@joe: please make another triserver build with this patch together with the patch from bug #397593

Comment 14

[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 15

[Glenn Randers-Pehrson]

Sorry, the previous comment belongs in bug #441971.

Comment 16

[Glenn Randers-Pehrson]

The test case mentioned in the description (solar flare image) looks fine with Joe's latest triserver build, which includes the v2 patch.  The simple TEST 1/TEST 2 image also looks fine.

Comment 17

[Glenn Randers-Pehrson]

They look fine on Windows XP.  I am not in position to try the other platforms.

Comment 18

[Glenn Randers-Pehrson]

The v2 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 19

[u279076]

Attachment: Screenshot

Here is a screenshot of the issue.

Comment 20

[Tony Chung [:tchung]]

Adding [FFT3.5] in whiteboard.  Came across this issue during RC1 testing

Comment 21

[Max Stepin]

Can this one be approved for check-in?

Comment 22

[Reed Loden [:reed]]

Fixed by landing of bug 504805.

Comment 23

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

Comment on attachment 380718 [details] [diff] [review]
Patch to clear the prev_row before each APNG frame (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.