RFR: 8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots [v8]

Tue May 30 20:43:06 UTC 2023

On Sat, 27 May 2023 01:25:40 GMT, Alexander Zvegintsev <azvegint at 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...
>
> Alexander Zvegintsev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix macos build

src/java.desktop/share/classes/java/awt/peer/RobotPeer.java line 130:

> 128:         return false;
> 129:     }
> 130: 

The only change to this file is adding a blank line. Please revert.

src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line 139:

> 137: 
> 138:         for (TokenItem tokenItem : tokensForRectangle) {
> 139:             retVal = getRGBPixelsImpl(

This can't be right. You surely want to check all cases of retVal not just the last one.

src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java line 88:

> 86: 
> 87:     public boolean hasValidBounds() {
> 88:         //This check is very formal, in order to filter out abnormal values

very "rough" ?

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 62:

> 60:     private static final String REL_NAME =
> 61:             ".java/.robot/screencast-tokens.properties";
> 62: 

.java is already hidden. Why make .robot hidden ?

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 66:

> 64:     private static final Path PROPS_PATH;
> 65:     private static final Path PROP_FILENAME;
> 66:     private static final Object PROPS_LOCK = new Object();

Why do you need PROPS_LOCK to synchronize access to PROPS ?
I don't see why you can't synchronize on PROPS directly ?

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 150:

> 148:     }
> 149: 
> 150:     private static class WatcherThread extends Thread {

What is all this for ? Reading and writing a file doesn't need all this.

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 387:

> 385:         }
> 386: 
> 387:         try (OutputStream output = Files.newOutputStream(PROPS_PATH)) {

Note that this stream is un-buffered (slow)
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/Files.html#newOutputStream(java.nio.file.Path,java.nio.file.OpenOption...)
Maybe doesn't matter given how small the file is.

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 43:

> 41: 
> 42: #define EXCEPTION_CHECK_DESCRIBE_CLEAR() if ((*env)->ExceptionCheck(env)) { \
> 43:                                             (*env)->ExceptionDescribe(env); \

Describe already clears the exception
https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#exceptiondescribe

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 375:

> 373:     DEBUG_SCREENCAST("@@@ using screen %i\n", index);
> 374:     if (index >= screenSpace.screenCount) {
> 375:         DEBUG_SCREENCAST("⚠ Wrong index for screen\n", NULL);

I've seen this "! in a triangle" symbol in a quite a few places in the DEBUG messages.
Looks like you got some non-ascii char in there.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210736642
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210738114
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210735716
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210744585
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210754519
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210749175
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210767223
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210788290
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210799141