RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
Alexey Ivanov
aivanov at openjdk.org
Mon Jan 8 13:50:29 UTC 2024
On Thu, 30 Nov 2023 19:20:50 GMT, Sergey Bylokhov <serb 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.
>
> Can this patch be covered by the new test?
> 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.
I believe Sergey's code in the test is correct. There are two images: `destTest` and `destGold`. The former is produced using the wrapper class, the latter is produced using the ICC color space. The wrapper `TestColorSpace` forwards all the calls to the `ICC_ColorSpace` that it wraps, therefore the transformed images must be *equal* because the applied transforms are absolutely the same.
To avoid any confusion, I suggest renaming `compareImages` to `areImagesEqual` which leaves no ambiguity for its return value. I've updated the code in [commit `1788ef6`](https://github.com/aivanov-jdk/jdk/commit/1788ef69958cefef7c65fdf37607f0410b868aff).
> 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 ?
I'm not sure… We want the filter to take another path, there could be a list of filters applied, if I understand @mrserb correctly. Sergey could be able to provide a more detailed guidance.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1881044740
More information about the client-libs-dev
mailing list