Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Fri, 3 Jul 2020 00:01:10 GMT, Kevin Rushforth wrote:
>> Oliver Schmidtmer has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Change to Math.ceil and add test
>
> tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line
> 90:
>
>> 89: Field fpixelsIm = JFXPanel.class.getDeclaredField("pixelsIm");
>> 90: fpixelsIm.setAccessible(true);
>> 91: BufferedImage pixelsIm = (BufferedImage)
>> fpixelsIm.get(myApp.jfxPanel);
>
> This isn't the pattern we use to access internal fields of a JavaFX class,
> and won't work.
>
> We typically use the "shim" pattern for such white-box testing. Can you look
> into adding shims to the `javafx.swing`
> module? Many of the other modules already have shims, so you can use that as
> a pattern. It will require adding a
> package-scope `test_getPixelsIm` method to `JFXPanel`. Alternatively, you
> can use AWT `Robot` to read the JFXPanel
> pixels.
I've added a shim. I would like to check the backing image directly.
> tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line
> 103:
>
>> 102: for (int y = 90; y < 115; y++) {
>> 103: if(colorOfDiagonal == pixelsIm.getRGB( x, y )) {
>> 104: fail( "image is skewed" );
>
> Are you sure that an equality test will work on all platforms and
> configurations? We usually use a tolerance when
> comparing colors whose components are other than 0 or 255.
> Somewhat related to this, is the expected value of `181` coming from the
> default value of the button border? It might
> be more robust to fill the Scene with a stroked rectangle whose color you
> control (e.g. set to black). Either that or
> set the color of the button border using an inline CSS style so you aren't
> dependent on the default inherited from the
> `modena.css` style sheet. Minor: add a space after the `if` and remove the
> extra spaces surrounding the function
> arguments `x, y`.
done, a rectangle with a defined background & border color. That is definitely
more stable.
-
PR: https://git.openjdk.java.net/jfx/pull/246
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Tue, 30 Jun 2020 18:28:11 GMT, Oliver Schmidtmer
wrote:
>> In edge cases where monitor scaling of 1.25 or 1.75 is active, Math.ceil and
>> Math.round produce different results and
>> EmbeddedScene#getPixels in JFXPanel#paintComponent causes an off-by-one
>> error on the line width and therefore sheared
>> rendering. The changes were already proposed by the submitter in
>> JBS-8220484.
>>
>> OCA is signed and send.
>
> Oliver Schmidtmer has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Change to Math.ceil and add test
I left specific feedback, mostly on the test, but also one question on the fix.
For the test, make sure you can run it as follows:
gradle --info -PFULL_TEST=true -PUSE_ROBOT=true cleanTest :systemTests:test
--tests JFXPanelScaledTest
(presuming you rename it to `JFXPanelScaledTest`)
It should fail without your fix and pass with your fix.
Currently, it will fail because of the call to `setAccessible` (see inline
comments).
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 2:
> 1: /*
> 2: * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
This should be `Copyright (c) 2020, Oracle` ...
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 39:
> 38: import javax.swing.*;
> 39: import java.awt.*;
> 40: import java.awt.image.BufferedImage;
We generally avoid wildcard imports.
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 49:
> 48:
> 49: public class JDK8220484Test {
> 50: static CountDownLatch launchLatch;
We no longer use the pattern of naming our test classes after the bug ID. I
recommend a descriptive name for the test
class.
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 71:
> 70: try {
> 71: if (!launchLatch.await(5 * TIMEOUT, TimeUnit.MILLISECONDS)) {
> 72: throw new AssertionFailedError("Timeout waiting for
> Application to launch (" + (5 * TIMEOUT) + "
> seconds)");
If you add `throws Exception` to the this setup method, then you can replace
the entire try / catch block with simply:
assertTrue("Timeout waiting for Application to launch",
launchLatch.await(5 * TIMEOUT, TimeUnit.MILLISECONDS));
This is the pattern we use for our newer system tests.
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 90:
> 89: Field fpixelsIm = JFXPanel.class.getDeclaredField("pixelsIm");
> 90: fpixelsIm.setAccessible(true);
> 91: BufferedImage pixelsIm = (BufferedImage)
> fpixelsIm.get(myApp.jfxPanel);
This isn't the pattern we use to access internal fields of a JavaFX class, and
won't work.
We typically use the "shim" pattern for such white-box testing. Can you look
into adding shims to the `javafx.swing`
module? Many of the other modules already have shims, so you can use that as a
pattern. It will require adding a
package-scope `test_getPixelsIm` method to `JFXPanel`.
Alternatively, you can use AWT `Robot` to read the JFXPanel pixels.
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 93:
> 92:
> 93:
> 94: assertEquals(127, pixelsIm.getWidth());
Minor: a single blank line is sufficient.
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 103:
> 102: for (int y = 90; y < 115; y++) {
> 103: if(colorOfDiagonal == pixelsIm.getRGB( x, y )) {
> 104: fail( "image is skewed" );
Are you sure that an equality test will work on all platforms and
configurations? We usually use a tolerance when
comparing colors whose components are other than 0 or 255.
Somewhat related to this, is the expected value of `181` coming from the
default value of the button border? It might
be more robust to fill the Scene with a stroked rectangle whose color you
control (e.g. set to black). Either that or
set the color of the button border using an inline CSS style so you aren't
dependent on the default inherited from the
`modena.css` style sheet.
Minor: add a space after the `if` and remove the extra spaces surrounding the
function arguments `x, y`.
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 104:
> 103: if(colorOfDiagonal == pixelsIm.getRGB( x, y )) {
> 104: fail( "image is skewed" );
> 105: }
Minor: remove the extra spaces surrounding the function argument.
tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 26:
> 25:
> 26: package test.javafx.embed.swing;
> 27:
This test will need to move to the `test.robot.javafx.embed.swing` package
since it relies on reading the actual
rendered pixels.
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/EmbeddedScene.java
line 235:
> 234:
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Tue, 30 Jun 2020 18:21:52 GMT, Oliver Schmidtmer wrote: >> In 2D, we normally use sun.java2d.pipe.Region.clipRound as it also checks >> for -ve/+ve max INTEGER but I guess that is >> internal class to FX so it's ok to use Math.round. Approval pending test >> creation. > > While both might work, as long as there is no mixed usage of round and ceil, > Math.ceil seems to be better. > > I'm not sure if the timed waiting for the resizes is the best way for > ensuring that the buffer is resized to the new > bounds, so I'm open for suggestions. To me at least it seems to create a > reproducible sheared output after the second > resize in the test case and not anymore after changing the calculations to > Math.ceil. This fix might be a candidate for JavaFX 15, so I recommend to _not_ merge the master branch. If we don't spot anything of concern during the review, then we might ask you to retarget your PR to the `jfx15` branch. - PR: https://git.openjdk.java.net/jfx/pull/246
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Wed, 17 Jun 2020 07:32:46 GMT, Prasanta Sadhukhan wrote: >> Oliver Schmidtmer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change to Math.ceil and add test > > In 2D, we normally use sun.java2d.pipe.Region.clipRound as it also checks for > -ve/+ve max INTEGER but I guess that is > internal class to FX so it's ok to use Math.round. Approval pending test > creation. While both might work, as long as there is no mixed usage of round and ceil, Math.ceil seems to be better. I'm not sure if the timed waiting for the resizes is the best way for ensuring that the buffer is resized to the new bounds, so I'm open for suggestions. To me at least it seems to create a reproducible sheared output after the second resize in the test case and not anymore after changing the calculations to Math.ceil. - PR: https://git.openjdk.java.net/jfx/pull/246
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
> In edge cases where monitor scaling of 1.25 or 1.75 is active, Math.ceil and > Math.round produce different results and > EmbeddedScene#getPixels in JFXPanel#paintComponent causes an off-by-one error > on the line width and therefore sheared > rendering. The changes were already proposed by the submitter in JBS-8220484. > > OCA is signed and send. Oliver Schmidtmer has updated the pull request incrementally with one additional commit since the last revision: Change to Math.ceil and add test - Changes: - all: https://git.openjdk.java.net/jfx/pull/246/files - new: https://git.openjdk.java.net/jfx/pull/246/files/5eda49bf..994ca03c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/246/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/246/webrev.00-01 Stats: 161 lines in 3 files changed: 155 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/246.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/246/head:pull/246 PR: https://git.openjdk.java.net/jfx/pull/246
