Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Fri, 12 Jan 2024 02:55:59 GMT, Sergey Bylokhov wrote: >> @mrserb , >> >> Did you mean to use the wrapper for the middle like `ColorSpace mid = >> createCS(ColorSpaceSelector.WRAPPED_PYCC);` , So we can achieve : >> >> **From** >> wrapper->icc_color_space->wrapper >> icc_color_space->icc_color_space->icc_color_space >> >>**To** : >> wrapper->wrapper->wrapper >> icc_color_space->wrapper->icc_color_space >> >> Correct me if I am wrong > >>Did you mean to use the wrapper for the middle like ColorSpace mid = >>createCS(ColorSpaceSelector.WRAPPED_PYCC); , So we can achieve : > > Just repeat existed checks using wrapper. So you will have all combinations: > >> wrapper->icc_color_space->wrapper >> icc_color_space->icc_color_space->icc_color_space >> wrapper->wrapper->wrapper >> icc_color_space->wrapper->icc_color_space Thank you, @mrserb, for the valuable review, the fix and the test have become more comprehensive as the result. Greatly appreciated! - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1898170895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v16]
On Tue, 16 Jan 2024 15:41:50 GMT, Renjith Kannath Pariyangad 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: > > Fixed small typo Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1823846459
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v16]
> 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: Fixed small typo - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/895de12f..c0ac3b03 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=15 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=14-15 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v7]
On Wed, 3 Jan 2024 19:21:47 GMT, Alexey Ivanov wrote: >> Renjith Kannath Pariyangad has updated the pull request incrementally with >> two additional commits since the last revision: >> >> - Copyright year updated >> - Review suggestions implemented > > test/jdk/java/awt/color/NonICCFilterTest.java line 38: > >> 36: * @test >> 37: * @bug 8316497 >> 38: * @summary Verifies Color filter on Non ICC profile > > Suggestion: > > * @summary Verifies Color filter on non-ICC profile Let's bring back this change: _non-ICC_ instead of _Non ICC_. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1453500039
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v15]
On Tue, 16 Jan 2024 04:21:48 GMT, Renjith Kannath Pariyangad 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: > > Updated the code with latest suggestion Looks good to me, except for a minor nit. - PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1823557429
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v15]
> 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: Updated the code with latest suggestion - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/bec09a31..895de12f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=14 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=13-14 Stats: 10 lines in 1 file changed: 0 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
On Sun, 14 Jan 2024 09:31:29 GMT, Sergey Bylokhov wrote: > > 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. > > That could be fine. We can create a gold image first, then parametrize the > test method by different src/mid/dst and compare the result vs gold, so we > will cover all possible combinations. It tried it… with for-loop over `ColorSpaceSelector.PYCC` and `ColorSpaceSelector.WRAPPED_PYCC` and I didn't like the result. Although I kept all the images inside the loop to be safe. The approach above repeats several lines, however, it's more straightforward and reporting errors is easier: you'll the the line where exception occurred. If it's in the loop, the exception could happen on the same line but with different parameters. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1451798570
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
On Fri, 12 Jan 2024 21:05:19 GMT, Alexey Ivanov wrote: > 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. That could be fine. We can create a gold image first, then parametrize the test method by different src/mid/dst and compare the result vs gold, so we will cover all possible combinations. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1451693480
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
On Fri, 12 Jan 2024 09:53:46 GMT, Renjith Kannath Pariyangad 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 The diffs are rather long above, this is how the `main` method should look like: https://github.com/aivanov-jdk/jdk/blob/e3f9b66089311949d5ff99a77511e309219247ba/test/jdk/java/awt/color/NonICCFilterTest.java#L128-L150 That is a `gold` color converter is created with the plain `ICC_ColorSpace` and it's applied to both images. Next, a `test` color converter is created with the wrapper and again it's applied to both images. - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1889960387
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
On Fri, 12 Jan 2024 09:53:46 GMT, Renjith Kannath Pariyangad
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
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
On Fri, 12 Jan 2024 09:53:46 GMT, Renjith Kannath Pariyangad 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 Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1819028509
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Fri, 12 Jan 2024 02:55:59 GMT, Sergey Bylokhov wrote: >> @mrserb , >> >> Did you mean to use the wrapper for the middle like `ColorSpace mid = >> createCS(ColorSpaceSelector.WRAPPED_PYCC);` , So we can achieve : >> >> **From** >> wrapper->icc_color_space->wrapper >> icc_color_space->icc_color_space->icc_color_space >> >>**To** : >> wrapper->wrapper->wrapper >> icc_color_space->wrapper->icc_color_space >> >> Correct me if I am wrong > >>Did you mean to use the wrapper for the middle like ColorSpace mid = >>createCS(ColorSpaceSelector.WRAPPED_PYCC); , So we can achieve : > > Just repeat existed checks using wrapper. So you will have all combinations: > >> wrapper->icc_color_space->wrapper >> icc_color_space->icc_color_space->icc_color_space >> wrapper->wrapper->wrapper >> icc_color_space->wrapper->icc_color_space @mrserb thank you for clarifying, repeated the check for covering all cases please review. - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1888770449
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v14]
> 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: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/6d47f16b..bec09a31 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=13 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=12-13 Stats: 19 lines in 1 file changed: 16 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Thu, 11 Jan 2024 06:53:22 GMT, Renjith Kannath Pariyangad wrote: >Did you mean to use the wrapper for the middle like ColorSpace mid = >createCS(ColorSpaceSelector.WRAPPED_PYCC); , So we can achieve : Just repeat existed checks using wrapper. So you will have all combinations: > wrapper->icc_color_space->wrapper > icc_color_space->icc_color_space->icc_color_space > wrapper->wrapper->wrapper > icc_color_space->wrapper->icc_color_space - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1888355107
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Wed, 10 Jan 2024 19:50:45 GMT, Sergey Bylokhov wrote: >> Can this patch be covered by the new test? > >>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. > > The current test validates two code paths: > - wrapper>icc_color_space->wrapper > - icc_color_space->icc_color_space->icc_color_space > > The color space in the middle is always icc_color_space: > https://github.com/openjdk/jdk/pull/16895/files#diff-70b19b2642d6d3f44904de8b6eb2993e1c97320e3476898c4372db364c4288b7R130 > > If we will use the wrapper for the middle as well we will cover this code > path: > https://github.com/openjdk/jdk/pull/16895/files#diff-e3d6eea060882cab00827c00e1a83b0e0a5b2a31fa9a9aa2122841bbd57c4a6dL853 @mrserb , Did you mean to use the wrapper for the middle like `ColorSpace mid = createCS(ColorSpaceSelector.WRAPPED_PYCC);` , So we can achieve : **Current** **New** wrapper->icc_color_space->wrapper> wrapper->wrapper->wrapper icc_color_space->icc_color_space->icc_color_space > icc_color_space->wrapper->icc_color_space Correct me if I am wrong - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1886405188
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Thu, 30 Nov 2023 19:20:50 GMT, Sergey Bylokhov 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'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. The current test validates two code paths: - wrapper>icc_color_space->wrapper - icc_color_space->icc_color_space->icc_color_space The color space in the middle is always icc_color_space: https://github.com/openjdk/jdk/pull/16895/files#diff-70b19b2642d6d3f44904de8b6eb2993e1c97320e3476898c4372db364c4288b7R130 If it we cover the wrapper for the middle as well we will cover this code path: https://github.com/openjdk/jdk/pull/16895/files#diff-e3d6eea060882cab00827c00e1a83b0e0a5b2a31fa9a9aa2122841bbd57c4a6dL853 - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1885608696
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v13]
On Tue, 9 Jan 2024 11:11:38 GMT, Renjith Kannath Pariyangad 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: > > Fixed other two typos I merged master and ran the client tests—the tests are green. - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1885305466
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v13]
On Tue, 9 Jan 2024 11:11:38 GMT, Renjith Kannath Pariyangad 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: > > Fixed other two typos Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1811221529
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v13]
> 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: Fixed other two typos - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/1788ef69..6d47f16b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=12 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=11-12 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Fri, 5 Jan 2024 16:16:51 GMT, Alexey Ivanov wrote: >> @mrserb for avoiding _ArrayIndexOutOfBoundsException_ I have modified >> `CS_GRAY `with `CS_LINEAR_RGB ` for making same number of channels. But the >> test passed irrespective of fix. >> Do you find any alternative way to fail the test with out fix ? > >> There are at least two similar copy/paste typos. In the next line >> >> https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L771 >> we use the srcNumComp which is the number of components for the incoming >> source color space, but the loop should be done over iccSrcNumComp which is >> a number of components of the "intermidiate" source color space. Similar >> issue is in the next line but for the destination >> >> https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L785 >> >> It is possible that these compensate/affects each other and produce some >> non-completly broken result. The test from the comment above start to pass >> after these will be fixed. > > Indeed, these should be `iccSrcNumComp` and `iccDstNumComp` according to > number of components in the allocated arrays. Modifying these lines make the > test pass. > >> Since we found a few bugs in this code path, it will be useful to check the >> code path in the nonICCBIFilter where the CSList is not null(the >> intermidiate transform is wrapper as well, as of now our test uses the >> built-in ColorSpace.getInstance(CS_sRGB)) > > Agree, another (unit) test is required which exercises other code paths. These typos also fixed - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1445952849
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Mon, 8 Jan 2024 13:53:21 GMT, Alexey Ivanov wrote: >>> 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. > >> 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). > > @Renjithkannath, you can merge the changes I made into your branch using the > following command: > > > git pull https://github.com/aivanov-jdk/jdk.git > renjith/sergey-8316497-colorConvertOp > > > The command will merge the changes from the remote branch into your currently > checked out branch. @aivanov-jdk Thank you for sharing the updated code I have merged your changes from the branch and pushed in. - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1882425761
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v12]
> 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 three additional commits since the last revision: - Rename compareImages to areImagesEqual compareImages is ambiguous. Does it return true when images equal? The name areImagesEqual clearly conveys the intended return values, one doesn't need to look into its code to understand what's expected. Thus, the return values in the method are reversed, and the condition of the test is negated. - Improvements to Sergey's test Use enum instead of two booleans to select ColorSpace. Create the ColorSpace and pass it to createImage. Use constants to control the size of the image. Print the position of the failed pixel. - 8316497: Sergey's version of the non-ICC test - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/56ce5c5e..1788ef69 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=11 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=10-11 Stats: 60 lines in 1 file changed: 28 ins; 6 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Mon, 8 Jan 2024 13:48:10 GMT, Alexey Ivanov wrote: > 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). @Renjithkannath, you can merge the changes I made into your branch using the following command: git pull https://github.com/aivanov-jdk/jdk.git renjith/sergey-8316497-colorConvertOp The command will merge the changes from the remote branch into your currently checked out branch. - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1881055364
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Thu, 30 Nov 2023 19:20:50 GMT, Sergey Bylokhov 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
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v11]
On Fri, 5 Jan 2024 15:49:56 GMT, Alexey Ivanov 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
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Fri, 5 Jan 2024 13:18:46 GMT, Renjith Kannath Pariyangad wrote: >> There are at least two similar copy/paste typos. >> In the next line >> https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L771 >> we use the srcNumComp which is the number of components for the incoming >> source color space, but the loop should be done over iccSrcNumComp which is >> a number of components of the "intermidiate" source color space. Similar >> issue is in the next line but for the destination >> https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L785 >> >> It is possible that these compensate/affects each other and produce some >> non-completly broken result. >> The test from the comment above start to pass after these will be fixed. >> >> Since we found a few bugs in this code path, it will be useful to check the >> code path in the nonICCBIFilter where the CSList is not null(the >> intermidiate transform is wrapper as well, as of now our test uses the >> built-in ColorSpace.getInstance(CS_sRGB)) > > @mrserb for avoiding _ArrayIndexOutOfBoundsException_ I have modified > `CS_GRAY `with `CS_LINEAR_RGB ` for making same number of channels. But the > test passed irrespective of fix. > Do you find any alternative way to fail the test with out fix ? > There are at least two similar copy/paste typos. In the next line > > https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L771 > we use the srcNumComp which is the number of components for the incoming > source color space, but the loop should be done over iccSrcNumComp which is a > number of components of the "intermidiate" source color space. Similar issue > is in the next line but for the destination > > https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L785 > > It is possible that these compensate/affects each other and produce some > non-completly broken result. The test from the comment above start to pass > after these will be fixed. Indeed, these should be `iccSrcNumComp` and `iccDstNumComp` according to number of components in the allocated arrays. Modifying these lines make the test pass. > Since we found a few bugs in this code path, it will be useful to check the > code path in the nonICCBIFilter where the CSList is not null(the intermidiate > transform is wrapper as well, as of now our test uses the built-in > ColorSpace.getInstance(CS_sRGB)) Agree, another (unit) test is required which exercises other code paths. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1443068123
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v11]
On Fri, 5 Jan 2024 12:03:43 GMT, Renjith Kannath Pariyangad 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 Based on @mrserb observations, additional changes are required. - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1806290196
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v11]
On Fri, 5 Jan 2024 12:03:43 GMT, Renjith Kannath Pariyangad 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
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Fri, 5 Jan 2024 03:32:10 GMT, Sergey Bylokhov wrote: >>> Still, the current test catches the typo in the code srcColorSpace → >>> dstColorSpace that's now replaced. From this point view, the stated bug is >>> fixed. >> >> But the implementation of TestColorSpacewe is wrong, and it may not work as >> expected after any other future updates. > > There are at least two similar copy/paste typos. > In the next line > https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L771 > we use the srcNumComp which is the number of components for the incoming > source color space, but the loop should be done over iccSrcNumComp which is a > number of components of the "intermidiate" source color space. Similar issue > is in the next line but for the destination > https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L785 > > It is possible that these compensate/affects each other and produce some > non-completly broken result. > The test from the comment above start to pass after these will be fixed. > > Since we found a few bugs in this code path, it will be useful to check the > code path in the nonICCBIFilter where the CSList is not null(the intermidiate > transform is wrapper as well, as of now our test uses the built-in > ColorSpace.getInstance(CS_sRGB)) @mrserb for avoiding _ArrayIndexOutOfBoundsException_ I have modified `CS_GRAY `with `CS_LINEAR_RGB ` for making same number of channels. But the test passed irrespective of fix. Do you find any alternative way to fail the test with out fix ? - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442869387
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v11]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/f6a435bc..56ce5c5e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=10 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=09-10 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 18:58:02 GMT, Sergey Bylokhov wrote: >>> > I thought about it… I thought that the filter itself should exercise the >>> > code path that's modified but it doesn't seem like it does… I mean >>> > creating images with regular ICC profile and using the wrapper >>> > TestColorSpace in conversion: new ColorConvertOp(new >>> > TestColorSpace(ColorSpace.getInstance(CS_LINEAR_RGB), null); but it >>> > didn't work, the test passes without the fix. >>> >>> It is passed since it blindly checks the difference between src and dst, >>> and since with proper implementation of TestColorSpacewe we always do >>> "some" conversion -> the images are different -> the test passed. >> >> Still, the current test catches the typo in the code `srcColorSpace` → >> `dstColorSpace` that's now replaced. From this point view, the stated bug is >> fixed. >> >> At the same time, I agree there seem to be more bugs in the area. > >> Still, the current test catches the typo in the code srcColorSpace → >> dstColorSpace that's now replaced. From this point view, the stated bug is >> fixed. > > But the implementation of TestColorSpacewe is wrong, and it may not work as > expected after any other future updates. There are at least two similar copy/paste typos. In the next line https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L771 we use the srcNumComp which is the number of components for the incoming source color space, but the loop should be done over iccSrcNumComp which is a number of components of the "intermidiate" source color space. Similar issue is in the next line but for the destination https://github.com/openjdk/jdk/blob/f73dbb985823d2d59bfcff8ba6951a0d9eb2cefc/src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java#L785 It is possible that these compensate/affects each other and produce some non-completly broken result. The test from the comment above start to pass after these will be fixed. Since we found a few bugs in this code path, it will be useful to check the code path in the nonICCBIFilter where the CSList is not null(the intermidiate transform is wrapper as well, as of now our test uses the built-in ColorSpace.getInstance(CS_sRGB)) - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442465871
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 18:37:54 GMT, Alexey Ivanov wrote: > Still, the current test catches the typo in the code srcColorSpace → > dstColorSpace that's now replaced. From this point view, the stated bug is > fixed. But the implementation of TestColorSpacewe is wrong, and it may not work as expected after any other future updates. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442142789
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 18:04:59 GMT, Sergey Bylokhov wrote: > > I thought about it… I thought that the filter itself should exercise the > > code path that's modified but it doesn't seem like it does… I mean creating > > images with regular ICC profile and using the wrapper TestColorSpace in > > conversion: new ColorConvertOp(new > > TestColorSpace(ColorSpace.getInstance(CS_LINEAR_RGB), null); but it didn't > > work, the test passes without the fix. > > It is passed since it blindly checks the difference between src and dst, and > since with proper implementation of TestColorSpacewe we always do "some" > conversion -> the images are different -> the test passed. Still, the current test catches the typo in the code `srcColorSpace` → `dstColorSpace` that's now replaced. From this point view, the stated bug is fixed. At the same time, I agree there seem to be more bugs in the area. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442124080
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 17:54:10 GMT, Sergey Bylokhov wrote: >> @mrserb With your code above, I get `ArrayIndexOutOfBoundsException`. >> >> Do you mean to say there are two additional bugs? >> >> 1. `ArrayIndexOutOfBoundsException` is thrown; >> 2. Even regular ICC_Profile result in unexpected behaviour. > > The ArrayIndexOutOfBoundsException is the bug we are working on in this PR. > The "unexpected behavior" we get when we use the TestColorSpace instead of > direct usage of built-in profiles - it looks like the codepath we updated has > some other bug that affects the rendering results. >I thought about it… I thought that the filter itself should exercise the code >path that's modified but it doesn't seem like it does… I mean creating images >with regular ICC profile and using the wrapper TestColorSpace in conversion: >new ColorConvertOp(new TestColorSpace(ColorSpace.getInstance(CS_LINEAR_RGB), >null); but it didn't work, the test passes without the fix. It is passed since it blindly checks the difference between src and dst, and since with proper implementation of TestColorSpacewe we always do "some" conversion -> the images are different -> the test passed. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442096879
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 17:04:01 GMT, Alexey Ivanov wrote: >> Your `TestColorSpace` class is a wrapper on top of the actual color space. >> So you can compare the results of conversion using `TestColorSpace` vs using >> "actual color space" directly. > > @mrserb With your code above, I get `ArrayIndexOutOfBoundsException`. > > Do you mean to say there are two additional bugs? > > 1. `ArrayIndexOutOfBoundsException` is thrown; > 2. Even regular ICC_Profile result in unexpected behaviour. The ArrayIndexOutOfBoundsException is the bug we are working on in this PR. The "unexpected behavior" we get when we use the TestColorSpace instead of direct usage of built-in profiles - it looks like the codepath we updated has some other bug that affects the rendering results. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442087244
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v10]
On Thu, 4 Jan 2024 13:05:42 GMT, Renjith Kannath Pariyangad
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:
>
> Copyright year updated
It looks good to me.
The current test fails without the fix in `ColorConvertOp` and passes with the
fix. Thus, the current bug is fixed and the regression test does its job.
It's good that further bugs have been identified, these are to be submitted and
then fixed. When the bugs are fixed, the test may be modified to test other
conditions, provided it still verifies this changeset.
test/jdk/java/awt/color/NonICCFilterTest.java line 108:
> 106: public static void main(String[] args) {
> 107:
> 108: BufferedImage src = createTestImage(true);
Suggestion:
public static void main(String[] args) {
BufferedImage src = createTestImage(true);
Remove the blank line at the start of the `main` method.
-
Marked as reviewed by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1804631569
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442041375
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 11:53:19 GMT, Renjith Kannath Pariyangad wrote: >> Your `TestColorSpace` class is a wrapper on top of the actual color space. >> So you can compare the results of conversion using `TestColorSpace` vs using >> "actual color space" directly. > > Thank you for sharing the detailed code, tried the above sample but its like > Aleksei mentioned above _always passing irrespective of fix_. The fix area > will pass only when non-ICC src and dst. Some of the color related function > internally applying icc profile or other propertied and pass through other > implementation. Not sure any other way to overcome this. I thought about it… I thought that the filter itself should exercise the code path that's modified but it doesn't seem like it does… I mean creating images with regular ICC profile and using the wrapper `TestColorSpace` in conversion: `new ColorConvertOp(new TestColorSpace(ColorSpace.getInstance(CS_LINEAR_RGB), null);` but it didn't work, the test passes without the fix. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442035800
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 06:14:49 GMT, Sergey Bylokhov wrote: >> @mrserb, Thank you for bringing this up. Sorry I am not pretty clear about >> your suggestion on _Conversion via TestColorSpace wrapper vs ColorSpace w/o >> wrapper._ . In existing code `compareImages` (pixel value compare) function >> is local, are you suggesting to move this into wrapper? > > Your `TestColorSpace` class is a wrapper on top of the actual color space. So > you can compare the results of conversion using `TestColorSpace` vs using > "actual color space" directly. @mrserb With your code above, I get `ArrayIndexOutOfBoundsException`. Do you mean to say there are two additional bugs? 1. `ArrayIndexOutOfBoundsException` is thrown; 2. Even regular ICC_Profile result in unexpected behaviour. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1442039232
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v10]
> 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: Copyright year updated - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/dbfadbe4..f6a435bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=09 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 06:14:49 GMT, Sergey Bylokhov wrote: >> @mrserb, Thank you for bringing this up. Sorry I am not pretty clear about >> your suggestion on _Conversion via TestColorSpace wrapper vs ColorSpace w/o >> wrapper._ . In existing code `compareImages` (pixel value compare) function >> is local, are you suggesting to move this into wrapper? > > Your `TestColorSpace` class is a wrapper on top of the actual color space. So > you can compare the results of conversion using `TestColorSpace` vs using > "actual color space" directly. Thank you for sharing the detailed code, tried the above sample but its like Aleksei mentioned above _always passing irrespective of fix_. The fix area will pass only when non-ICC src and dst. Some of the color related function internally applying icc profile or other propertied and pass through other implementation. Not sure any other way to overcome this. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1441659467
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v9]
> 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 csRGB conversion call - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/9fc84de6..dbfadbe4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=08 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=07-08 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 06:08:00 GMT, Renjith Kannath Pariyangad
wrote:
>> Something like this:
>>
>> /*
>> * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> *
>> * This code is free software; you can redistribute it and/or modify it
>> * under the terms of the GNU General Public License version 2 only, as
>> * published by the Free Software Foundation.
>> *
>> * This code is distributed in the hope that it will be useful, but WITHOUT
>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>> * version 2 for more details (a copy is included in the LICENSE file that
>> * accompanied this code).
>> *
>> * You should have received a copy of the GNU General Public License version
>> * 2 along with this work; if not, write to the Free Software Foundation,
>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>> *
>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>> * or visit www.oracle.com if you need additional information or have any
>> * questions.
>> */
>>
>> import java.awt.Color;
>> import java.awt.GradientPaint;
>> import java.awt.Graphics2D;
>> import java.awt.Transparency;
>> import java.awt.color.ColorSpace;
>> import java.awt.image.BufferedImage;
>> import java.awt.image.ColorConvertOp;
>> import java.awt.image.ComponentColorModel;
>> import java.awt.image.DataBuffer;
>> import java.awt.image.WritableRaster;
>>
>> /*
>> * @test
>> * @bug 8316497
>> * @summary Verifies Color filter on Non ICC profile
>> */
>> public final class NonICCFilterTest {
>>
>> private static final class TestColorSpace extends ColorSpace {
>>
>> private final ColorSpace cs;
>>
>> TestColorSpace(ColorSpace cs) {
>> super(cs.getType(), cs.getNumComponents());
>> this.cs = cs;
>> }
>>
>> public float[] toRGB(float[] colorvalue) {
>> return cs.toRGB(colorvalue);
>> }
>>
>> public float[] fromRGB(float[] rgbvalue) {
>> return cs.fromRGB(rgbvalue);
>> }
>>
>> public float[] toCIEXYZ(float[] colorvalue) {
>> return cs.toCIEXYZ(colorvalue);
>> }
>>
>> public float[] fromCIEXYZ(float[] xyzvalue) {
>> return cs.fromCIEXYZ(xyzvalue);
>> }
>> }
>>
>> private static BufferedImage createTestImage(boolean isSrc, boolean
>> plain) {
>> ColorSpace cs = createCS(isSrc, plain);
>> ComponentColorM...
>
> @mrserb, Thank you for bringing this up. Sorry I am not pretty clear about
> your suggestion on _Conversion via TestColorSpace wrapper vs ColorSpace w/o
> wrapper._ . In existing code `compareImages` (pixel value compare) function
> is local, are you suggesting to move this into wrapper?
Your `TestColorSpace` class is a wrapper on top of the actual color space. So
you can compare the results of conversion using `TestColorSpace` vs using
"actual color space" directly.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1441377540
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 05:53:44 GMT, Sergey Bylokhov wrote:
>> and instead of comparing that the images are just different, it is probably
>> better to compare results of conversion via TestColorSpace wrapper vs
>> ColorSpace w/o wrapper.
>
> Something like this:
>
> /*
> * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> * under the terms of the GNU General Public License version 2 only, as
> * published by the Free Software Foundation.
> *
> * This code is distributed in the hope that it will be useful, but WITHOUT
> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> * version 2 for more details (a copy is included in the LICENSE file that
> * accompanied this code).
> *
> * You should have received a copy of the GNU General Public License version
> * 2 along with this work; if not, write to the Free Software Foundation,
> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> *
> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
> * or visit www.oracle.com if you need additional information or have any
> * questions.
> */
>
> import java.awt.Color;
> import java.awt.GradientPaint;
> import java.awt.Graphics2D;
> import java.awt.Transparency;
> import java.awt.color.ColorSpace;
> import java.awt.image.BufferedImage;
> import java.awt.image.ColorConvertOp;
> import java.awt.image.ComponentColorModel;
> import java.awt.image.DataBuffer;
> import java.awt.image.WritableRaster;
>
> /*
> * @test
> * @bug 8316497
> * @summary Verifies Color filter on Non ICC profile
> */
> public final class NonICCFilterTest {
>
> private static final class TestColorSpace extends ColorSpace {
>
> private final ColorSpace cs;
>
> TestColorSpace(ColorSpace cs) {
> super(cs.getType(), cs.getNumComponents());
> this.cs = cs;
> }
>
> public float[] toRGB(float[] colorvalue) {
> return cs.toRGB(colorvalue);
> }
>
> public float[] fromRGB(float[] rgbvalue) {
> return cs.fromRGB(rgbvalue);
> }
>
> public float[] toCIEXYZ(float[] colorvalue) {
> return cs.toCIEXYZ(colorvalue);
> }
>
> public float[] fromCIEXYZ(float[] xyzvalue) {
> return cs.fromCIEXYZ(xyzvalue);
> }
> }
>
> private static BufferedImage createTestImage(boolean isSrc, boolean
> plain) {
> ColorSpace cs = createCS(isSrc, plain);
> ComponentColorModel cm = new ComponentColorModel(cs, false, false,
> Transparency.OPAQUE, DataBuffer.TYPE_BYTE);
> WritableRaster raster = ...
@mrserb, Thank you for bringing this up. Sorry I am not pretty clear about your
suggestion on _Conversion via TestColorSpace wrapper vs ColorSpace w/o
wrapper._ . In existing code `compareImages` (pixel value compare) function is
local, are you suggesting to move this into wrapper?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1441374248
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 05:15:05 GMT, Sergey Bylokhov wrote:
>> The difference between srcColorSpace.fromCIEXYZ and dtsColorSpace.fromCIEXYZ
>> can trigger an exception if both have different number of components, for
>> example srcColorSpace = CS_GRAY and srcColorSpace.CS_sRGB
>> =>ArrayIndexOutOfBoundsException will be triggered.
>>
>>
>> private static TestColorSpace createCS(boolean isSrc) {
>> ColorSpace cs = ColorSpace.getInstance(
>> isSrc ? ColorSpace.CS_GRAY : ColorSpace.CS_sRGB);
>> return new TestColorSpace(cs);
>> }
>>
>> public static void main(String[] args) {
>> BufferedImage src = createTestImage(true);
>> BufferedImage dest = createTestImage(false);
>>
>> ColorSpace mid = ColorSpace.getInstance(ColorSpace.CS_sRGB);
>> ColorConvertOp test = new ColorConvertOp(mid, null);
>> test.filter(src, dest);
>> }
>>
>> 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:97)
>
> and instead of comparing that the images are just different, it is probably
> better to compare results of conversion via TestColorSpace wrapper vs
> ColorSpace w/o wrapper.
Something like this:
/*
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
import java.awt.Color;
import java.awt.GradientPaint;
import java.awt.Graphics2D;
import java.awt.Transparency;
import java.awt.color.ColorSpace;
import java.awt.image.BufferedImage;
import java.awt.image.ColorConvertOp;
import java.awt.image.ComponentColorModel;
import java.awt.image.DataBuffer;
import java.awt.image.WritableRaster;
/*
* @test
* @bug 8316497
* @summary Verifies Color filter on Non ICC profile
*/
public final class NonICCFilterTest_123 {
private static final class TestColorSpace extends ColorSpace {
private final ColorSpace cs;
TestColorSpace(ColorSpace cs) {
super(cs.getType(), cs.getNumComponents());
this.cs = cs;
}
public float[] toRGB(float[] colorvalue) {
return cs.toRGB(colorvalue);
}
public float[] fromRGB(float[] rgbvalue) {
return cs.fromRGB(rgbvalue);
}
public float[] toCIEXYZ(float[] colorvalue) {
return cs.toCIEXYZ(colorvalue);
}
public float[] fromCIEXYZ(float[] xyzvalue) {
return cs.fromCIEXYZ(xyzvalue);
}
}
private static BufferedImage createTestImage(boolean isSrc, boolean plain) {
ColorSpace cs = createCS(isSrc, plain);
ComponentColorModel cm = new ComponentColorModel(cs, false, false,
Transparency.OPAQUE, DataBuffer.TYPE_BYTE);
WritableRaster raster = cm.createCompatibleWritableRaster(100, 100);
BufferedImage img = new BufferedImage(cm, raster, false, null);
Graphics2D g = img.createGraphics();
GradientPaint gp = new GradientPaint(0, 0, Color.GREEN,
raster.getWidth(), raster.getHeight(), Color.BLUE);
g.setPaint(gp);
g.fillRect(0, 0, raster.getWidth(), raster.getHeight());
g.dispose();
return img;
}
private static ColorSpace createCS(boolean isSrc, boolean plain) {
ColorSpace cs = ColorSpace.getInstance(
isSrc ? ColorSpace.CS_GRAY : ColorSpace.CS_sRGB);
if (plain) {
return cs;
}
return new TestColorSpace(cs);
}
private static boolean compareImages(BufferedImage destTest, BufferedImage
destGold) {
for (int x = 0; x < destTest.getWidth(); x++) {
for (int y = 0; y < destTest.getHeight(); y++) {
int rgb1 = destTest.getRGB(x, y);
int rgb2 = destGold.getRGB(x, y);
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v8]
> 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 the review suggesion - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/f73dbb98..9fc84de6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=07 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=06-07 Stats: 24 lines in 1 file changed: 10 ins; 2 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 04:42:24 GMT, Renjith Kannath Pariyangad
wrote:
>> Hm… If modify the `toRGB` and `fromRGB` methods to use `csRGB` object, then
>> the test does not fail without the fix.
>>
>> What's more interesting is that the images are equal before `filter` is
>> applied.
>
> @aivanov-jdk Thank you for clarifying, I thought he mean to modify `toCIEXYZ`
> and `fromCIEXYZ`. Yes if we modify `toRGB `and `fromRGB` then it will not
> pass through fixed area, So we may need to skip this.
The difference between srcColorSpace.fromCIEXYZ and dtsColorSpace.fromCIEXYZ
can trigger an exception if both have different number of components, for
example srcColorSpace = CS_GRAY and srcColorSpace.CS_sRGB
=>ArrayIndexOutOfBoundsException will be triggered.
private static TestColorSpace createCS(boolean isSrc) {
ColorSpace cs = ColorSpace.getInstance(
isSrc ? ColorSpace.CS_GRAY : ColorSpace.CS_sRGB);
return new TestColorSpace(cs);
}
public static void main(String[] args) {
BufferedImage src = createTestImage(true);
BufferedImage dest = createTestImage(false);
ColorSpace mid = ColorSpace.getInstance(ColorSpace.CS_sRGB);
ColorConvertOp test = new ColorConvertOp(mid, null);
test.filter(src, dest);
}
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:97)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1441303144
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Thu, 4 Jan 2024 05:10:55 GMT, Sergey Bylokhov wrote:
>> @aivanov-jdk Thank you for clarifying, I thought he mean to modify
>> `toCIEXYZ` and `fromCIEXYZ`. Yes if we modify `toRGB `and `fromRGB` then it
>> will not pass through fixed area, So we may need to skip this.
>
> The difference between srcColorSpace.fromCIEXYZ and dtsColorSpace.fromCIEXYZ
> can trigger an exception if both have different number of components, for
> example srcColorSpace = CS_GRAY and srcColorSpace.CS_sRGB
> =>ArrayIndexOutOfBoundsException will be triggered.
>
>
> private static TestColorSpace createCS(boolean isSrc) {
> ColorSpace cs = ColorSpace.getInstance(
> isSrc ? ColorSpace.CS_GRAY : ColorSpace.CS_sRGB);
> return new TestColorSpace(cs);
> }
>
> public static void main(String[] args) {
> BufferedImage src = createTestImage(true);
> BufferedImage dest = createTestImage(false);
>
> ColorSpace mid = ColorSpace.getInstance(ColorSpace.CS_sRGB);
> ColorConvertOp test = new ColorConvertOp(mid, null);
> test.filter(src, dest);
> }
>
> 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:97)
and instead of comparing that the images are just different, it is probably
better to compare results of conversion via TestColorSpace wrapper vs
ColorSpace w/o wrapper.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1441305407
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Wed, 3 Jan 2024 21:04:50 GMT, Alexey Ivanov wrote: >> I agree with Sergey and I can't see any updates. > > Hm… If modify the `toRGB` and `fromRGB` methods to use `csRGB` object, then > the test does not fail without the fix. > > What's more interesting is that the images are equal before `filter` is > applied. @aivanov-jdk Thank you for clarifying, I thought he mean to modify `toCIEXYZ` and `fromCIEXYZ`. Yes if we modify `toRGB `and `fromRGB` then it will not pass through fixed area, So we may need to skip this. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1441287458
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Wed, 3 Jan 2024 19:54:47 GMT, Alexey Ivanov wrote: >> Updated this > > I agree with Sergey and I can't see any updates. Hm… If modify the `toRGB` and `fromRGB` methods to use `csRGB` object, then the test does not fail without the fix. What's more interesting is that the images are equal before `filter` is applied. - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440948889
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Wed, 3 Jan 2024 03:42:35 GMT, Renjith Kannath Pariyangad
wrote:
>> test/jdk/java/awt/color/NonICCFilterTest.java line 53:
>>
>>> 51:
>>> 52: public float[] toRGB(float[] colorvalue) {
>>> 53: return colorvalue;
>>
>> Just for completeness, it is probably better to use csRGB.toRGB and
>> csRGB.fromRGB in these two methods
>
> Updated this
I agree with Sergey and I can't see any updates.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440877582
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v7]
On Wed, 3 Jan 2024 03:45:06 GMT, Renjith Kannath Pariyangad
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
> two additional commits since the last revision:
>
> - Copyright year updated
> - Review suggestions implemented
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java line 1:
> 1: /*
Please update the copyright year.
test/jdk/java/awt/color/NonICCFilterTest.java line 38:
> 36: * @test
> 37: * @bug 8316497
> 38: * @summary Verifies Color filter on Non ICC profile
Suggestion:
* @summary Verifies Color filter on non-ICC profile
test/jdk/java/awt/color/NonICCFilterTest.java line 51:
> 49: csRGB = ColorSpace.getInstance(bSrc ?
> ColorSpace.CS_LINEAR_RGB :
> 50: ColorSpace.CS_sRGB);
> 51: }
I propose to create a better wrapper:
protected TestColorSpace(ColorSpace csRGB) {
super(csRGB.getType(), csRGB.getNumComponents());
this.csRGB = csRGB;
}
You'll create the wrapper in a cleaner way:
private static ColorSpace createColorSpace(boolean isSrc) {
return new TestColorSpace(ColorSpace.getInstance(isSrc
?
ColorSpace.CS_LINEAR_RGB
: ColorSpace.CS_sRGB));
}
test/jdk/java/awt/color/NonICCFilterTest.java line 53:
> 51: }
> 52:
> 53: public float[] toRGB(float[] colorvalue) {
Please add `@Override` annotations to the methods: `toRGB`, `fromRGB`,
`toCIEXYZ`, `fromCIEXYZ`.
test/jdk/java/awt/color/NonICCFilterTest.java line 55:
> 53: public float[] toRGB(float[] colorvalue) {
> 54: return colorvalue;
> 55: }
Suggestion:
public float[] toRGB(float[] colorvalue) {
return csRGB.toRGB(colorvalue);
}
This is what Sergey means. The comment applies to `fromRGB` method too.
test/jdk/java/awt/color/NonICCFilterTest.java line 101:
> 99: BufferedImage src, dest;
> 100: src = createTestImage(true);
> 101: dest = createTestImage(false);
Declare variables at the same line where you initialise them:
Suggestion:
BufferedImage src = createTestImage(true);
BufferedImage dest = createTestImage(false);
Use `dst` as a more common abbreviation?
-
PR Review: https://git.openjdk.org/jdk/pull/16895#pullrequestreview-1802942699
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440885046
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440842711
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440879982
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440886545
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440881611
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440883343
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Tue, 2 Jan 2024 05:31:17 GMT, Sergey Bylokhov wrote:
>> Renjith Kannath Pariyangad has updated the pull request incrementally with
>> one additional commit since the last revision:
>>
>> Removed unnecessary try block
>
> test/jdk/java/awt/color/NonICCFilterTest.java line 109:
>
>> 107: throw new RuntimeException("Test failed: Source equal to
>> Destination");
>> 108: }
>> 109: }
>
> The "}" is missing in the latest version.
oops, missed while integrating.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440054991
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v7]
> 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 two additional commits since the last revision: - Copyright year updated - Review suggestions implemented - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/fdc1ce8a..f73dbb98 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=06 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=05-06 Stats: 7 lines in 1 file changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Tue, 2 Jan 2024 05:29:35 GMT, Sergey Bylokhov wrote:
>> Renjith Kannath Pariyangad has updated the pull request incrementally with
>> one additional commit since the last revision:
>>
>> Removed unnecessary try block
>
> test/jdk/java/awt/color/NonICCFilterTest.java line 47:
>
>> 45:
>> 46: protected TestColorSpace(boolean bSrc) {
>> 47: super(CS_sRGB, 3);
>
> CS_sRGB is not a constant of the ColorSpace type, the
> ICC_Profile.getInstance(xx).getColorSpaceType() and
> profile.getNumComponents() should be used.
Integrated this, please review
> test/jdk/java/awt/color/NonICCFilterTest.java line 53:
>
>> 51:
>> 52: public float[] toRGB(float[] colorvalue) {
>> 53: return colorvalue;
>
> Just for completeness, it is probably better to use csRGB.toRGB and
> csRGB.fromRGB in these two methods
Updated this
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440043145
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1440043507
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
On Tue, 2 Jan 2024 03:53:06 GMT, Renjith Kannath Pariyangad
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 unnecessary try block
test/jdk/java/awt/color/NonICCFilterTest.java line 47:
> 45:
> 46: protected TestColorSpace(boolean bSrc) {
> 47: super(CS_sRGB, 3);
CS_sRGB is not a constant of the ColorSpace type, the
ICC_Profile.getInstance(xx).getColorSpaceType() and profile.getNumComponents()
should be used.
test/jdk/java/awt/color/NonICCFilterTest.java line 53:
> 51:
> 52: public float[] toRGB(float[] colorvalue) {
> 53: return colorvalue;
Just for completeness, it is probably better to use csRGB.toRGB and
csRGB.fromRGB in these two methods
test/jdk/java/awt/color/NonICCFilterTest.java line 109:
> 107: throw new RuntimeException("Test failed: Source equal to
> Destination");
> 108: }
> 109: }
The "}" is missing in the latest version.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1439162717
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1439163073
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1439163248
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v6]
> 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 unnecessary try block - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/79627652..fdc1ce8a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=05 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v4]
On Tue, 2 Jan 2024 00:33:47 GMT, Sergey Bylokhov wrote:
>> Renjith Kannath Pariyangad has updated the pull request incrementally with
>> one additional commit since the last revision:
>>
>> Removed space
>
> test/jdk/java/awt/color/NonICCFilterTest.java line 106:
>
>> 104: ccop.filter(src, dest);
>> 105:
>> 106: try {
>
> This try block seems unnecessary.
@mrserb, Thank you for your review, I have removed try block.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1439139317
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v5]
> 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 unnecessary try block - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/1bf2cb3b..79627652 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=03-04 Stats: 7 lines in 1 file changed: 0 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v4]
On Fri, 22 Dec 2023 04:03:14 GMT, Renjith Kannath Pariyangad
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 space
test/jdk/java/awt/color/NonICCFilterTest.java line 106:
> 104: ccop.filter(src, dest);
> 105:
> 106: try {
This try block seems unnecessary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1439107074
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v4]
> 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 space - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/71748d39..1bf2cb3b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=02-03 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v3]
On Fri, 22 Dec 2023 03:54:53 GMT, Renjith Kannath Pariyangad 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: > > Integrated suggesions Thank you @turbanoff for your time and review, I have integrated your suggestions - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1867205997
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v3]
> 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: Integrated suggesions - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/b7faa3a7..71748d39 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=01-02 Stats: 9 lines in 1 file changed: 3 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v2]
On Thu, 7 Dec 2023 09:17:05 GMT, Renjith Kannath Pariyangad 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: > > Added test case test/jdk/java/awt/color/NonICCFilterTest.java line 79: > 77: raster.getWidth(), raster.getHeight(), Color.BLUE); > 78: g.setPaint(gp); > 79: g.fillRect(0,0,raster.getWidth(), raster.getHeight()); Suggestion: g.fillRect(0, 0, raster.getWidth(), raster.getHeight()); - PR Review Comment: https://git.openjdk.org/jdk/pull/16895#discussion_r1434524762
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Thu, 30 Nov 2023 19:20:50 GMT, Sergey Bylokhov 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? Thank you @mrserb and @aivanov-jdk for your review and suggestions, Successfully made a verification test for this case, please review and let me know your suggestions including test name and location. Regards. Renjith - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1844973349
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix [v2]
> 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: Added test case - Changes: - all: https://git.openjdk.org/jdk/pull/16895/files - new: https://git.openjdk.org/jdk/pull/16895/files/6910d9c6..b7faa3a7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16895=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16895=00-01 Stats: 115 lines in 1 file changed: 115 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Thu, 30 Nov 2023 03:34:08 GMT, Renjith Kannath Pariyangad 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. The fix looks good to me. I agree with @mrserb, it would be good to create a test case. I don't have much experience with color transforms, so I don't have any pointers as to how to create one. If it takes a considerable amount of time, I guess it's fine to go without the test. - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1841358386
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Thu, 30 Nov 2023 19:20:50 GMT, Sergey Bylokhov 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? @mrserb , Thanks for the review, Could you elaborate little more or share your thought? As per my understanding this fix will impact only on Non ICC color profile workflow. Tried bit for making a new test but as of now its not success. There is no samples/test case available with Non ICC profile your guidance will help here. - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1841049605
Re: RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
On Thu, 30 Nov 2023 03:34:08 GMT, Renjith Kannath Pariyangad 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? - PR Comment: https://git.openjdk.org/jdk/pull/16895#issuecomment-1834408439
RFR: 8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix
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. - Commit messages: - JDK-8316497 : ColorConvertOp - typo for non-ICC conversions needs one-line fix Changes: https://git.openjdk.org/jdk/pull/16895/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16895=00 Issue: https://bugs.openjdk.org/browse/JDK-8316497 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16895.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16895/head:pull/16895 PR: https://git.openjdk.org/jdk/pull/16895
