8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots #13803

azvegint · 2023-05-04T14:18:44Z

Modern Linux systems often come with Wayland 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 to capture screenshots and it cannot access data outside the XWayland server

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:

org.freedesktop.portal.Screenshot DBUS API
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.

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

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:

added pipewire headers that are needed for the build
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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8307456 to be approved

Issues

JDK-8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots (Bug - "3")
JDK-8307456: [Wayland] [XWayland] java.awt.Robot taking screenshots (CSR)

Reviewers

Phil Race (@prrace - Reviewer) ⚠️ Review applies to 389cfd73
Alexander Zuev (@azuev-java - Reviewer) ⚠️ Review applies to 63b34b9d
Prasanta Sadhukhan (@prsadhuk - Reviewer) ⚠️ Review applies to f5579309

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13803/head:pull/13803
$ git checkout pull/13803

Update a local copy of the PR:
$ git checkout pull/13803
$ git pull https://git.openjdk.org/jdk.git pull/13803/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13803

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13803.diff

Webrev

Link to Webrev Comment

azvegint · 2023-05-04T14:19:28Z

/csr

bridgekeeper · 2023-05-04T14:20:38Z

👋 Welcome back azvegint! 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.

openjdk · 2023-05-04T14:22:38Z

@azvegint has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@azvegint please create a CSR request for issue JDK-8280982 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2023-05-04T14:25:38Z

@azvegint The following labels will be automatically applied to this pull request:

build
client

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

mlbridge · 2023-05-04T15:34:41Z

Webrevs

erikj79

I would like to understand why we need to add third party headers to our source tree. Are these not generally available on Linux systems?

You say that the functionality isn't enabled by default, is that a runtime thing? It loos like it will be built unconditionally.

make/modules/java.desktop/lib/Awt2dLibraries.gmk

prrace · 2023-05-04T20:22:27Z

I would like to understand why we need to add third party headers to our source tree. Are these not generally available on Linux systems?

They are not available on anything except very new Linux distros.
Forget RHEL 6.x / 7.x. I'm fairly sure not 8.x either.
RHEL/OL 9.x could an example of the earliest that would work to build without these imports.
And that's just for building, even current 9.1 doesn't have sufficiently working runtime libs.
There should be a fix for that in 9.2 ..

Ubuntu 22.04 is however known to be OK.

You say that the functionality isn't enabled by default, is that a runtime thing? It loos like it will be built unconditionally.

Yes, a runtime thing. An implementation System Property needs to be set.

mrserb · 2023-05-05T02:37:28Z

If the user wishes to change their mind about the screens allowed to be captured, the user should use the new Robot#resetScreenCapturePermission method

Do we really need to add this new API? probably we can implement the feature w/o this new method?

mkartashev · 2023-05-05T09:33:00Z

src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java

