RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]

Sergey Bylokhov serb at openjdk.org
Fri Jan 5 03:34:26 UTC 2024


On Thu, 4 Jan 2024 18:58:02 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

>>> > I thought about it… I thought that the filter itself should exercise the code path that's modified but it doesn't seem like it does… I mean creating images with regular ICC profile and using the wrapper TestColorSpace in conversion: new ColorConvertOp(new TestColorSpace(ColorSpace.getInstance(CS_LINEAR_RGB), null); but it didn't work, the test passes without the fix.
>>> 
>>> It is passed since it blindly checks the difference between src and dst, and since with proper implementation of TestColorSpacewe we always do "some" conversion -> the images are different -> the test passed.
>> 
>> Still, the current test catches the typo in the code `srcColorSpace` → `dstColorSpace` that's now replaced. From this point view, the stated bug is fixed.
>> 
>> At the same time, I agree there seem to be more bugs in the area.
>
>> Still, the current test catches the typo in the code srcColorSpace → dstColorSpace that's now replaced. From this point view, the stated bug is fixed.
> 
> But the implementation of TestColorSpacewe is wrong, and it may not work as expected after any other future updates.

There are at least two similar copy/paste typos.
In the next line https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L771 we use the srcNumComp which is the number of components for the incoming source color space, but the loop should be done over iccSrcNumComp which is a number of components of the "intermidiate" source color space. Similar issue is in the next line but for the destination https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L785

It is possible that these compensate/affects each other and produce some non-completly broken result.
The test from the comment above start to pass after these will be fixed. 

Since we found a few bugs in this code path, it will be useful to check the code path in the nonICCBIFilter where the CSList is not null(the intermidiate transform is wrapper as well, as of now our test uses the built-in ColorSpace.getInstance(CS_sRGB))

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442465871


More information about the client-libs-dev mailing list