Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% #246

Closed
wants to merge 7 commits into from

Conversation

@Schmidor
Copy link
Contributor

Schmidor commented Jun 3, 2020

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Prasanta Sadhukhan (psadhukhan - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/246/head:pull/246
$ git checkout pull/246

… EmbeddedScene#getPixels does

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.
@bridgekeeper bridgekeeper bot added the oca label Jun 3, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 3, 2020

Hi @Schmidor, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Schmidor" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@Schmidor Schmidor changed the title 8220484: calculate buffer size of JFXPanel#createResizePixelBuffer as EmbeddedScene#getPixels does 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% Jun 3, 2020
@Schmidor
Copy link
Contributor Author

Schmidor commented Jun 3, 2020

/issue 8230007

@openjdk
Copy link

openjdk bot commented Jun 3, 2020

@Schmidor
Adding additional issue to issue list: 8230007: HiDPI scaling (125%) makes WebView in JFXPanel have a skewed/tilted rendering.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 3, 2020

@Schmidor in general, we don't use the /issue command to identify other issues that happen to be fixed by a commit. Rather, if they are the same bug, or if one is caused by the other, we would closed the other one as a duplicate. This seems like a good thing for me to add to the CONTRIBUTING guidelines.

@bridgekeeper bridgekeeper bot removed the oca label Jun 12, 2020
@openjdk openjdk bot added the rfr label Jun 12, 2020
@mlbridge
Copy link

mlbridge bot commented Jun 12, 2020

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 12, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jun 12, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 12, 2020

/issue remove 8230007

@openjdk
Copy link

openjdk bot commented Jun 12, 2020

@kevinrushforth Only the author (@Schmidor) is allowed to issue the /issue command.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 12, 2020

Even though this is a seemingly simple fix, I'd like @prsadhuk to also review it.

@Schmidor Can you do the following?

  1. Provide an evaluation as to why this is the correct fix, and indicate what testing you have done to ensure no regressions in behavior. Changes to rounding in width and height computations tend to be trickier than they might seem.
  2. Provide a test as part of this fix. Ideally, this would be an automated test, in the system tests project (under tests/system), and should set the glass.win.uiScale system property to 1.25 before running the test. There are a few examples you can follow under tests/system that create a JFrame with a JFXPanel and use Robot to verify the rendered image. If it turns out that an automated is not feasible, a manual test would be an acceptable alternative.
  3. Enter the /issue remove 8230007 command to remove that issue from this PR. I've already closed JDK-8230007 in JBS as a duplicate.
@Schmidor
Copy link
Contributor Author

Schmidor commented Jun 12, 2020

/issue remove 8230007

@openjdk
Copy link

openjdk bot commented Jun 12, 2020

@Schmidor
Removing additional issue from issue list: 8230007.

@Schmidor
Copy link
Contributor Author

Schmidor commented Jun 12, 2020

I'll look into a more detailed description with code references and try to create a test.

The backbuffer of the Stage is created with Math.round(baseSize*uiScale) and for Swing the DataBuffer is referenced into an BufferdImage, where the bounds are calculated with Math.ceil(baseSize*uiScale). So when the base-width is 101px and uiscale is 1.25, the unrounded size is 126.25. The real buffer-width is then 126px, but rendered in swing using 127px. So there is an offset of one pixel per line and the resulting image is skewed.

Copy link
Collaborator

prsadhuk left a comment

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.

@Schmidor
Copy link
Contributor Author

Schmidor commented Jun 30, 2020

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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 2, 2020

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.

Copy link
Member

kevinrushforth left a comment

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).

@openjdk openjdk bot removed the rfr label Jul 3, 2020
@Schmidor Schmidor force-pushed the Schmidor:JDK-8220484 branch from 555c5f3 to 0960a38 Jul 6, 2020
@Schmidor Schmidor force-pushed the Schmidor:JDK-8220484 branch from 0960a38 to 593fe75 Jul 6, 2020
@openjdk openjdk bot added rfr and removed rfr labels Jul 6, 2020
@Schmidor Schmidor force-pushed the Schmidor:JDK-8220484 branch from c8d72e3 to 882f95c Jul 6, 2020
@openjdk openjdk bot added the rfr label Jul 6, 2020
Copy link
Member

kevinrushforth left a comment

The test is looking better now. And the fix to FXPanel looks correct as well, although needs to be tested.

I left a few comments relating to the tests. I haven't looked at the SWT test in detail, but will do that later. I also still need to test this on multiple platforms (I have a concern about platforms other than Windows due to assumptions the test is making).

