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

[Renjith Kannath Pariyangad]

On Fri, 5 Jan 2024 15:49:56 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> 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.

@aivanov-jdk , Thank you for the detailed investigation. 
Earlier I didn't do much investigation with or with out fix after getting _ArrayIndexOutOfBoundsException_. Now its narrow down as with fix there is no _array out of bound_ and with out there is. 
About the failure post fix : I think its because of typo, As per the logic comparing two image and if its not equal then test pass. I think  if condition missed `not (!)` (original code return _true_ if match and _false_ if docent, in Sergey sample `compareImages ` returns _true_ if not match and _false_ if match but `if (compareImages(destTest, destGold))` same. @mrserb hope this observation is correct.

Thank to @mrserb and @aivanov-jdk for your time and investigation, this helped to bring up few more issues. What do you suggest to cover these cases ?

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

PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1880373713