Bug 33624

Summary: Open-source image decoders have decode bugs when scaling
Product: WebKit Reporter: Peter Kasting <pkasting>
Component: ImagesAssignee: Peter Kasting <pkasting>
Status: RESOLVED FIXED    
Severity: Normal CC: yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch v1
none
patch v2
levin: review-
patch v2.1 abarth: review+

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.