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

[Alexey Ivanov]

On Fri, 5 Jan 2024 12:03:43 GMT, Renjith Kannath Pariyangad <rkannathpari at 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