New issue
Advanced search Search tips

Issue 698808 link

Starred by 2 users
Status: Assigned
Owner:
scroggo@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 699675
issue 437662


Sign in to add a comment

How to handle incorrect particular APNG images

Project Member Reported by scroggo@chromium.org, Mar 6 2017 
APNG support ( issue 437662 ) is in progress at https://codereview.chromium.org/2618633004/

There are a few error cases brought up in https://philip.html5.org/tests/apng/tests.html that the patch does not address. They are discussed in https://codereview.chromium.org/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode326.

case 58 (058.png): "Default image's fcTL size not matching IHDR."
The spec suggests (https://wiki.mozilla.org/APNG_Specification#Error_Handling) that if there is an error, an APNG should be treated as a static PNG. In this case, even when treated as a static PNG, the image is wrong - it only contains enough IDAT data for the first 32 out of 64 rows. Currently, libpng and PNGImageDecoder do not treat this image as an error. Should they? If not, PNGImageDecoder will need to determine that not enough rows were received, and fail if the image is an APNG.

One could even modify the file to only contain the default image (as in 058-oneFrame.png), such that treating it as a static PNG draws no differently than drawing the broken APNG.

case 59 (059.png): "fdAT too small."
This is a similar case to 58. We need to do the same checking, but in this case reverting to the default image shows something different than the incorrect APNG.

case 60 (60.png): "fdAT too large."
This is analogous to an IDAT that is too large. PNGImageDecoder currently ignores this problem (for static PNGs). In this case, libpng does report a warning, so if we want to consider this an error, we can listen for the warning. In this file, reverting to the default image is sensible, although similar to 58 we could create a file where the IDAT is too large, and we end up showing the same image regardless of whether we treated it as an APNG failure.
058.png
398 bytes View Download
059.png
405 bytes View Download
060.png
622 bytes View Download
058-oneFrame.png
197 bytes View Download

Comment 1 by scroggo@chromium.org, Mar 6 2017 

(Interestingly, monorail considered 058.png and 058-oneFrame.png to be broken images, though Chromium does not.)

Comment 2 by scroggo@chromium.org, Mar 8 2017

Components: Internals>Images>Codecs

Comment 3 by scroggo@chromium.org, Mar 23 2017

Blockedon: 699675
> case 60 (60.png): "fdAT too large."
> This is analogous to an IDAT that is too large. PNGImageDecoder currently
> ignores this problem (for static PNGs). In this case, libpng does report a
> warning, so if we want to consider this an error, we can listen for the
> warning.

More work needs to be done than just listening to the warning. My initial thought was that we could just call setFailed(), but that does not change anything for this file with deferred decoding. (Some other ImageDecoder clients will check to see if the image failed and return an empty SkBitmap if there was a failure.)

There is only one frame, so BitmapImage happily continues drawing this failed frame. (If there were more frames following, the PNGImageDecoder would refuse to parse and decode them.)

Comment 4 by benhenry@chromium.org, Aug 1

Status: Assigned (was: Untriaged)
►

Sign in to add a comment