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

8267430: GraphicsDevice.setDisplayMode(REFRESH_RATE_UNKNOWN) throws IAE: Unable to set display mode! #4373

Closed
wants to merge 4 commits into from

Conversation

@prrace
Copy link
Contributor

@prrace prrace commented Jun 5, 2021

There are two issues here, both macOS specific.
First the original reported one that occurs when running on a shared remote VNC type desktop on macOS.
Only a single supported display mode is returned and it is also the current mode.
A program that simply enumerates the reported modes and tries to set them, if it reaches the native layer,
will fail because macOS reports the same display mode it returned as valid as now being illegal.
It does not appear to be as simple as a user's permission level since it occurs with an admin user as well
so it probably is the case that macOS simply does not allow this shared desktop to be changed although
there's no docs I can find
Ordinarily we would not have tried this since we test if the requested display mode is the same as the native
one and skip it, but the provided test used REFRESH_RATE_UNKNOWN which is allowed as meaning match
any mode that satisfies W,H,BPP. But this caused it to fail the equals test so the code is enhanced to check for that

Second, when creating a test it was discovered that at least on the built-in retina display of a 16" macbook pro,
the system default contig is never in the list of available modes and can be discovered only by querying the current mode
before any modes are changed. This beheaviour is 100% consistent no matter if the system was just rebooted, has
an external monitor attached as well, or not, activates the discrete card, or not.

Since we've had code since at least 2013 that added the default mode I suspect this has been seen before but nowhere
can I find an explanation.

It has odd consequences like after you change the display mode, you will no longer see the default display mode reported,
so the list of available modes is reduced by one.
But for a typical use case which doesn't re-query or more typically caches the original display mode in order to restore it, it presents a bigger problem that trying to restore will always fail.

So the other thing this fix does is detect when we fail to restore the initial mode and try again in a different way.
Only if that fails do we then throw an exception.


Progress

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

Issue

  • JDK-8267430: GraphicsDevice.setDisplayMode(REFRESH_RATE_UNKNOWN) throws IAE: Unable to set display mode!

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4373/head:pull/4373
$ git checkout pull/4373

Update a local copy of the PR:
$ git checkout pull/4373
$ git pull https://git.openjdk.java.net/jdk pull/4373/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4373

View PR using the GUI difftool:
$ git pr show -t 4373

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4373.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 5, 2021

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Jun 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 5, 2021

@prrace The following label will be automatically applied to this pull request:

  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the awt label Jun 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 5, 2021

Loading

@prrace
Copy link
Contributor Author

@prrace prrace commented Jun 7, 2021

fixed typo in comment

Loading

@openjdk openjdk bot removed the rfr label Jun 7, 2021
@victordyakov
Copy link

@victordyakov victordyakov commented Jun 8, 2021

please review @mrserb and @azvegint

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Jun 8, 2021

@prrace @victordyakov looks like "removed the rfr label yesterday" due to jcheck failure, so the fix is not under review right now.
" @openjdk openjdk / jcheck
Whitespace error
Column 23: trailing whitespace"

Loading

@openjdk openjdk bot added the rfr label Jun 8, 2021
@prrace
Copy link
Contributor Author

@prrace prrace commented Jun 8, 2021

@victordyakov @mrserb white space is fixed now

Loading

mrserb
mrserb approved these changes Jun 9, 2021
@prrace
Copy link
Contributor Author

@prrace prrace commented Jun 9, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

@prrace This PR has not yet been marked as ready for integration.

Loading

@prrace prrace changed the title 8267430: GraphicsDevice.setDisplayMode(REFRESH_RATE_UNKNOWN) throws AE: Unable to set display mode! 8267430: GraphicsDevice.setDisplayMode(REFRESH_RATE_UNKNOWN) throws IAE: Unable to set display mode! Jun 9, 2021
@prrace
Copy link
Contributor Author

@prrace prrace commented Jun 9, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

@prrace 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:

8267430: GraphicsDevice.setDisplayMode(REFRESH_RATE_UNKNOWN) throws IAE: Unable to set display mode!

Reviewed-by: serb

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 199 new commits pushed to the master branch:

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Jun 9, 2021
@openjdk openjdk bot closed this Jun 9, 2021
@openjdk openjdk bot added integrated and removed ready labels Jun 9, 2021
@openjdk openjdk bot removed the rfr label Jun 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

@prrace Since your change was applied there have been 199 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 991ca14.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

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