+    private static volatile Boolean isOnWayland = null;
+
+    @SuppressWarnings("removal")
+    public static boolean isOnWayland() {


It seems that this method doesn't have to part of a public interface of UNIXToolkit.

You're right. Furthermore, Wayland detection is no longer used in this change, so I am removing it.

It is now in #13830, where it belongs.

src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c

mkartashev · 2023-05-05T10:06:27Z

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c

+    return TRUE;
+}
+
+static inline void convertRGBxToBGRx(int* in) {


Will this work correctly on little-endian ARM systems?

I haven't tested it, but I'll try to find a machine to test it on.

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c

mkartashev · 2023-05-05T10:48:06Z

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c

+                    (*env)->SetIntArrayRegion(
+                            env, pixelArray,
+                            start, len,
+                            ((jint *) screenProps->captureData)


I'm not sure if endianness of captureData matches the expected endianness of pixelArray.

For now, it is going to be supported on x86-64 only, but it is a good candidate for investigation.

Is this enforced somehow? I mean what happens if this code is built on risc-v or arm?

azvegint · 2023-05-05T14:58:15Z

If the user wishes to change their mind about the screens allowed to be captured, the user should use the new Robot#resetScreenCapturePermission method

Do we really need to add this new API? probably we can implement the feature w/o this new method?

I couldn't think of anything better.
We need to somehow let the robot know that the saved restore_token no longer meets the user's expectations.
It seems that the current robot API is not suitable for this.

mrserb · 2023-05-08T18:41:56Z

It seems that the current robot API is not suitable for this.

Can we just reset the token when we create a new instance of Robot?

azvegint · 2023-05-09T14:37:53Z

It seems that the current robot API is not suitable for this.

Can we just reset the token when we create a new instance of Robot?

This will be a big problem for automated testing, as each test with the robot will require user confirmation(or even more than one, as some tests instantiate it several times).
This is exactly what we are trying to avoid.

On the other hand, we can introduce a new system property(e.g. awt.robot.screencastKeepPermission) that may or may not be true by default.

prrace · 2023-05-09T20:43:13Z

I'm thinking about the re-set API

"If the user wishes to change their mind about the screens allowed to be captured, the user should use the new Robot#resetScreenCapturePermission method"

Well the "user" is hosed unless the app calls this every time ... or provides UI for it ..

I want to work through the scenarios and how much of it is specific to the behaviours of the API you are using and so forth.
Since you use the Preferences API for saving the token, if you keyed it off the Robot class rather than the internal API class
then anyone using the Preferences API could delete the token, couldn't they ?

And we presumably can store alongside the token, info about the screens attached at the time we obtained the token ?

Can you point (here in the implementation review) to the exact native functions that popup up the dialog ?
I'm still trying to find my way through that code. Is it opaque to the caller (us) what the user actually approved ?

I can't imagine why we would need to reset "we have all permissions" to "we have no permissions" so I'm supposing
the issue is when they didn't grant permissions .. but if they "deny" permissions that's surely different than "this
token is no longer valid" ?

The first time we need a permission for the screencast API there is a dialog.
The user makes some choices and we then choose to save a token so that the user isn't prompted again.
We must at least know though that they approved SOMETHING but we don't know what ?
Or do we ?
If they DENY, would we save that ? Interesting question because if we don't save something we'll keep spamming them until they approve :-)

Next time, what happens if for some reaon the token is no longer valid ?
And what might make it invalid ?

If the user denied screen 2, or added screen 2 later, would that not result in an invalid token and we'd know
to ask again ? No reset needed ? Also I can't figure out if "validating" the token can cause the dialog as a side effect
or if there's some API we explicitly call to request a new token.

azvegint · 2023-05-09T22:09:26Z

I want to work through the scenarios and how much of it is specific to the behaviours of the API you are using and so forth. Since you use the Preferences API for saving the token, if you keyed it off the Robot class rather than the internal API class then anyone using the Preferences API could delete the token, couldn't they ?

Right now it can be read with a simple call:
System.out.println(Preferences.userRoot().node("/sun/awt/screencast").get("restoreToken", "")); (modification is also simple).

Moving to Robot doesn't change anything in this regard. So anyone can delete it.

And we presumably can store alongside the token, info about the screens attached at the time we obtained the token ?

We can try to do that and switch it depending on the area requested, but I tried to keep it simple.

Can you point (here in the implementation review) to the exact native functions that popup up the dialog ? I'm still trying to find my way through that code. Is it opaque to the caller (us) what the user actually approved ?

(It is collapsed by the github in the review, so links are to the branch)

basically if you didn't provide a correct token or no token at all to SelectSources call it will prompt with dialog right after the Start call

I can't imagine why we would need to reset "we have all permissions" to "we have no permissions" so I'm supposing the issue is when they didn't grant permissions .. but if they "deny" permissions that's surely different than "this token is no longer valid" ?

It is not for the "we have all permissions" case, it is for the case when we only have a permission to some screens. So it won't hurt.

The first time we need a permission for the screencast API there is a dialog. The user makes some choices and we then choose to save a token so that the user isn't prompted again. We must at least know though that they approved SOMETHING but we don't know what ? Or do we ?

No we don't.

We receive the response callbackScreenCastStart as a separate streams for each allowed display.

Based on this response we building a list of available displays.
So denied screens will not be there.

At this point we can compare this result with the screens got from X11 API(won't be compatible with Wayland, where we also want to reuse it later) or with Wayland(+1 dependency at this point).

If they DENY, would we save that ? Interesting question because if we don't save something we'll keep spamming them until they approve :-)

The DENY case propagated to java and we are throwing the security exception.

Next time, what happens if for some reaon the token is no longer valid ?

It just prompts a permission request again.

And what might make it invalid ?

Adding or removing a display for example.

If the user denied screen 2, or added screen 2 later, would that not result in an invalid token and we'd know to ask again ? No reset needed ? Also I can't figure out if "validating" the token can cause the dialog as a side effect or if there's some API we explicitly call to request a new token.

Let me elaborate how this restore_token works:

We may or may not provide the restore_token while calling SelectSources.

If the token provided is not valid (we have no control over this, it's checked on the system side), the permission prompt will appear.

Later, after granting permissions, the restore_token is returned by Start callback. We should update it, as it may not be the same as we provided before.

When we are calling the resetScreenCapturePermission we are not actually revoking the token from the system, we are just doesn't add it to the SelectSources call. (but, if needed, we can provide it later and it should work).

So whenever we don't provide a valid token, we get a confirmation prompt.

prrace · 2023-05-10T21:59:37Z

I want to work through the scenarios and how much of it is specific to the behaviours of the API you are using and so forth. Since you use the Preferences API for saving the token, if you keyed it off the Robot class rather than the internal API class then anyone using the Preferences API could delete the token, couldn't they ?

Right now it can be read with a simple call: System.out.println(Preferences.userRoot().node("/sun/awt/screencast").get("restoreToken", "")); (modification is also simple).

Moving to Robot doesn't change anything in this regard. So anyone can delete it.

I was thinking it better to make it an API class, but we can discuss later when we are sure we want the API at all.

And we presumably can store alongside the token, info about the screens attached at the time we obtained the token ?

We can try to do that and switch it depending on the area requested, but I tried to keep it simple.

If it can avoid the need for dialogs in some additional cases, its worth it, but
we can add this later, correct ?
But it may be worth doing now - see comments below.

Can you point (here in the implementation review) to the exact native functions that popup up the dialog ? I'm still trying to find my way through that code. Is it opaque to the caller (us) what the user actually approved ?

(It is collapsed by the github in the review, so links are to the branch)

basically if you didn't provide a correct token or no token at all to SelectSources call it will prompt with dialog right after the Start call

I can't imagine why we would need to reset "we have all permissions" to "we have no permissions" so I'm supposing the issue is when they didn't grant permissions .. but if they "deny" permissions that's surely different than "this token is no longer valid" ?

It is not for the "we have all permissions" case, it is for the case when we only have a permission to some screens. So it won't hurt.

But how does the app know when to call it ?

The first time we need a permission for the screencast API there is a dialog. The user makes some choices and we then choose to save a token so that the user isn't prompted again. We must at least know though that they approved SOMETHING but we don't know what ? Or do we ?

No we don't.

We receive the response callbackScreenCastStart as a separate streams for each allowed display.

Based on this response we building a list of available displays. So denied screens will not be there.

OK, so to make sure I understand, you use the token to see if you can obtain a stream for all connected displays. If any "fail" you know the user didn't approve that.

So we can do this every time right ?
And in conjunction with knowing which screen[s] were connected when we captured
the token we know the situation changed and so we know it is appropriate to ignore the stored token - causing the user to be re-prompted.

And even if the screens have changed if we already have the token we need for THIS screen capture we know whether we already have the permission we need. And if we do then we just use it and don't cause a user re-prompt.

At this point I think we've done everything that "revoke" was needed for.

Then [if I have everything right] the only open question is whether when
permissions to the required screen or screens was previously denied,
do we re-prompt ? My take is that we do not "store" a token that denies access.
So next time the app is run they will be re-prompted.
But we probably don't want to keep re-prompting within this running instance of the JDK.

BTW "the app" means "any app that can find this token, doesn't it" ?
This is probably what we want, and this is also how the Apple permissions work ..
but maybe non-intuitive to users who don't know that both App A and App B are written in Java.
I don't propose doing anything here, since if we tied it to a specific "class" then
it would not work very well for 500 individual regression tests that each need robot :-)

At this point we can compare this result with the screens got from X11 API(won't be compatible with Wayland, where we also want to reuse it later) or with Wayland(+1 dependency at this point).

If they DENY, would we save that ? Interesting question because if we don't save something we'll keep spamming them until they approve :-)

The DENY case propagated to java and we are throwing the security exception.

Next time, what happens if for some reaon the token is no longer valid ?

It just prompts a permission request again.

And what might make it invalid ?

Adding or removing a display for example.

If the user denied screen 2, or added screen 2 later, would that not result in an invalid token and we'd know to ask again ? No reset needed ? Also I can't figure out if "validating" the token can cause the dialog as a side effect or if there's some API we explicitly call to request a new token.

Let me elaborate how this restore_token works:

We may or may not provide the restore_token while calling SelectSources.

If the token provided is not valid (we have no control over this, it's checked on the system side), the permission prompt will appear.

Later, after granting permissions, the restore_token is returned by Start callback. We should update it, as it may not be the same as we provided before.

When we are calling the resetScreenCapturePermission we are not actually revoking the token from the system, we are just doesn't add it to the SelectSources call. (but, if needed, we can provide it later and it should work).

I think most people reading the API would interpret it as "rm , not
"don't use the token". And since its not tied to a particular invocation of Robot screen
capture APIs it means that if you then call System.exit(0) right afterwards, so no new token is captured, it didn't do what you thought it did.
Also if we follow the suggested policy of not storing tokens which don't grant permission
then we'd also not over-write it in that case.

So whenever we don't provide a valid token, we get a confirmation prompt.

azvegint · 2023-05-11T14:13:48Z

It is not for the "we have all permissions" case, it is for the case when we only have a permission to some screens. So it won't hurt.

But how does the app know when to call it ?

Right now it doesn't know. If the user is not satisfied with the result, he can "reset" it. But it seems to be a weak position.

OK, so to make sure I understand, you use the token to see if you can obtain a stream for all connected displays. If any "fail" you know the user didn't approve that.

So we can do this every time right ? And in conjunction with knowing which screen[s] were connected when we captured the token we know the situation changed and so we know it is appropriate to ignore the stored token - causing the user to be re-prompted.

And even if the screens have changed if we already have the token we need for THIS screen capture we know whether we already have the permission we need. And if we do then we just use it and don't cause a user re-prompt.

At this point I think we've done everything that "revoke" was needed for.

The problem is how to relate the tokens to the screens. We don't have some universal id that says that the display we saved earlier is this current display.

There is an opaque id of stream:
https://flatpak.github.io/xdg-desktop-portal/#gdbus-method-org-freedesktop-portal-ScreenCast.Start

 Stream properties include:

id s

    Opaque identifier. **Will be unique for this stream and local to this session**. 
Will persist with future sessions, if they are restored using a restore token. 
This property was added in version 4 of this interface. Optional.

But it didn't help much, as it local to the session(each token - different session), token looks like 0, 1, etc

We can match the stream layout to the screen layout in the system, but it has its flaws:

Let's say we have following layout of screens in system, displays have the same resolution(e.g. 1920*1080):

Screen capture permission is granted for A and denied for B.

On a first run we are capturing screen area at (100, 100, 100, 100).
We match an acquired restore_token to the screen at (0,0,1920,1080)
User decided to change screens to following layout:
we are capturing screen area at (100, 100, 100, 100) again
Oh, we have a restore_token for the (0,0,1920,1080) screen, let's reuse it.
Ouch, we got black pixels because the token was issued for display A and it is now (1920,0,1920,1080)

Then [if I have everything right] the only open question is whether when permissions to the required screen or screens was previously denied, do we re-prompt ? My take is that we do not "store" a token that denies access. So next time the app is run they will be re-prompted. But we probably don't want to keep re-prompting within this running instance of the JDK.

BTW "the app" means "any app that can find this token, doesn't it" ?

Yes, it is currently shared between Java applications for the same user.

Also if we follow the suggested policy of not storing tokens which don't grant permission
then we'd also not over-write it in that case.

Currently we only overwrite tokens when we get a new one from the system. In the full DENY case, we do not receive any token.

prrace

I'm getting pretty consistent testing results now - meaning the ones that have known
issues are the ones that fail, and I've looked at the code sufficiently.
The restore token seems to be behaving properly now.

I did have one case I've not understood where a test system got very confused about the display resolution of the monitor and needed a full power cycle.
I don't know if a specific test caused that or it was something else, but it isn't repeatable (at least not yet) so shouldn't block integration.

I am quite sure that SOME of the test failures are unstable tests that we've been lucky with all these years and xwayland is now showing up the instability. Things un-related to robot and more about timing for the most part. Although there's one test that seriously over-stresses robot with > 10,000 calls in a loop to getPixelColor() I'll see if I can push test update fixes to at least some of these to reduce noise.

But I'm OK to approve now.

openjdk · 2023-05-31T20:21:01Z

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

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

Reviewed-by: prr, kizune, psadhukhan

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

749d480: 8305763: Parsing a URI with an underscore goes through a silent exception, negatively impacting performance
3ccb3c0: 8305906: HttpClient may use incorrect key when finding pooled HTTP/2 connection for IPv6 address
a25b7b8: 8295976: GetThreadListStackTraces returns wrong state for blocked VirtualThread
fadcd65: 8309527: Improve test proxy performance
0ed4af7: 8309472: IGV: Add dump_igv(custom_name) for improved debugging
f1c7afc: 8306647: Implementation of Structured Concurrency (Preview)
a08c5cb: 8307953: [AIX] C locale's font setting was changed by JEP 400
0ceb432: 8309570: ProblemList sun/security/pkcs11/Signature/TestRSAKeyLength.java
65bdbc7: 8309396: com/sun/jdi/JdbMethodExitTest.java fails with virtual threads due to a bug in determining the main thread id
4a75fd4: 8301553: Support Password-Based Cryptography in SunPKCS11
... and 235 more: https://git.openjdk.org/jdk/compare/8474e693b4404ba62927fe0e43e68b904d66fbde...master

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.

make/modules/java.desktop/lib/Awt2dLibraries.gmk

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java

src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java

prrace · 2023-05-31T22:30:29Z

I spoke too soon :-(
I remembered about an odd result from last week and it seems that whenever I have a full screen window capture fails - at least on my system and I think one lab system.
See test and output below from a fully patched Ubuntu 22.04.02 system

import java.awt.Frame;
import java.awt.GraphicsDevice;
import java.awt.Rectangle;
import java.awt.Robot;

public class FSW {

public static void main(String args[]) throws Exception {
    Robot robot = new Robot();
    Frame frame = new Frame("FSW");
    GraphicsDevice gd = frame.getGraphicsConfiguration().getDevice();
    try {
        gd.setFullScreenWindow(frame);
        robot.delay(5000);
        robot.createScreenCapture(new Rectangle(50, 50, 10, 10));
    } finally {
        gd.setFullScreenWindow(null);
        frame.dispose();
    }
}

}

% java FSW
cropTo:163 Unexpected stride / 4: 0 srcW: 1920

and the capture fails every time.

And again with debugging turned on

ScreencastWatcher: started
// getRGBPixels affectedScreenBounds [java.awt.Rectangle[x=0,y=0,width=1920,height=1200]]
// getTokens exact matches 1. [Token: 41dc61fb-279b-4069-b408-30873451fb03
java.awt.Rectangle[x=0,y=0,width=1920,height=1200]
]
// getTokens same sizes 2. [Token: 41dc61fb-279b-4069-b408-30873451fb03
java.awt.Rectangle[x=0,y=0,width=1920,height=1200]
]
// storeToken old: |41dc61fb-279b-4069-b408-30873451fb03| new |41dc61fb-279b-4069-b408-30873451fb03| allowed bounds [0, 0, 1920, 1200]
// Storing TokenItem:
Token: 41dc61fb-279b-4069-b408-30873451fb03
java.awt.Rectangle[x=0,y=0,width=1920,height=1200]

cropTo:163 Unexpected stride / 4: 0 srcW: 1920
initXdgDesktopPortal:236 connection/sender name :1.525 / 1_525
checkVersion:190 ScreenCast protocol version 4
Java_sun_awt_screencast_ScreencastHelper_getRGBPixelsImpl:833 taking screenshot at
x: 50 y 50 w 10 h 10 with token |41dc61fb-279b-4069-b408-30873451fb03|
initXdgDesktopPortal:236 connection/sender name :1.525 / 1_525
checkVersion:190 ScreenCast protocol version 4
callbackScreenCastStart:611 available screen count 1
rebuildScreenData:87
==== screenId#35
rebuildScreenData:132 -----------------------
rebuildScreenData:133 screenId#35
|| bounds x 0 y 0 w 1920 h 1200
|| capture area x 0 y 0 w 0 h 0 shouldCapture 0

rebuildScreenData:134 #---------------------#

callbackScreenCastStart:617 rebuildScreenData result |0|
callbackScreenCastStart:630 restore_token |41dc61fb-279b-4069-b408-30873451fb03|
storeRestoreToken:653 saving token, old: |41dc61fb-279b-4069-b408-30873451fb03| > new: |41dc61fb-279b-4069-b408-30873451fb03|
portalScreenCastStart:709 ScreenCastStartResult |0|
getPipewireFd:886 portalScreenCastStart result |0|
checkCanCaptureAllRequiredScreens:847 Found allowed screen bounds in affected screen bounds 0 0 1920 1200
getPipewireFd:900 --- portalScreenCastStart
getPipewireFd:907 pwFd 17
doLoop:557 screenId#35[loc(0,0) size(1920x1200)] @@@ adding screen 0
checkScreen:472 screenId#35
|| bounds x 0 y 0 w 1920 h 1200
|| capture area x 50 y 50 w 10 h 10 shouldCapture 1

connectStream:373 @@@ using screen 0
connectStream:411 screenId#35
|| bounds x 0 y 0 w 1920 h 1200
|| capture area x 50 y 50 w 10 h 10 shouldCapture 1

startStream:355 screenId#35: stream connecting 0x7f04548e3600
onStreamStateChanged:303 screenId#35[loc(0,0) size(1920x1200)] state 0 (unconnected) -> 1 (connecting) err (null)
onStreamStateChanged:303 screenId#35[loc(0,0) size(1920x1200)] state 1 (connecting) -> 2 (paused) err (null)
onStreamParamChanged:197 screenId#35[loc(0,0) size(1920x1200)] param event id 4
onStreamParamChanged:218 screenId#35[loc(0,0) size(1920x1200)] stream format: Spa:Enum:VideoFormat:BGRx (8) 1920x1200
connectStream:424 screenId#35[loc(0,0) size(1920x1200)] frame size: 1920x1200
doLoop:563 screenId#35[loc(0,0) size(1920x1200)] @@@ screen processed 0
onStreamStateChanged:303 screenId#35[loc(0,0) size(1920x1200)] state 2 (paused) -> 3 (streaming) err (null)
onStreamProcess:236 screenId#35[loc(0,0) size(1920x1200)] hasFormat 1 captureDataReady 0 shouldCapture 1
onStreamProcess:271 screenId#35
|| bounds x 0 y 0 w 1920 h 1200
|| capture area x 50 y 50 w 10 h 10 shouldCapture 1

onStreamProcess:272 screenId#35[loc(0,0) size(1920x1200)] got a frame of size 0 offset 0 stride 0 flags 0 FD 31 captureDataReady 0
onStreamProcess:292 screenId#35[loc(0,0) size(1920x1200)] data ready
Java_sun_awt_screencast_ScreencastHelper_getRGBPixelsImpl:856
all data ready
Java_sun_awt_screencast_ScreencastHelper_getRGBPixelsImpl:864 screenId#35[loc(0,0) size(1920x1200)] @@@ copying screen data 0, captureData 0x7f03d40042a0
|| x 50 y 50 w 10 h 10 requested area
|| x 0 y 0 w 1920 h 1200 screen bound
|| x 50 y 50 w 10 h 10 in-screen coords capture area

onStreamParamChanged:197 screenId#35[loc(0,0) size(1920x1200)] param event id 4
onStreamStateChanged:303 screenId#35[loc(0,0) size(1920x1200)] state 3 (streaming) -> 2 (paused) err (null)
onStreamStateChanged:303 screenId#35[loc(0,0) size(1920x1200)] state 2 (paused) -> 0 (unconnected) err (null)
doCleanup:109 STOPPING loop

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java

src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java

prrace · 2023-06-01T16:47:18Z

Regarding the failed capture

cropTo:163 Unexpected stride / 4: 0 srcW: 1920

I've confirmed that on my system we DO enter FSEM using Xrandr
If I just comment out the call to XSendEvent(..) in X11GD_SetFullscreenMode(..)
then the capture works fine.

So something about this code, or pipewire or XWayland isn't handling this properly.

I found this
flatpak/xdg-desktop-portal#610
which linked to this
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1914

but it was supposed to be fixed a while ago ..

azuev-java

Looks good, seems to be working reasonably well on Ubuntu 20 and 22.

….java in headless environment

prsadhuk

Mostly alignment issue with couple of code comments..

prsadhuk · 2023-06-07T04:44:44Z

src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java

-                            AccessController.doPrivileged(
-                                    new GetPropertyAction("awt.robot.gtk", "true")
-                            ));
+                AccessController.doPrivileged(


Code formatting..better to align AccessController below Boolean

I moved a little, but with indent.
Compared to strictly under the Boolean, for my taste it is easier to quickly parse with the eyes, and it does not visually merge.

prsadhuk · 2023-06-07T04:44:50Z

src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java

+                        "awt.robot.screenshotMethod",
+                        isOnWayland
+                                ? METHOD_SCREENCAST
+                                : METHOD_X11


code alignment issue...will look better if all starts in same column

src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java

src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.h

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c

prsadhuk · 2023-06-07T05:23:34Z

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c

+
+                if ((*env)->ExceptionCheck(env)) {
+                    (*env)->ExceptionDescribe(env);
+                    (*env)->ExceptionClear(env);


Actually, these checks are not needed here at all, this is an oversight, there was a Java function call before.
The check after ReleaseIntArrayElements is also unnecessary.

azvegint · 2023-06-07T13:06:10Z

/integrate

openjdk · 2023-06-07T13:08:21Z

Going to push as commit 9d7bf53.
Since your change was applied there have been 254 commits pushed to the master branch:

a1ab377: 8309550: jdk.jfr.internal.Utils::formatDataAmount method should gracefully handle amounts equal to Long.MIN_VALUE
c49129f: 8308445: Linker should check that capture state segment is big enough
fa79111: 8308031: Linkers should reject unpromoted variadic parameters
16ebf47: 8309594: Cleanup naming in JavacParser related to unnamed classes
5722903: 8307374: Add a JFR event for tracking RSS
1de40f3: 8302145: ddepth should be uint in PhaseIdealLoop::register_node()
a6726b6: 8309568: javac crashes attempting to -Xprint on a class file of an unnamed class
8cdd95e: 8305959: x86: Improve itable_stub
9233dcc: 8309297: Adjust ShenandoahHeap print_heap_regions_on
749d480: 8305763: Parsing a URI with an underscore goes through a silent exception, negatively impacting performance
... and 244 more: https://git.openjdk.org/jdk/compare/8474e693b4404ba62927fe0e43e68b904d66fbde...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-07T13:08:36Z

@azvegint Pushed as commit 9d7bf53.

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

azvegint added 2 commits May 4, 2023 16:06

add pipewire header files

c0856d6

main changes

8665e98

azvegint mentioned this pull request May 4, 2023

8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots #13804

Closed

4 tasks

openjdk bot changed the title ~~8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots~~ 8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots May 4, 2023

openjdk bot added the csr Pull request needs approved CSR before integration label May 4, 2023

openjdk bot added build build-dev@openjdk.org client client-libs-dev@openjdk.org labels May 4, 2023

replace tabs with spaces

94fcd91

openjdk bot added the rfr Pull request is ready for review label May 4, 2023

azvegint mentioned this pull request May 4, 2023

8307779: Relax the java.awt.Robot specification #13809

Closed

4 tasks

erikj79 reviewed May 4, 2023

View reviewed changes

make/modules/java.desktop/lib/Awt2dLibraries.gmk Outdated Show resolved Hide resolved

mkartashev reviewed May 5, 2023

View reviewed changes

azvegint added 3 commits May 5, 2023 14:28

BUILD_LIBPIPEWIRE_HEADER_DIRS -> LIBPIPEWIRE_HEADER_DIRS

8756731

remove wayland detection

7085693

update, based on review comments

482471d

refactoring to remove resetScreenCapturePermission

3f01bb2

azvegint added 3 commits May 31, 2023 15:48

removing non-ascii

1c04dc0

address token storage comments

3bdcd9f

improve retVal processing

389cfd7

prrace approved these changes May 31, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 31, 2023

turbanoff reviewed May 31, 2023

View reviewed changes

make/modules/java.desktop/lib/Awt2dLibraries.gmk Outdated Show resolved Hide resolved

turbanoff reviewed May 31, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java Show resolved Hide resolved

turbanoff reviewed May 31, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java Show resolved Hide resolved

turbanoff reviewed May 31, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java Outdated Show resolved Hide resolved

turbanoff reviewed May 31, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java Outdated Show resolved Hide resolved

turbanoff reviewed May 31, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java Outdated Show resolved Hide resolved

turbanoff reviewed Jun 1, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java Show resolved Hide resolved

turbanoff reviewed Jun 1, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java Show resolved Hide resolved

turbanoff reviewed Jun 1, 2023

View reviewed changes

src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java Outdated Show resolved Hide resolved

address review comments

63b34b9

azuev-java approved these changes Jun 5, 2023

View reviewed changes

azvegint added 2 commits June 6, 2023 22:28

fix failure of javax/swing/reliability/HangDuringStaticInitialization…

9a91115

….java in headless environment

move screencast-tokens.properties to a new location

f557930

prsadhuk approved these changes Jun 7, 2023

View reviewed changes

address review comments

76c7bdd

openjdk bot added the integrated Pull request has been integrated label Jun 7, 2023

openjdk bot closed this Jun 7, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2023

8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots #13803

8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots #13803

Conversation

azvegint commented May 4, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

azvegint commented May 4, 2023

bridgekeeper bot commented May 4, 2023

openjdk bot commented May 4, 2023

openjdk bot commented May 4, 2023

mlbridge bot commented May 4, 2023 • edited Loading

Webrevs

erikj79 left a comment

Choose a reason for hiding this comment

prrace commented May 4, 2023

mrserb commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azvegint commented May 5, 2023

mrserb commented May 8, 2023 • edited Loading

azvegint commented May 9, 2023 • edited Loading

prrace commented May 9, 2023

azvegint commented May 9, 2023

prrace commented May 10, 2023

azvegint commented May 11, 2023

prrace left a comment

Choose a reason for hiding this comment

openjdk bot commented May 31, 2023 • edited Loading

prrace commented May 31, 2023 • edited Loading

prrace commented Jun 1, 2023 • edited Loading

azuev-java left a comment

Choose a reason for hiding this comment

prsadhuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azvegint commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

azvegint commented May 4, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 4, 2023 •

edited

Loading

mrserb commented May 8, 2023 •

edited

Loading

azvegint commented May 9, 2023 •

edited

Loading

openjdk bot commented May 31, 2023 •

edited

Loading

prrace commented May 31, 2023 •

edited

Loading

prrace commented Jun 1, 2023 •

edited

Loading