Bug 33624 – Open-source image decoders have decode bugs when scaling

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

[Peter Kasting]

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.

[Peter Kasting]

Created attachment 46504 [details]
patch v1

[Peter Kasting]

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.

[Peter Kasting]

Created attachment 46508 [details]
patch v2

[David Levin]

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).

[Yong Li]

(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.

[Yong Li]

(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.

[Peter Kasting]

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.

[Yong Li]

(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.

[Yong Li]

Peter, I'll do a test with your patch.

[Yong Li]

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)

[Adam Barth]

Comment on attachment 46512 [details]
patch v2.1

I wish this had tests, but I understand why it doesn't.

[Peter Kasting]

Fixed in r53305.