Summary: | Open-source image decoders have decode bugs when scaling | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Kasting <pkasting> | ||||||||
Component: | Images | Assignee: | Peter Kasting <pkasting> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | yong.li.webkit | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Peter Kasting
2010-01-13 13:56:12 PST
Created attachment 46504 [details]
patch v1
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. Created attachment 46508 [details]
patch v2
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).
(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. (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. 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.
(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. Peter, I'll do a test with your patch. 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 on attachment 46512 [details]
patch v2.1
I wish this had tests, but I understand why it doesn't.
Fixed in r53305. |