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
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -34,6 +34,7 @@
import java.awt.Window;
import java.awt.geom.Rectangle2D;
import java.awt.peer.WindowPeer;
import java.util.Arrays;
import java.util.Objects;

import sun.java2d.SunGraphicsEnvironment;
@@ -65,9 +66,11 @@

// Save/restore DisplayMode for the Full Screen mode
private DisplayMode originalMode;
private DisplayMode initialMode;

public CGraphicsDevice(final int displayID) {
this.displayID = displayID;
this.initialMode = getDisplayMode();
This conversation was marked as resolved by prrace

This comment has been minimized.

@mrserb

mrserb Jun 7, 2021
Member

This probably should be revalidated when this device is invalidated, otherwise deleted device will restore its own old-initial mode, instead of the new-mode for the new device.

This comment has been minimized.

@prrace

prrace Jun 7, 2021
Author Contributor

Ok I've added copying it in invalidate()


if (MacOSFlags.isMetalEnabled()) {
// Try to create MTLGraphicsConfig, if it fails,
@@ -201,6 +204,7 @@ public int getScaleFactor() {
public void invalidate(CGraphicsDevice device) {
//TODO do we need to restore the full-screen window/modes on old device?
displayID = device.displayID;
initialMode = device.initialMode;
}

@Override
@@ -306,14 +310,47 @@ public boolean isDisplayChangeSupported() {
return true;
}

/* If the modes are the same or the only difference is that
* the new mode will match any refresh rate, no need to change.
*/
private boolean isSameMode(final DisplayMode newMode,
final DisplayMode oldMode) {

return (Objects.equals(newMode, oldMode) ||
(newMode.getRefreshRate() == DisplayMode.REFRESH_RATE_UNKNOWN &&
newMode.getWidth() == oldMode.getWidth() &&
newMode.getHeight() == oldMode.getHeight() &&
newMode.getBitDepth() == oldMode.getBitDepth()));
}

@Override
public void setDisplayMode(final DisplayMode dm) {
if (dm == null) {
throw new IllegalArgumentException("Invalid display mode");
}
if (!Objects.equals(dm, getDisplayMode())) {
nativeSetDisplayMode(displayID, dm.getWidth(), dm.getHeight(),
dm.getBitDepth(), dm.getRefreshRate());
if (!isSameMode(dm, getDisplayMode())) {
try {
nativeSetDisplayMode(displayID, dm.getWidth(), dm.getHeight(),
dm.getBitDepth(), dm.getRefreshRate());
} catch (Throwable t) {
/* In some cases macOS doesn't report the initial mode
* in the list of supported modes.
* If trying to reset to that mode causes an exception
* try one more time to reset using a different API.
* This does not fix everything, such as it doesn't make
* that mode reported and it restores all devices, but
* this seems a better compromise than failing to restore
*/
This conversation was marked as resolved by prrace

This comment has been minimized.

@mrserb

mrserb Jun 7, 2021
Member

Would like to highlight that this tradeoff will break the spec, since we successfully restore the mode which is not in the list of modes.

This comment has been minimized.

@prrace

prrace Jun 7, 2021
Author Contributor

So .. it was in the lsit of modes that the app was handed, and it is a bug that it isn't still there.
But what I've just done is fixed getDIsplayModes() to include it

if (isSameMode(dm, initialMode)) {
nativeResetDisplayMode();
if (!isSameMode(initialMode, getDisplayMode())) {
throw new IllegalArgumentException(
"Could not reset to initial mode");
}
} else {
throw t;
}
}
}
}

@@ -324,7 +361,22 @@ public DisplayMode getDisplayMode() {

@Override
public DisplayMode[] getDisplayModes() {
return nativeGetDisplayModes(displayID);
DisplayMode[] nativeModes = nativeGetDisplayModes(displayID);
boolean match = false;
for (DisplayMode mode : nativeModes) {
if (initialMode.equals(mode)) {
match = true;
break;
}
}
if (match) {
return nativeModes;
} else {
int len = nativeModes.length;
DisplayMode[] modes = Arrays.copyOf(nativeModes, len+1, DisplayMode[].class);
modes[len] = initialMode;
return modes;
}
}

public static boolean usingMetalPipeline() {
@@ -344,6 +396,8 @@ private void initScaleFactor() {

private static native double nativeGetScaleFactor(int displayID);

private static native void nativeResetDisplayMode();

private static native void nativeSetDisplayMode(int displayID, int w, int h, int bpp, int refrate);

private static native DisplayMode nativeGetDisplayMode(int displayID);
@@ -261,6 +261,18 @@ static jobject createJavaDisplayMode(CGDisplayModeRef mode, JNIEnv *env) {
return ret;
}

/*
* Class: sun_awt_CGraphicsDevice
* Method: nativeResetDisplayMode
* Signature: ()V
*/
JNIEXPORT void JNICALL
Java_sun_awt_CGraphicsDevice_nativeResetDisplayMode
(JNIEnv *env, jclass class)
{
CGRestorePermanentDisplayConfiguration();
}

/*
* Class: sun_awt_CGraphicsDevice
* Method: nativeSetDisplayMode
@@ -0,0 +1,72 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/**
* @test
* @bug 8267430
* @key headful
* @summary verify setting a display mode with unknow refresh rate works
*/

import java.awt.DisplayMode;
import java.awt.GraphicsDevice;
import java.awt.GraphicsEnvironment;

public class UnknownRefrshRateTest {

public static void main(String[] args) throws Exception {

GraphicsEnvironment ge = GraphicsEnvironment.getLocalGraphicsEnvironment();
GraphicsDevice[] devices = ge.getScreenDevices();

for (GraphicsDevice d : devices) {

if (!d.isDisplayChangeSupported()) {
This conversation was marked as resolved by prrace

This comment has been minimized.

@azvegint

azvegint Jun 7, 2021
Member

BTW, do we want to run this test on other platforms?

If so, we need to modify test. Because isDisplayChangeSupported() may return true on Windows and Linux only if we have a fullscreen window showing.

This comment has been minimized.

@prrace

prrace Jun 7, 2021
Author Contributor

I don't see the harm.

continue;
}
DisplayMode odm = d.getDisplayMode();
System.out.println("device=" + d + " original mode=" + odm);

DisplayMode[] modes = d.getDisplayModes();
System.out.println("There are " + modes.length + " modes.");
try {
for (int i=0; i<modes.length; i++) {
DisplayMode mode = modes[i];
System.out.println("copying from mode " + i + " : " + mode);
int w = mode.getWidth();
int h = mode.getHeight();
int bpp = mode.getBitDepth();
int refRate = DisplayMode.REFRESH_RATE_UNKNOWN;
DisplayMode newMode = new DisplayMode(w, h, bpp, refRate);
d.setDisplayMode(newMode);
Thread.sleep(2000);
System.out.println("set " + d.getDisplayMode());
}
} finally {
System.out.println("restoring original mode"+odm);
d.setDisplayMode(odm);
This conversation was marked as resolved by prrace

This comment has been minimized.

@mrserb

mrserb Jun 7, 2021
Member

I suggest waiting here as well.

This comment has been minimized.

@mrserb

mrserb Jun 8, 2021
Member

I still suggest waiting here, other tests were updated to wait 10 seconds, since on the old systems the last restore may affect the next test.

Thread.sleep(10000);
}
}
}
}