On Fri, 15 Mar 2024 11:37:53 GMT, Karthik P K <k...@openjdk.org> wrote:
>> There were two different problems with this test's stability and HiDPI and >> both were fixed with this change. >> >> The whole reason for this tests lack of stability comes with how Windows >> calculates window position when using HiDPI. When window's X/Y coordinates >> are not provided by the application, they are (generally speaking) randomly >> selected by Windows. X/Y are picked as two integers, and afterwards the >> HiDPI scaling is applied. With fractional scaling like 125% this can cause >> X/Y coordinates to have fractional values. >> >> When performing a screen capture, Robot takes a generous estimate of >> window's position and dimensions. This is done by taking provided X/Y >> coordinates (in case of this test, stage's X/Y coords), multiplying them by >> scaling (ex. 1.25) and then flooring the result. Similar estimation is done >> to opposite coordinates - we take X/Y coords, add width/height to them >> respectively, multiply them by scaling and ceiling them. As a result, we >> have minX/Y and maxX/Y coords of capture region, which is used to calculate >> capture region's width and height. The first problem with test's stability >> is right here - the test did not take this calculation and floor/ceil >> operations into account, which with appropriately randomized window >> coordinates could cause discrepancy between expected width/height and >> actual. This logic that is used by `getScreenCapture()` seems to be >> necessary and good, so to remedy the problem I replicated those calculations >> on test side. >> >> Another problem came after that - fractional X/Y values caused capture to >> not always line up with pixel layout, which made the 1-pixel border of >> capture sometimes incorrect (an average of stage's MAGENTA color and >> whatever background happened to be under the displayed stage). This means we >> cannot exactly expect those pixels to have exactly MAGENTA color when the >> system has fractional HiDPI enabled. To help that, the test now doesn't >> check for correctness of the 1-pixel border of captured image. I feel this >> still gives us enough confidence that the Robot screen capture works >> properly, while stabilizing the test on all systems. >> >> I briefly considered an alternative approach to hard-set X/Y values but >> doing it this way gives us a bit more robustness, makes the test independent >> of Stage's X/Y coordinates which matter quite a lot. >> >> Verified this change on macOS and especially on Windows - ran the test on >> Windows 50 times each on 125% and 100% scaling and noticed no intermittent >> failures. > > Tested the changes in Mac, Windows and Linux. The changes fixes the issues in > Mac and Windows. > I ran the test in Linux VM and it failed with and without the fix with error: > > RobotTest > testScreenCapture FAILED > junit.framework.AssertionFailedError: expected: rgba(255,0,255,255) but > was: rgba(0,0,0,255) > at > test.robot.javafx.scene.RobotTest.assertColorEquals(RobotTest.java:848) > at > test.robot.javafx.scene.RobotTest.testScreenCapture(RobotTest.java:774) > > > Changing the scale also did not make any difference. Not sure if it is > related to anything in VM. > It is a Ubuntu 22.04 VM > > Added a minor comment inline. @karthikpandelu I just tried this on Ubuntu 22.04 VM and everything seems to work fine. (0,0,0,255) returned from capture suggests capture failed - could it be your VM is running on Wayland instead of X11? > tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java line 758: > >> 756: // Below calculations follow how getScreenCapture should >> calculate screen capture dimensions. This >> 757: // is to make this code consistent and stable on HiDPI systems. >> 758: int stageX = (int)stage.getX(); > > Minor: Do you think space should be added after `(int)` similar to `(double)`? It would be good for consistent formatting. I'll add those. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1403#issuecomment-2007138448 PR Review Comment: https://git.openjdk.org/jfx/pull/1403#discussion_r1530334956
