On Fri, 12 Jan 2024 09:53:46 GMT, Renjith Kannath Pariyangad
<[email protected]> 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:
>
> Included wrapper check
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/color/NonICCFilterTest.java line 140:
> 138:
> 139: ColorConvertOp gold = new ColorConvertOp(mid, null);
> 140: gold.filter(srcGold, destGold);
Shouldn't both filter use `gold` instead?
Then later, you create `test` with a wrapper instead?
test/jdk/java/awt/color/NonICCFilterTest.java line 151:
> 149:
> 150: gold = new ColorConvertOp(mid, null);
> 151: gold.filter(srcGold, destGold);
Should both filter use the same instance of `ColorConvertOp` where the first
one is `gold` and is applied to both `gold-` and `test-` images, then the next
instance `ColorConvertOp` is a `test` with the wrapper which is again applied
to both images?
Wouldn't it be clearer?
Suggestion:
ColorConvertOp gold = new
ColorConvertOp(createCS(ColorSpaceSelector.PYCC), null);
gold.filter(srcTest, destTest);
gold.filter(srcGold, destGold);
if (!areImagesEqual(destTest, destGold)) {
throw new RuntimeException("ICC test failed");
}
ColorConvertOp test = new
ColorConvertOp(createCS(ColorSpaceSelector.WRAPPED_PYCC), null);
test.filter(srcTest, destTest);
test.filter(srcGold, destGold);
Currently, we have `test` and `gold` which use the same `ColorSpace` which
could be plain or wrapper for both cases. It looks confusing to me.
What do you think, @mrserb?
-------------
PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1819034007
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1450918862
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1450923724
