RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
Alexey Ivanov
aivanov at openjdk.org
Fri Jan 12 21:08:25 UTC 2024
On Fri, 12 Jan 2024 09:53:46 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:
>
> 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
More information about the client-libs-dev
mailing list