-
Notifications
You must be signed in to change notification settings - Fork 464
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
8329820: [Linux] Prefer EGL over GLX #1381
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # modules/javafx.graphics/src/main/native-prism-es2/x11/X11GLContext.c
👋 Welcome back tsayao! A progress list of the required criteria for merging this PR into |
@tsayao this pull request can not be integrated into git checkout egl
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
❗ This change is not yet ready to be integrated. |
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 took a brief look at your changes and I shared some comments.
Overall the changes look good. The EGL/GLX loading in GLFactory feels a bit hacky, but other than making EGL a completely separate Prism backend (which would duplicate/reorganize a lot of common EGL/GLX code - definitely a no-go IMO) I don't have a better idea how to solve this. Maybe with an ES2-specific property like prism.es2.forceglx=true
?
This brings me to a question - other than some form of debugging/fallback for when EGL is still new to JFX, do we even need GLX implementation when we have a working EGL implementation? I'm fairly sure that current distros (especially ones officially supported by JFX) have EGL support available at this point in time, and since it's an intermediary layer between application and runtime it shouldn't depend on used GPU.
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/com/sun/prism/es2/LinuxEGLContext.java
Outdated
Show resolved
Hide resolved
Also a side note - the PR title does not match the JBS issue. |
Webrevs
|
This will need a fair bit of testing and review. I would like a chance to review it, but won't be able to get to it for a couple weeks. Reviewers: @kevinrushforth @lukostyra @johanvos @arapte |
@kevinrushforth |
I have to figure out how to test Monocle, since I have changed the logic of defines on |
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 a few comments, mostly for adjusting error-checking. Once those are fixed and others have their input I'll test those changes.
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLDrawable.c
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLDrawable.c
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c
Outdated
Show resolved
Hide resolved
I changed it to use EGL_DEFAULT_DISPLAY in an attempt to make no assumptions on the platform. It works, but I'm not sure if it's correct. I'm looking at Mesa egl source to be sure. I also changed the dummy window to use PBUFFER (also to make no assumptions on the platform, so there's no need to create a XWindow). I'm also not sure about that. |
It is possible to set the platform with |
I'll have to fix it I got a reply explaining it (the docs where not very clear) |
I changed it to use platform specific way to get It can be seen by setting I also reverted the Dummy window code change. |
@tsayao Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
Reopening so maybe it could be usefult to 8332108 |
Testing 8332108 with this branch seems to have a significance in favor of EGL: Using this branch, compare the attached example with and without |
Note: Running full Robot systemtests are failing. Will look. |
@tsayao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
I will get back to this next week. |
Would it be better to still default to GLX and have a flag to use EGL (and switch within a few releases), or to go with EGL and have a flag to fallback to GLX? Currently (on this PR) it defaults to EGL first, and if it fails, try GLX (I don't know a scenario where EGL won't work, on Xorg o XWayland). |
|
It is related to system's DPI setting. We had a problem with UI elements sometimes being incorrectly rendered/shifted when scaling setting in the system was set to more than 100% - see JDK-8299968 |
Found the problem:
From the DOCS:
This also happens on the current GLX implementation and is probably a leak. Place |
@tsayao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
EGL requires an |
Wayland implementation will require EGL.
EGL works with Xorg as well. The idea is to be EGL first and if it fails, fallback to GLX. A force flag
prism.es2.forceGLX=true
is available.See:
Switching the Linux graphics stack from GLX to EGL
Prefer EGL to GLX for the GL support on X11
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1381/head:pull/1381
$ git checkout pull/1381
Update a local copy of the PR:
$ git checkout pull/1381
$ git pull https://git.openjdk.org/jfx.git pull/1381/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1381
View PR using the GUI difftool:
$ git pr show -t 1381
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1381.diff
Webrev
Link to Webrev Comment