On Fri, 5 Jan 2024 12:03:43 GMT, Renjith Kannath Pariyangad <rkannathp...@openjdk.org> wrote:
>> Hi Reviewers, >> There was a typo for color conversion instead of dstColorSpace function >> srcColorSpace was used. Please review and let me know your suggestions if >> any. >> >> Renjith. > > Renjith Kannath Pariyangad has updated the pull request incrementally with > one additional commit since the last revision: > > Removed unwanted converion I've been thinking about it more, and I agree with Sergey. There are two types of `ColorSpace`s: ICC and non-ICC. The `TestColorSpace` is a wrapper on top of an `ICC_ColorSpace` (with Sergey's implementation that forwards all the method calls to its wrapped `ColorSpace` instance stored in the `cs` field. The expectation is that using the same ColorSpace result in the same output. We — _Sergey_ — do just that. 1. Take two images with an `ICC_ColorSpace` and transform them. 2. Take another pair of images with the wrapped `ICC_ColorSpace` and transform them. The result of the transformation must be the same, right? It's a better test. I've created [`renjith/sergey-8316497-colorConvertOp`](https://github.com/aivanov-jdk/jdk/tree/renjith/sergey-8316497-colorConvertOp) branch. It's based on Renjith's `[8316497-v1](https://github.com/Renjithkannath/jdk/tree/8316497-v1)` which corresponds to this PR. Sergey's code from [his comment](https://github.com/openjdk/jdk/pull/16895#discussion_r1441366764) above is available as [commit `ac6b10f`](https://github.com/aivanov-jdk/jdk/commit/ac6b10fa82e64934b46a642ad9a87d64d80affbf). I refactored it by introducing an enum for selecting the `ColorSpace`, it makes it easier to understand which `ColorSpace` is created. I modified `createTestImage` to accept a `ColorSpace`, this way it's not hidden inside but explicitly pass, which also improves the readability of the test. This is [commit `8e85480`](https://github.com/aivanov-jdk/jdk/commit/8e854807d63fe8dc5909c8562cb64cad53679749). Here's the link to my latest version of [the `NonICCFilterTest.java` test](https://github.com/aivanov-jdk/jdk/blob/8e854807d63fe8dc5909c8562cb64cad53679749/test/jdk/java/awt/color/NonICCFilterTest.java). Indeed, without Renjith's fix, the test fails with `ArrayIndexOutOfBoundsException`: Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 at java.desktop/java.awt.image.ColorConvertOp.nonICCBIFilter(ColorConvertOp.java:821) at java.desktop/java.awt.image.ColorConvertOp.filter(ColorConvertOp.java:275) at NonICCFilterTest.main(NonICCFilterTest.java:132) With the fix, the test still fails but differently: x = 0, y = 0 rgb1 = fff61041 rgb2 = ff00ff00 Exception in thread "main" java.lang.RuntimeException: Test failed at NonICCFilterTest.main(NonICCFilterTest.java:138) which means non-ICC color spaces aren't handled correctly. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1878880308
