Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread Kevin Rushforth
On Fri, 9 Feb 2024 16:02:19 GMT, Andy Goryachev  wrote:

>> Kevin Rushforth has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address review feedback
>
> tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java 
> line 69:
> 
>> 67:  * @bug 8311492
>> 68:  */
>> 69: public class TransparentLCDTest {
> 
> Was there a reason you did not use `VisualTestBase` class?
> The code is fine the way it is, I am just curious.

I started out using `VisualTestBase`, but I needed to set system properties 
before launching the FX runtime, and there is no way that I could find to 
insert any code into the subclass that would get executed before  the 
superclass.

The only thing I ended up using from `VisualTestBase` was the 
`assertColorEquals` method, which could be moved to Util anyway. There is other 
cleanup that could be done to `VisualTestBase`, so I'll file a follow-on bug.

> tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java 
> line 190:
> 
>> 188: @AfterEach
>> 189: public void doTeardown() {
>> 190: Platform.runLater(() -> {
> 
> should this be Util.runAndWait() ?

It doesn't really matter in this case since Runnables are run in order, meaning 
that the creation and showing of the stage for the next text is guaranteed to 
happen after the stage is hidden. I can change it, though.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484526979
PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484529935


Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread Andy Goryachev
On Fri, 9 Feb 2024 14:29:18 GMT, Kevin Rushforth  wrote:

>> JavaFX LCD text rendering (aka sub-pixel antialiasing) uses a pixel shader 
>> and alpha blending. The alpha channel is used is ways that interfere with 
>> its use for transparency. The existing logic checks that the current blend 
>> equation is SRC_OVER and that the surface is opaque, and that we are 
>> rendering using a Paint of type Color. It fails to check that the text color 
>> is opaque. When it isn't, the resulting alpha value is not preserved, even 
>> in the middle of the filled portion of the text, resulting in a visually 
>> noticeable difference in color.
>> 
>> ![transparent-lcd-text](https://github.com/openjdk/jfx/assets/34689748/81f9503c-c706-44d8-b4db-6b47f0eb5fea)
>> 
>> The solution is to add the missing check for alpha == 1 to the test that 
>> checks whether we can use LCD text rendering. I note that Java2D falls back 
>> to gray scale when the text color is transparent for a similar reason.
>> 
>> I added a robot test that checks the color in the middle of the filled 
>> portion of a rendered text character and also checks that we use LCD for 
>> opaque colors and GRAY scale for transparent colors.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review feedback

looks good, some minor comments.

tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java line 
69:

> 67:  * @bug 8311492
> 68:  */
> 69: public class TransparentLCDTest {

Was there a reason you did not use `VisualTestBase` class?
The code is fine the way it is, I am just curious.

-

PR Review: https://git.openjdk.org/jfx/pull/1361#pullrequestreview-1872732167
PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484517970


Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread Andy Goryachev
On Fri, 9 Feb 2024 15:58:12 GMT, Kevin Rushforth  wrote:

>> tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java 
>> line 81:
>> 
>>> 79: // color, or in the middle of the text fill area, where we expect 
>>> to find
>>> 80: // the unadjusted text color.
>>> 81: private static final double TOLERANCE = 2.0 / 255.0;
>> 
>> if the intent is to allow for 2 levels of difference, perhaps it should be 
>> (2.0 / 255) + 0.1 or something
>
> Maybe. Or maybe even `2.5 / 255.0`. I like the latter idea better, so will do 
> that.

yes, much better, thank you.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484514261


Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread Kevin Rushforth
On Fri, 9 Feb 2024 15:53:20 GMT, Andy Goryachev  wrote:

>> Kevin Rushforth has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address review feedback
>
> tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java 
> line 67:
> 
>> 65:  *
>> 66:  * @test
>> 67:  * @bug 8311492
> 
> for my education: what is the meaning of these tags?  how are they used?

These are jtreg tags. They aren't used now, since we don't use jtreg to run our 
tests, but might in the future.

> tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java 
> line 81:
> 
>> 79: // color, or in the middle of the text fill area, where we expect to 
>> find
>> 80: // the unadjusted text color.
>> 81: private static final double TOLERANCE = 2.0 / 255.0;
> 
> if the intent is to allow for 2 levels of difference, perhaps it should be 
> (2.0 / 255) + 0.1 or something

Maybe. Or maybe even `2.5 / 255.0`. I like the latter idea better, so will do 
that.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484508446
PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484512704


Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread Andy Goryachev
On Fri, 9 Feb 2024 14:29:18 GMT, Kevin Rushforth  wrote:

>> JavaFX LCD text rendering (aka sub-pixel antialiasing) uses a pixel shader 
>> and alpha blending. The alpha channel is used is ways that interfere with 
>> its use for transparency. The existing logic checks that the current blend 
>> equation is SRC_OVER and that the surface is opaque, and that we are 
>> rendering using a Paint of type Color. It fails to check that the text color 
>> is opaque. When it isn't, the resulting alpha value is not preserved, even 
>> in the middle of the filled portion of the text, resulting in a visually 
>> noticeable difference in color.
>> 
>> ![transparent-lcd-text](https://github.com/openjdk/jfx/assets/34689748/81f9503c-c706-44d8-b4db-6b47f0eb5fea)
>> 
>> The solution is to add the missing check for alpha == 1 to the test that 
>> checks whether we can use LCD text rendering. I note that Java2D falls back 
>> to gray scale when the text color is transparent for a similar reason.
>> 
>> I added a robot test that checks the color in the middle of the filled 
>> portion of a rendered text character and also checks that we use LCD for 
>> opaque colors and GRAY scale for transparent colors.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review feedback

tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java line 
67:

> 65:  *
> 66:  * @test
> 67:  * @bug 8311492

for my education: what is the meaning of these tags?  how are they used?

tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java line 
81:

> 79: // color, or in the middle of the text fill area, where we expect to 
> find
> 80: // the unadjusted text color.
> 81: private static final double TOLERANCE = 2.0 / 255.0;

if the intent is to allow for 2 levels of difference, perhaps it should be (2.0 
/ 255) + 0.1 or something

tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java line 
190:

> 188: @AfterEach
> 189: public void doTeardown() {
> 190: Platform.runLater(() -> {

should this be Util.runAndWait() ?

tests/system/src/test/java/test/robot/javafx/scene/TransparentLCDTest.java line 
272:

> 270: runTest(true);
> 271: }
> 272: 

extra newline?

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484506628
PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484509375
PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484512177
PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484513289


Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread John Hendrikx
On Fri, 9 Feb 2024 14:29:18 GMT, Kevin Rushforth  wrote:

>> JavaFX LCD text rendering (aka sub-pixel antialiasing) uses a pixel shader 
>> and alpha blending. The alpha channel is used is ways that interfere with 
>> its use for transparency. The existing logic checks that the current blend 
>> equation is SRC_OVER and that the surface is opaque, and that we are 
>> rendering using a Paint of type Color. It fails to check that the text color 
>> is opaque. When it isn't, the resulting alpha value is not preserved, even 
>> in the middle of the filled portion of the text, resulting in a visually 
>> noticeable difference in color.
>> 
>> ![transparent-lcd-text](https://github.com/openjdk/jfx/assets/34689748/81f9503c-c706-44d8-b4db-6b47f0eb5fea)
>> 
>> The solution is to add the missing check for alpha == 1 to the test that 
>> checks whether we can use LCD text rendering. I note that Java2D falls back 
>> to gray scale when the text color is transparent for a similar reason.
>> 
>> I added a robot test that checks the color in the middle of the filled 
>> portion of a rendered text character and also checks that we use LCD for 
>> opaque colors and GRAY scale for transparent colors.
>
> Kevin Rushforth has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review feedback

Marked as reviewed by jhendrikx (Committer).

-

PR Review: https://git.openjdk.org/jfx/pull/1361#pullrequestreview-1872674735


Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread Kevin Rushforth
On Fri, 9 Feb 2024 13:09:23 GMT, John Hendrikx  wrote:

>> Kevin Rushforth has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address review feedback
>
> modules/javafx.graphics/src/main/java/com/sun/prism/sw/SWGraphics.java line 
> 664:
> 
>> 662: getRenderTarget().isOpaque() &&
>> 663: (this.paint.getType() == Paint.Type.COLOR) &&
>> 664: ((Color)this.paint).getAlpha() == 1.0f &&
> 
> Could avoid the cast here if you want:
> 
> Suggestion:
> 
> this.paint instanceof Color c && 
> c.getAlpha() == 1.0f &&

I had thought of it earlier, but didn't do it at the time. I made this change 
along with the suggested test changes. Thanks for the suggestions.

-

PR Review Comment: https://git.openjdk.org/jfx/pull/1361#discussion_r1484397314


Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]

2024-02-09 Thread Kevin Rushforth
> JavaFX LCD text rendering (aka sub-pixel antialiasing) uses a pixel shader 
> and alpha blending. The alpha channel is used is ways that interfere with its 
> use for transparency. The existing logic checks that the current blend 
> equation is SRC_OVER and that the surface is opaque, and that we are 
> rendering using a Paint of type Color. It fails to check that the text color 
> is opaque. When it isn't, the resulting alpha value is not preserved, even in 
> the middle of the filled portion of the text, resulting in a visually 
> noticeable difference in color.
> 
> ![transparent-lcd-text](https://github.com/openjdk/jfx/assets/34689748/81f9503c-c706-44d8-b4db-6b47f0eb5fea)
> 
> The solution is to add the missing check for alpha == 1 to the test that 
> checks whether we can use LCD text rendering. I note that Java2D falls back 
> to gray scale when the text color is transparent for a similar reason.
> 
> I added a robot test that checks the color in the middle of the filled 
> portion of a rendered text character and also checks that we use LCD for 
> opaque colors and GRAY scale for transparent colors.

Kevin Rushforth has updated the pull request incrementally with one additional 
commit since the last revision:

  Address review feedback

-

Changes:
  - all: https://git.openjdk.org/jfx/pull/1361/files
  - new: https://git.openjdk.org/jfx/pull/1361/files/25ad1dab..62c768cf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx=1361=01
 - incr: https://webrevs.openjdk.org/?repo=jfx=1361=00-01

  Stats: 19 lines in 2 files changed: 0 ins; 9 del; 10 mod
  Patch: https://git.openjdk.org/jfx/pull/1361.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/1361/head:pull/1361

PR: https://git.openjdk.org/jfx/pull/1361