Bug 49333 – simplify png decoding loop

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Pascal Massimino]

Created attachment 73520 [details]
simpler png loop

Hi,

the loop looks simpler this way.
I may have miss something here, though.

Pascal

[Peter Kasting]

Comment on attachment 73520 [details]
simpler png loop

This isn't quite right.  If row 0 has alpha and row 1 does not, you'll setHasAlpha(true) on the first row and (false) on the second.

Instead you should probably do:

if (nonTrivialAlpha)
    buffer.setHasAlpha(true);

...at the end.

It would be good to add a pixel test with a png that exercises this particular case.

[Pascal Massimino]

Created attachment 73538 [details]
updated patch

Fixed the logic, and made sure buffer.setHasAlpha() was called once, like before.
Added a pixel test for this case.

[Adam Barth]

Comment on attachment 73538 [details]
updated patch

This patch is incorrectly based.  Please use "webkit-patch upload" or svn-create-patch to create your patches so they can be processed by all our automated tools.

[Peter Kasting]

Comment on attachment 73538 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73538&action=review

Does the test image render visibly wrong with your original patch?  If so, I'm basically OK with this patch (can't r+, not a reviewer).

> third_party/WebKit/WebCore/ChangeLog:9
> +        Tests should stay be 1:1 the same.
> +        Added a special test for this case: png_per_row_alpha_decoding.html

Just say: "Test: fast/images/png_per_row_alpha_decoding.html" -- it's implied that image output won't change.

[Pascal Massimino]

Created attachment 73539 [details]
attachment with correct path base

same as 73538, but with correct path

[Adam Barth]

Comment on attachment 73539 [details]
attachment with correct path base

Marking r? for a few seconds so the bots pick up this change for analysis.

[Adam Barth]

Comment on attachment 73539 [details]
attachment with correct path base

If Peter's happy, this looks good to me.  Please address Peter's nit with the ChangeLog before landing.

[Pascal Massimino]

Created attachment 73548 [details]
updated patch

updated patch, with smaller PNG test image and fixed ChangeLog.

This test-PNG has an alpha-less first and last row, whereas the center
rows do have alpha. So whether setHasAlpha() occurs first or last, it
would be forcing a no-alpha bad image.

That being said, i see no effect of setHasAlpha() on linux. The renderer
seems to ignore this global flag and use whatever alpha value is associated
with each pixels.

[WebKit Review Bot]

Comment on attachment 73548 [details]
updated patch

Rejecting patch 73548 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 73548 [details]
updated patch

Clearing flags on attachment: 73548

Committed r71784: <http://trac.webkit.org/changeset/71784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

BTW, this was missing the .checksum file, I added it in http://trac.webkit.org/changeset/71836.