@@ -3597,9 +3607,11 @@ project(":systemTests") {
testCompile project(":base").sourceSets.test.output
testCompile project(":controls").sourceSets.test.output
testCompile project(":swing").sourceSets.test.output
testCompile project(":swt")
testCompile name: SWT_FILE_NAME

This comment has been minimized.

@kevinrushforth

kevinrushforth Jul 6, 2020 Member

We can't have a dependency on swt from any other project. SWT tests need to be confined to modules/javafx.swt/src/test/java/

This comment has been minimized.

@kevinrushforth

kevinrushforth Jul 6, 2020 Member

I note that this does work for me. So copying the pattern used by systemTests into the swt project might be all that is needed to get it to work there.

This comment has been minimized.

@Schmidor

Schmidor Jul 7, 2020 Author Contributor

I assumed I should add it to the robot package, as it is also a visual test. I've moved it to the swt module

This comment has been minimized.

@kevinrushforth

kevinrushforth Jul 8, 2020 Member

I verified that the SWT test works well, when manually enabled in build.gradle and if it runs in isolation. As for putting it in a robot sub-package, that seems helpful, but since it won't be enabled anyway, is optional.

We can file a follow-up issue to enable SWT tests again (as long as they aren't enabled by default. My thought is to change the default value of SWT_TEST to false, and then enable the tests on platforms other than macOS when the SWT_TEST is set to true along with FULL_TEST).

build.gradle Outdated Show resolved Hide resolved
@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 6, 2020

Also, in general we recommend not to force-push to your branch after the review has started, unless there is some compelling reason to do so. It makes it harder for reviewers to look at incremental diffs.

Copy link
Member

kevinrushforth left a comment

The test looks good with one minor typo in the constant field name (see below). I verified that it fails on Linux and Mac (as I suspected), so you will need to limit the test to running on Windows.

Schmidor added 2 commits Jul 20, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 20, 2020

I think this is ready to target to jfx15, so go ahead and do that. I'll finish my review after that.

@Schmidor Schmidor changed the base branch from master to jfx15 Jul 21, 2020
Copy link
Member

kevinrushforth left a comment

Looks good.

@prsadhuk can you also review this?

@openjdk
Copy link

openjdk bot commented Jul 22, 2020

@Schmidor This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%

Reviewed-by: kcr, psadhukhan
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 41 commits pushed to the jfx15 branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge jfx15 into your branch, and then specify the current head hash when integrating, like this: /integrate 5f60ea5da892f7dbf1cb646c27983bb510fb24c6.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @prsadhuk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jul 22, 2020
@Schmidor
Copy link
Contributor Author

Schmidor commented Jul 22, 2020

/integrate

@openjdk openjdk bot added the sponsor label Jul 22, 2020
@openjdk
Copy link

openjdk bot commented Jul 22, 2020

@Schmidor
Your change (at version ea1ff37) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 22, 2020

/sponsor

@openjdk openjdk bot closed this Jul 22, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Jul 22, 2020
@openjdk
Copy link

openjdk bot commented Jul 22, 2020

@kevinrushforth @Schmidor The following commits have been pushed to jfx15 since your change was applied:

  • 5f60ea5: 8248381: Create a daemon thread for MonocleTimer
  • 2f4666a: 8248490: [macOS] Undecorated stage does not minimize
  • 5de99be: Merge
  • e2d1c02: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
  • d67c47f: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread
  • cd57bb5: Merge
  • 126637f: 8201570: Get two bytes for the Linux input event type, not four
  • f3a0446: 8247963: Update SQLite to version 3.32.3
  • 32584db: 8238954: Improve performance of tiled snapshot rendering
  • 869ea40: 8244212: Optionally download media and webkit libraries from latest openjfx EA build
  • 62f8cee: 8247947: Build DirectShow Samples (Base Classes) from source checked into repo
  • 527cc2b: 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine
  • 45c9854: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts
  • 15f97ed: 8240264: iOS: Unnecessary logging on every pulse when GL context changes
  • 2ca509a: 8193800: TreeTableView selection changes on sorting
  • a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
  • 8440b64: 8208169: can not print selected pages of web page
  • 1727dfa: 8247163: JFXPanel throws exception on click when no Scene is set
  • 54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
  • fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
  • bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
  • b200891: 8244824: TableView : Incorrect German translation
  • f6ec5f1: Merge
  • b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
  • afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
  • 5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
  • ba501ef: 8246357: Allow static build of webkit library on linux
  • a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
  • 9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux
  • 6bd0e22: 8239095: Upgrade libFFI to the latest 3.3 version
  • 853ac78: 8245282: Button/Combo Behavior: memory leak on dispose
  • 1663624: Merge
  • 13f42e1: Merge
  • 78bf7b7: 8245422: Better Pisces rasterizing
  • f8cc090: Merge
  • 4597437: Merge
  • f1ac39b: Merge
  • bb5ce2c: Merge
  • d299465: Merge
  • d2346b8: Merge
  • 397587d: 8241108: Glib improvements

Your commit was automatically rebased without conflicts.

Pushed as commit 3cc29e3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.