-
Notifications
You must be signed in to change notification settings - Fork 521
8324941: POC for Headless platform for JavaFX #1836
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back jvos! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
/reviewers 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My testing shows that the headless platform works as expected.
Some general comments:
- All files need a copyright header.
- Very long lines can benefit from a few line breaks.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessApplication.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessView.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessView.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessView.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/NestedRunnableProcessor.java
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindowManager.java
Outdated
Show resolved
Hide resolved
…s/HeadlessRobot.java use switch expression Co-authored-by: mstr2 <[email protected]>
…s/HeadlessRobot.java use switch expressions Co-authored-by: mstr2 <[email protected]>
Thanks for the comments, those make the PR much cleaner! |
|
||
@Override | ||
protected Screen[] staticScreen_getScreens() { | ||
final int screenWidth = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something that can be configured by the developer? E.g. a simple system property with a default value of 1000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question, and I believe it is part of a broader discussion. There are a number of places (e.g. here) where I use hardcoded, default values which should be fine for most usecases I'm aware of. However, I believe there is definitely value in having more configurable values (e.g. in screen dimensions, but also number of screens, default window sizes, multiclick values etc).
The challenge with this is that we need some way to have developers manipulate these values, for example using system properties (as you state as well). This is then adding a feature that doesn't exist yet, so it would require a deeper discussion on how to make the Headless platform configurable. I'm absolutely +1 on that, but I believe it would be easier if we do that as a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest a system property like
-Dheadless.screens="1920x1024"
-Dheadless.screens="1920x1024x16"
to be able to configure the screen resolution and the bit depth (and maybe a scale as well).
I don't know whether supporting multiple screens makes sense here, but perhaps for testing?
And, if the code cannot parse the property it would default to something reasonable like 1280x768 or slightly larger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm absolutely +1 on that, but I believe it would be easier if we do that as a follow-up?
I agree, makes sense. This way, we can also gradually add properties as (if) needed.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessApplication.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessApplication.java
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessApplication.java
Show resolved
Hide resolved
mouseEvent = MouseEvent.DRAG; | ||
} | ||
int modifiers = 0; | ||
view.notifyMouse(mouseEvent, buttonEvent, (int)mouseX-wx, (int)mouseY-wy, (int)mouseX, (int)mouseY, modifiers, false, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the coordinates be rounded instead of floor'ed (cast to int)?
(also in other places below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a logical point, I would say yes. However, if you look at the Robot implementations for e.g. GTK, there are casts to int as well. So in order to be consistent, I think we should cast to int (and floor, indeed).
int wy = activeWindow.getY(); | ||
int repeat = Math.abs(wheelAmt); | ||
for (int i = 0; i < repeat; i++) { | ||
view.notifyScroll((int) mouseX, (int) mouseY, wx, wy, 0, dff, 0, 0, 0, 0, 0, multiplierX, multiplierY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are all parameters correct? modifiers
and lines
, specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit complex to easily determine if those parameters have relevance at all. There are no failing tests due to this, so I see this as a good opportunity to add tests in case issues (unexpected behavior) is detected.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
Outdated
Show resolved
Hide resolved
for (int i = 0; i < height; i++) { | ||
for (int j = 0; j < width; j++) { | ||
int idx = i * width + j; | ||
int fidx = (y + i) * 1000 + x + j; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does 1000 come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the stride used in the ByteBuffer holding the frame. I've moved it to a final int and removed all hard occurrences of 1000 .
int pH = pixels.getHeight(); | ||
int offsetX = this.getX(); | ||
int offsetY = this.getY(); | ||
int stride = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same magic 1000 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the stride used in the ByteBuffer holding the frame. I've moved it to a final int and removed all hard occurrences of 1000 .
} | ||
|
||
void clearRect(int x0, int w0, int y0, int h0) { | ||
int stride = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the stride used in the ByteBuffer holding the frame. I've moved it to a final int and removed all hard occurrences of 1000 .
try { | ||
latch.await(); | ||
} catch (InterruptedException e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we print here or send to Application.reportException(e)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the method has been removed as it wasn't used.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessRobot.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/glass/ui/headless/HeadlessWindow.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one inline comment about the need for copyright headers.
I did a quick first pass, and this looks like a reasonable replacement for the Headless monocle platform to support running headless tests. It has the advantage of not needing to manually add monocle, which is not shipped on desktop platforms.
This is effectively an experimental feature used for testing. Since there is no API, and it is only enabled by setting a (as yet undocumented) system property, it seems OK that it isn't complete and doesn't have an associated CSR.
Longer term, we will want to think about how this might evolve. The first question is: Does this have any use case beyond testing? AWT has a headless mode that developers can use for server-side rendering. We have discussed the possibility of adding a similar feature for JavaFX, although it is less clear how it would be used, since we don't have image APIs that would be expected for such applications.
Even if this remains a testing feature, how should it eventually be configured? Using system properties? Some sort of config file? Something else?
How should this interact with the AWT headless toolkit? Or does it?
This requires the prism SW pipeline. How / where should be document this restriction?
@@ -0,0 +1,216 @@ | |||
package com.sun.glass.ui.headless; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the new files need a standard copyright header.
if (this.screens == null) { | ||
float scaleX = 1.f; | ||
float scaleY = 1.f; | ||
String scale = System.getProperty("glass.gtk.uiScale"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"gtk" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops. I took a similar approach here as the one used in GtkApplication, but it is not in line with my own suggestion to keep configuration out of this PR.
I removed this property check, as I believe configurations (via properties or not) should be discussed globally, and having one property already being used in the code, would make that discussion harder.
Good question, and the answer is "Yes, absolutely". One of the main drivers for this was a project where PDF prints need to be generated server-side, starting from |
Did you happen to see my proposal to use the software renderer to provide a Canvas#GraphicsContext like API for writable images? Snapshot is a nice feature, but it feels like a rather roundabout way to first use the GPU to draw things, then copy it back to main memory for pixel manipulation or say PDF creation... See here: https://www.mail-archive.com/[email protected]/msg20618.html I'm curious what you think of the proposal (and I'm curious if the headless platform would need to support it or interfere with it :)) |
Hi @craigraw, thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 craigraw" for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
Good question. |
… now. Especially, don't use properties prefixed for GTK.
I see the benefits from your proposal, and I believe it makes sense. I don't think it interferes with the headless platform, since it is more in the prism-side of things, but there is always some overlap. The Robot.getScreenCapture() is one of those methods where the implementation would depend on it. |
This was discussed in a number of comments as well, and the suggested approach is to leave configuration for follow-up PR's. There are a number of options (e.g. system properties, but also an API), and some of them would require a CSR and/or more formal work. Getting more input from usecases would be helpful here.
There is no interaction yet. I believe there might be value in more direct interactions with AWT though, e.g. with the AWT Print API.
Surprisingly (or not), most of the tests I've ran (e.g. the OpenJFX system tests) do not require the prism SW pipeline. But tests where one wants to check what pixels are rendered on screen require prism SW indeed. |
In my case, it was Scene::snapshot(), but basically the question is where the screen size has any effect? Robot? It does not seem to impact Scene::snapshot. (I've used the standalone Monkey Tester with an auto snapshot added, see https://github.com/andy-goryachev-oracle/MonkeyTest/tree/headless ) |
After spending a year in the sandbox repository, the Headless Platform is now ready to be reviewed in the main repository.
the Headless Platform
The Headless Platform is a top-level com.sun.glass.ui platform that replaces the second-level Monocle-Headless subplatform, that is part of the top-level Monocle platform.
The platform can be used like any other platform, especially for running headless JavaFX applications, or for running tests (e.g. on CI systems)
changes
The code for the Headless Platform is in a new package com.sun.glass.ui.headless in the javafx.graphics module, and it does not require a code change in other packages.
This PR adds a simple change in the
build.gradle
file, to make the Headless Platform the standard when running headless tests (instead of using Monocle/Headless)enable the Headless Platform
Setting the system property
glass.platform
toHeadless
will select the Headless Platform instead of the default one (either gtk, mac or win).testing
gradlew --info -PHEADLESS_TEST=true -PFULL_TEST=true :systemTests:cleanTest :systemTests:test
runs all the system tests, apart from the robot tests. There are 2 failing tests, but there are valid reasons for those to fail.
robot tests
Most of the robot tests are working on headless as well. add
-PUSE_ROBOT
to test those.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1836/head:pull/1836
$ git checkout pull/1836
Update a local copy of the PR:
$ git checkout pull/1836
$ git pull https://git.openjdk.org/jfx.git pull/1836/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1836
View PR using the GUI difftool:
$ git pr show -t 1836
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1836.diff
Using Webrev
Link to Webrev Comment