On Fri, 5 Jan 2024 15:49:56 GMT, Alexey Ivanov <[email protected]> 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
