Re: RFR: 8311492: FontSmoothingType LCD produces wrong color when transparency is used [v2]
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]
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.
>>
>> 
>>
>> 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]
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]
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]
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.
>>
>> 
>>
>> 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]
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. >> >>  >> >> 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]
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]
> 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. > >  > > 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
