Bug 33624 - Open-source image decoders have decode bugs when scaling
: Open-source image decoders have decode bugs when scaling
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Images
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Peter Kasting
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-13 13:56 PST by Peter Kasting
Modified: 2010-01-14 17:30 PST (History)
1 user (show)

See Also:


Attachments
patch v1 (1.45 KB, patch)
2010-01-13 14:05 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v2 (2.27 KB, patch)
2010-01-13 14:24 PST, Peter Kasting
levin: review-
Details | Formatted Diff | Diff
patch v2.1 (2.38 KB, patch)
2010-01-13 14:48 PST, Peter Kasting
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2010-01-13 13:56:12 PST
When image scaling is on, the PNGImageDecoder wrongly assumes that all images are 4 bytes per pixel, when some images may have no alpha channel.  This leads to reading/displaying garbage.  Simple patch to fix coming soon.
Comment 1 Peter Kasting 2010-01-13 14:05:37 PST
Created attachment 46504 [details]
patch v1
Comment 2 Peter Kasting 2010-01-13 14:22:14 PST
Yong, I think there's a similar (though inverted) bug in JPEG decoding for CMYK JPEGs:

for (int x = 0; x < numColumns; ++x) {
    JSAMPLE* jsample = src + scaledColumns[x] * 3;
    unsigned c = jsample[0];
    unsigned m = jsample[1];
    unsigned y = jsample[2];
    unsigned k = jsample[3];
    ...

Note how we access four bytes, but only step by three.

I'm going to update my patch to fix this too.
Comment 3 Peter Kasting 2010-01-13 14:24:48 PST
Created attachment 46508 [details]
patch v2
Comment 4 David Levin 2010-01-13 14:40:59 PST
Comment on attachment 46508 [details]
patch v2

The change looks great, but there is no layout test. Please either add one or explain why that isn't possible (in the changelog).
Comment 5 Yong Li 2010-01-13 14:42:06 PST
(In reply to comment #2)
> Yong, I think there's a similar (though inverted) bug in JPEG decoding for CMYK
> JPEGs:
> for (int x = 0; x < numColumns; ++x) {
>     JSAMPLE* jsample = src + scaledColumns[x] * 3;
>     unsigned c = jsample[0];
>     unsigned m = jsample[1];
>     unsigned y = jsample[2];
>     unsigned k = jsample[3];
>     ...
> Note how we access four bytes, but only step by three.
> I'm going to update my patch to fix this too.

? I don't think so.
Comment 6 Yong Li 2010-01-13 14:43:49 PST
(In reply to comment #5)
> (In reply to comment #2)
> > Yong, I think there's a similar (though inverted) bug in JPEG decoding for CMYK
> > JPEGs:
> > for (int x = 0; x < numColumns; ++x) {
> >     JSAMPLE* jsample = src + scaledColumns[x] * 3;
> >     unsigned c = jsample[0];
> >     unsigned m = jsample[1];
> >     unsigned y = jsample[2];
> >     unsigned k = jsample[3];
> >     ...
> > Note how we access four bytes, but only step by three.
> > I'm going to update my patch to fix this too.
> ? I don't think so.

oops. never mind.
Comment 7 Peter Kasting 2010-01-13 14:48:11 PST
Created attachment 46512 [details]
patch v2.1

Added a sentence about why there is no layout test.

If Yong wants to create a pair of tests for this and add them to the patch here, he's welcome to do so.  I don't know whether the platforms that support scaling are running the pixel tests, though.
Comment 8 Yong Li 2010-01-14 13:49:04 PST
(In reply to comment #7)
> Created an attachment (id=46512) [details]
> patch v2.1
> 
> Added a sentence about why there is no layout test.
> 
> If Yong wants to create a pair of tests for this and add them to the patch
> here, he's welcome to do so.  I don't know whether the platforms that support
> scaling are running the pixel tests, though.

I also have no idea whether there's any buildbot that turns this image decoder down-sampling on.
Comment 9 Yong Li 2010-01-14 14:16:47 PST
Peter, I'll do a test with your patch.
Comment 10 Yong Li 2010-01-14 15:17:19 PST
I just tested the patch agains a cmyk jpeg and a 24bit png. I've seen problems and the patch fixes them. (I modified the threshold to force scaling)
Comment 11 Adam Barth 2010-01-14 17:25:45 PST
Comment on attachment 46512 [details]
patch v2.1

I wish this had tests, but I understand why it doesn't.
Comment 12 Peter Kasting 2010-01-14 17:30:29 PST
Fixed in r53305.