8154847: Rendering is incorrect or not visible with StageStyle.UNIFIED on some graphics cards#2058
8154847: Rendering is incorrect or not visible with StageStyle.UNIFIED on some graphics cards#2058crschnick wants to merge 1 commit intoopenjdk:masterfrom
Conversation
|
👋 Welcome back crschnick! A progress list of the required criteria for merging this PR into |
|
/contributor add @beldenfox |
|
@crschnick This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 5 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@lukostyra, @kevinrushforth) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@crschnick |
|
Reviewers: @lukostyra @kevinrushforth /reviewers 2 |
|
@kevinrushforth |
lukostyra
left a comment
There was a problem hiding this comment.
LGTM. This change is fairly straightforward and it removes a pretty important point of discrepancy between hardware. To my knowledge even modern APIs support ABGR format more commonly than XBGR, so I don't suspect this would break anything.
My main Windows machine did not have the issue (so I guess it won the GPU driver lottery) but I verified it with the Windows 10 Hyper-V VM - Unified Stages do not render without the fix and render properly with transparency when adding the fix. Other sanity testing also looked good.
kevinrushforth
left a comment
There was a problem hiding this comment.
I wasn't able to reproduce this (I tried on an old system that once had done so, but that was when it was running Windows 10 with a different driver).
I did run some tests and I see no regressions, so I'll approve this as well.
|
/integrate |
|
@crschnick |
|
/sponsor |
|
Going to push as commit 28bde15.
Your commit was automatically rebased without conflicts. |
|
@kevinrushforth @crschnick Pushed as commit 28bde15. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
I think this is a good target for a backport? I have no idea how the backport process works in practice though. Is there page where that process is documented? |
Given where we are in the JavaFX 26 release cycle, it is too late for the GA release, but this seems like a good candidate for a 26 update release (jfx26u) and possibly also a 25 update release (jfx25u). See this message on the mailing list. The easiest way is to use Skara's |
|
/backport jfx26u |
|
@crschnick To use the |
|
Ok so according https://wiki.openjdk.org/spaces/SKARA/pages/39616516/Skara#Skara-GitHub, associating an account is only posible for Author, Committer or Reviewer If I understand correctly, I am a Contributor right now. But I show up in https://openjdk.org/census#crschnick as Author? Or do the different projects use inconsistent naming schemes for the same thing? |
Skara doesn't know that you are you. :) Meaning, Skara doesn't know that I meant to point you at this shortly after you became an Author, but forgot. |
|
Oh yeah I get that, but I was confused the names for the roles. I got them mixed up, everything is good now |
|
/backport jfx26u |
|
@crschnick the backport was successfully created on the branch backport-crschnick-28bde153-master in my personal fork of openjdk/jfx26u. To create a pull request with this backport targeting openjdk/jfx26u:master, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jfx26u: |
|
Ok that worked now. Two things I noticed: The backport does not list co-contributors and the autogenerated email with the PR description template contains leading spaces, meaning that you need to do some manual fomatting after copy-pasting the text |
The PR description is just meant for those who will look at it, so not listing co-contributors doesn't present a problem. I hadn't noticed the leading spaces before, but I don't typically look at the email Skara sends when creating a backport PR. I see that you created the backport PR successfully, so I'll take a look at it. |
|
/backport jfx25u |
|
@crschnick the backport was successfully created on the branch backport-crschnick-28bde153-master in my personal fork of openjdk/jfx25u. To create a pull request with this backport targeting openjdk/jfx25u:master, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jfx25u: |
|
/backport jfx17u |
|
@crschnick the backport was successfully created on the branch backport-crschnick-28bde153-master in my personal fork of openjdk/jfx17u. To create a pull request with this backport targeting openjdk/jfx17u:master, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jfx17u: |
The previous value of BackBufferFormat for the D3DPRESENT_PARAMETERS for the d3d9 swap chain was set to D3DFMT_UNKNOWN by default. This causes the d3d implementation to use a default value, which depends on the system configuration. The RTT texture by JavaFX always uses D3DFMT_A8R8G8B8.
If the d3d swap chain defaults to D3DFMT_X8R8G8B8, this creates a format mismatch. In theory, that shouldn't be a big deal as it's only about the alpha channel being used or not in this case. But for whatever reason, the format conversion by d3d is completely whack here. This is not only an alpha conversion issue, I tested for that. The whole conversion is wrong.
This only occurs when using a translucent background, e.g. with the UNIFIED stage style previously. This issue is a blocker for the new translucent backdrops and translucent EXTENDED stage style PRs that are open. Because without this, you can't guarantee that they will display properly on all Windows systems. Most will work fine, but some will be unusable.
Most d3d swap chain implementations I have seen don't set an explicit BackBufferFormat. However, in the case of JavaFX, it is easier to set the format here than do it for the RTT texture to maintain the same format on all platforms.
The most important part about this issue is reproduction and testing: I have tested it on multiple Windows 11 systems, Windows 10 1709, and Windows 10 22H2. It fixes the issue on the affected systems and maintains the same behavior on the other systems.
The reproduction instructions of the original issue are quite old, so it's difficult to get this properly set up on modern systems. The hardware that was affected back then might have gotten driver updates. The easiest way I found was to run Hyper-V on Windows, and create a new MSIX packaging environment VM.
You can do so at Hyper-V Manager -> Action -> Quick Create -> Windows 10 MSIX packaging environment. This creates a Windows 10 VM which reproduces the issue reliably. The Windows 11 VM does not for some reason.
You can run the BackdropTest from https://github.com/openjdk/jfx/pull/2048/changes#diff-c25c5ccf435e59b6bfd593251ab6dfc31a12a782a78191a38b4e9adab31587a3 with and without the BackBufferFormat line added to compare.
Progress
Issue
Reviewers
Contributors
<mfox@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2058/head:pull/2058$ git checkout pull/2058Update a local copy of the PR:
$ git checkout pull/2058$ git pull https://git.openjdk.org/jfx.git pull/2058/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2058View PR using the GUI difftool:
$ git pr show -t 2058Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2058.diff
Using Webrev
Link to Webrev Comment