On Thu, 4 Jan 2024 18:58:02 GMT, Sergey Bylokhov <[email protected]> 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
