On Thu, 4 May 2023 14:18:44 GMT, Alexander Zvegintsev <azveg...@openjdk.org> wrote:
> Modern Linux systems often come with > [Wayland](https://wayland.freedesktop.org/) by default. > This comes with some difficulties, and one of them is the inability to get > screenshots from the system. > This is because we now use the [X Window System > API](https://en.wikipedia.org/wiki/X_Window_System) to capture screenshots > and it cannot access data outside the [XWayland > server](https://wayland.freedesktop.org/xserver.html) > > But this functionality is a very important part of automated testing. > > > At the moment there are two obvious solutions to this problem, and both use > [xdg-desktop-portal](https://github.com/flatpak/xdg-desktop-portal): > > 1. [org.freedesktop.portal.Screenshot DBUS > API](https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.Screenshot) > It has several drawbacks though: > + It saves a screenshot to disk, which must be read and deleted(may add some > delays depending on the type of a disk drive). > + There is no way to disable the visual "screen flash" after screenshot > + It asks a user confirmation to save a screenshot. This confirmation can be > saved on Gnome 43+. > Since we would like Ubuntu 22.04 LTS which comes with Gnome 42 this option is > not acceptable for us because it would require user confirmation for each > screenshot. > But we still can consider this option as a fallback. > > > > 2. > [org.freedesktop.portal.ScreenCast](https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.ScreenCast) > It typically used by applications that need to capture the contents of the > user's screen or a specific window for the purpose of sharing, recording, or > streaming. > This might be a bit of overkill, but it avoids several of the problems > mentioned in the Screenshot API. > > + implementation is more complicated comparing to Screenshot API > + no intermediate file, screenshot data can be obtained from memory > + Permission to make screenshots can be stored with > [`restore_token`](https://flatpak.github.io/xdg-desktop-portal/#gdbus-method-org-freedesktop-portal-ScreenCast.SelectSources) > > > So this PR adds the ability to take screenshots using the ScreenCast API. > This functionality is currently disabled by default. > > This change also introduces some new behavior for the robot: > A system window now appears asking for confirmation from the user to capture > the screen. > + The user can refuse the screen capture completely. In this case a security > exception will be thrown. > + The user can allow a screen capture for all screens or some of them. For > screens without permission there will be a black image. > + If the user wishes to change their mind about the screens allowed to be > captured, the user should use the new `Robot#resetScreenCapturePermission` > method > > Also added several system properties: > `awt.robot.screencastEnabled` false by default, uses the ScreenCast API if > true > `awt.robot.screencastDebug` false by default, prints some debug information > if true > > For convenience, this change is divided into two commits: > 1. added [pipewire > headers](https://gitlab.freedesktop.org/pipewire/pipewire/) that are needed > for the build > 2. main changes > > Changes in the documentation are in a separate draft PR (#13809) also for > convenience. > > At the moment, the planned supported systems are Ubuntu 22.04+, the latest > Oracle Linux 9.1 and RHEL 9.1 has some issues with restore_token support. src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java line 428: > 426: > 427: @SuppressWarnings("removal") > 428: public static boolean isOnWayland() { It seems that this method doesn't have to part of a public interface of `UNIXToolkit`. src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line 56: > 54: } > 55: > 56: private static final int DENIED = -11; //defined in native code The comment is confusing as the field is defined right here, not in native code. src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line 58: > 56: private static final int DENIED = -11; //defined in native code > 57: > 58: private static synchronized native int getRGBPixelsImpl( Is `synchronized` really needed on the declaration of `getRGBPixelsImpl()`? src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 50: > 48: jmethodID storeTokenMethodID = NULL; > 49: > 50: inline void debug_screencast( Does this one need to be externally visible? If not, it can be (also) made `static`. src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 138: > 136: } > 137: > 138: static inline void convertRGBxToBGRx(int* in) { Will this work correctly on little-endian ARM systems? src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 602: > 600: } > 601: > 602: pipewire_libhandle = dlopen("libpipewire-0.3.so.0", There's a useful macro for wrapping library names: `VERSIONED_JNI_LIB_NAME("pipewire-0.3", "0")` src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 722: > 720: > 721: const gchar *token = jtoken > 722: ? token = (*env)->GetStringUTFChars(env, > jtoken, NULL) The variable `token` is unnecessarily used in its own initializer here. src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 789: > 787: env, pixelArray, > 788: start, len, > 789: ((jint *) screenProps->captureData) I'm not sure if endianness of `captureData` matches the expected endianness of `pixelArray`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185887472 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185891793 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185894200 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185907122 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185917587 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185942008 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185946946 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1185951908
