RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v11]
Alexey Ivanov
aivanov at openjdk.org
Fri Jan 5 15:52:24 UTC 2024
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
More information about the client-libs-dev
mailing list