On Fri, 5 Jan 2024 13:18:46 GMT, Renjith Kannath Pariyangad <rkannathp...@openjdk.org> wrote:
>> 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)) > > @mrserb for avoiding _ArrayIndexOutOfBoundsException_ I have modified > `CS_GRAY `with `CS_LINEAR_RGB ` for making same number of channels. But the > test passed irrespective of fix. > Do you find any alternative way to fail the test with out fix ? > 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. Indeed, these should be `iccSrcNumComp` and `iccDstNumComp` according to number of components in the allocated arrays. Modifying these lines make the test pass. > 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)) Agree, another (unit) test is required which exercises other code paths. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1443068123
