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

8313697: [XWayland][Screencast] consequent getPixelColor calls are slow #15250

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import java.util.Timer;
import java.util.TimerTask;
import java.util.stream.IntStream;

/**
Expand All @@ -54,6 +56,13 @@ public class ScreencastHelper {
private static final int DENIED = -11;
private static final int OUT_OF_BOUNDS = -12;

private static final int DELAY_BEFORE_SESSION_CLOSE = 2000;

private static volatile TimerTask timerTask = null;
private static final Timer timerCloseSession
= new Timer("auto-close screencast session", true);


private ScreencastHelper() {
}

Expand Down Expand Up @@ -105,11 +114,30 @@ private static List<Rectangle> getSystemScreensBounds() {
).toList();
}

private static synchronized native void closeSession();

private static void timerCloseSessionRestart() {
if (timerTask != null) {
timerTask.cancel();
}

timerTask = new TimerTask() {
@Override
public void run() {
closeSession();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happen if the "closeSession" is executed why the new Robot#getRGBPixels is called? In the robot code we call timerCloseSessionRestart to restart/cancel timer, but I think it could be possible that closeSession() is already executed and may close session right before it will be used by the robot but after we "open" it.

Or this case is not possible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both ScreencastHelper#getRGBPixels(Robot#getRGBPixels calls it internally) and ScreencastHelper#closeSession are synchronized.
So ScreencastHelper#closeSession will wait until the execution of ScreencastHelper#getRGBPixels is finished and vice versa.

Many hours of testing with random delays close to DELAY_BEFORE_SESSION_CLOSE worked fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java part looks good to me. Why 2s and not more or less ?
The delay could be set through a system property ?

Copy link
Member Author

@azvegint azvegint Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 2s and not more or less ?

This is the average value chosen so as not to slow down tests execution, and not to annoy the user with the "Screen is being shared" icon image while the session is still open.

The delay could be set through a system property ?

This functionality could be added, but I don't see much reason to do so.
If a screenshot is taken from time to time, the benefit of holding the session for a long time will be quite small compared to frequent requests.
Plus it requires some CSR work.

}
};

timerCloseSession.schedule(timerTask, DELAY_BEFORE_SESSION_CLOSE);
}

public static synchronized void getRGBPixels(
int x, int y, int width, int height, int[] pixelArray
) {
if (!IS_NATIVE_LOADED) return;

timerCloseSessionRestart();

Rectangle captureArea = new Rectangle(x, y, width, height);

List<Rectangle> affectedScreenBounds = getSystemScreensBounds()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,10 @@ static Set<TokenItem> getTokens(List<Rectangle> affectedScreenBounds) {
return tokenItem;
})
.filter(Objects::nonNull)
.sorted((t1, t2) -> //Token with more screens preferred
t2.allowedScreensBounds.size()
- t1.allowedScreensBounds.size()
)
.toList();
}

Expand Down
2 changes: 2 additions & 0 deletions src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,7 @@ GtkApi* gtk3_load(JNIEnv *env, const char* lib_name)

fp_g_string_new = dl_symbol("g_string_new");
fp_g_string_erase = dl_symbol("g_string_erase");
fp_g_string_set_size = dl_symbol("g_string_set_size");
fp_g_string_free = dl_symbol("g_string_free");

glib_version_2_68 = !fp_glib_check_version(2, 68, 0);
Expand Down Expand Up @@ -3110,6 +3111,7 @@ static void gtk3_init(GtkApi* gtk) {

gtk->g_string_new = fp_g_string_new;
gtk->g_string_erase = fp_g_string_erase;
gtk->g_string_set_size = fp_g_string_set_size;
gtk->g_string_free = fp_g_string_free;
gtk->g_string_replace = fp_g_string_replace;
gtk->g_string_printf = fp_g_string_printf;
Expand Down
3 changes: 3 additions & 0 deletions src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,9 @@ static GString *(*fp_g_string_erase)(GString *string,
gssize pos,
gssize len);

static GString *(*fp_g_string_set_size)(GString* string,
gsize len);

static gchar *(*fp_g_string_free)(GString *string,
gboolean free_segment);

Expand Down
4 changes: 4 additions & 0 deletions src/java.desktop/unix/native/libawt_xawt/awt/gtk_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,10 @@ typedef struct GtkApi {
gssize pos,
gssize len);

GString *(*g_string_set_size)(GString* string,
gsize len);


gchar *(*g_string_free)(GString *string,
gboolean free_segment);

Expand Down
150 changes: 103 additions & 47 deletions src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ int DEBUG_SCREENCAST_ENABLED = FALSE;
(*env)->ExceptionDescribe(env); \
}

static volatile gboolean sessionClosed = TRUE;
static GString *activeSessionToken;

struct ScreenSpace screenSpace = {0};
static struct PwLoopData pw = {0};

Expand Down Expand Up @@ -89,8 +92,8 @@ static void doCleanup() {
struct ScreenProps *screenProps = &screenSpace.screens[i];
if (screenProps->data) {
if (screenProps->data->stream) {
fp_pw_thread_loop_lock(pw.loop);
fp_pw_stream_disconnect(screenProps->data->stream);
fp_pw_thread_loop_lock(pw.loop);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving pw_stream_disconnect call out of this lock fixes JDK-8310334

fp_pw_stream_destroy(screenProps->data->stream);
fp_pw_thread_loop_unlock(pw.loop);
screenProps->data->stream = NULL;
Expand Down Expand Up @@ -123,7 +126,11 @@ static void doCleanup() {
if (screenSpace.screens) {
free(screenSpace.screens);
screenSpace.screens = NULL;
screenSpace.screenCount = 0;
}

gtk->g_string_set_size(activeSessionToken, 0);
sessionClosed = TRUE;
}

/**
Expand All @@ -132,6 +139,24 @@ static void doCleanup() {
static gboolean initScreencast(const gchar *token,
GdkRectangle *affectedBounds,
gint affectedBoundsLength) {
gboolean isSameToken = !token
? FALSE
: strcmp(token, activeSessionToken->str) == 0;

if (!sessionClosed) {
if (isSameToken) {
DEBUG_SCREENCAST("Reusing active session.\n", NULL);
return TRUE;
} else {
DEBUG_SCREENCAST(
"Active session has a different token |%s| -> |%s|,"
" closing current session.\n",
activeSessionToken->str, token
);
doCleanup();
}
}

fp_pw_init(NULL, NULL);

pw.pwFd = RESULT_ERROR;
Expand All @@ -145,6 +170,8 @@ static gboolean initScreencast(const gchar *token,
return FALSE;
}

gtk->g_string_printf(activeSessionToken, "%s", token);
sessionClosed = FALSE;
return TRUE;
}

Expand Down Expand Up @@ -386,8 +413,19 @@ static gboolean connectStream(int index) {

data->screenProps = &screenSpace.screens[index];

data->hasFormat = FALSE;
if (!sessionClosed && data->stream) {
fp_pw_thread_loop_lock(pw.loop);
int result = fp_pw_stream_set_active(data->stream, TRUE);
fp_pw_thread_loop_unlock(pw.loop);

DEBUG_SCREEN_PREFIX(data->screenProps,
"stream %p: activate result |%i|\n",
data->stream, result);

return result == 0; // 0 - success
};

data->hasFormat = FALSE;

data->stream = fp_pw_stream_new(
pw.core,
Expand Down Expand Up @@ -505,60 +543,64 @@ static const struct pw_core_events coreEvents = {
* @return TRUE on success
*/
static gboolean doLoop(GdkRectangle requestedArea) {
pw.loop = fp_pw_thread_loop_new("AWT Pipewire Thread", NULL);

if (!pw.loop) {
DEBUG_SCREENCAST("!!! Could not create a loop\n", NULL);
doCleanup();
return FALSE;
}

pw.context = fp_pw_context_new(
fp_pw_thread_loop_get_loop(pw.loop),
NULL,
0
);
if (!pw.loop && !sessionClosed) {
pw.loop = fp_pw_thread_loop_new("AWT Pipewire Thread", NULL);

if (!pw.context) {
DEBUG_SCREENCAST("!!! Could not create a pipewire context\n", NULL);
doCleanup();
return FALSE;
}
if (!pw.loop) {
DEBUG_SCREENCAST("!!! Could not create a loop\n", NULL);
doCleanup();
return FALSE;
}

if (fp_pw_thread_loop_start(pw.loop) != 0) {
DEBUG_SCREENCAST("!!! Could not start pipewire thread loop\n", NULL);
doCleanup();
return FALSE;
}
pw.context = fp_pw_context_new(
fp_pw_thread_loop_get_loop(pw.loop),
NULL,
0
);

fp_pw_thread_loop_lock(pw.loop);
if (!pw.context) {
DEBUG_SCREENCAST("!!! Could not create a pipewire context\n", NULL);
doCleanup();
return FALSE;
}

pw.core = fp_pw_context_connect_fd(
pw.context,
pw.pwFd,
NULL,
0
);
if (fp_pw_thread_loop_start(pw.loop) != 0) {
DEBUG_SCREENCAST("!!! Could not start pipewire thread loop\n", NULL);
doCleanup();
return FALSE;
}

if (!pw.core) {
DEBUG_SCREENCAST("!!! Could not create pipewire core\n", NULL);
goto fail;
}
fp_pw_thread_loop_lock(pw.loop);

pw_core_add_listener(pw.core, &pw.coreListener, &coreEvents, NULL);
pw.core = fp_pw_context_connect_fd(
pw.context,
pw.pwFd,
NULL,
0
);

for (int i = 0; i < screenSpace.screenCount; ++i) {
struct PwStreamData *data =
(struct PwStreamData*) malloc(sizeof (struct PwStreamData));
if (!data) {
ERR("failed to allocate memory\n");
if (!pw.core) {
DEBUG_SCREENCAST("!!! Could not create pipewire core\n", NULL);
goto fail;
}

memset(data, 0, sizeof (struct PwStreamData));
pw_core_add_listener(pw.core, &pw.coreListener, &coreEvents, NULL);
}

for (int i = 0; i < screenSpace.screenCount; ++i) {
struct ScreenProps *screen = &screenSpace.screens[i];
screen->data = data;
if (!screen->data && !sessionClosed) {
struct PwStreamData *data =
(struct PwStreamData*) malloc(sizeof (struct PwStreamData));
if (!data) {
ERR("failed to allocate memory\n");
goto fail;
}

memset(data, 0, sizeof (struct PwStreamData));

screen->data = data;
}

DEBUG_SCREEN_PREFIX(screen, "@@@ adding screen %i\n", i);
if (checkScreen(i, requestedArea)) {
Expand Down Expand Up @@ -746,6 +788,8 @@ JNIEXPORT jboolean JNICALL Java_sun_awt_screencast_ScreencastHelper_loadPipewire
return JNI_FALSE;
}

activeSessionToken = gtk->g_string_new("");

gboolean usable = initXdgDesktopPortal();
portalScreenCastCleanup();
return usable;
Expand Down Expand Up @@ -783,6 +827,17 @@ static void arrayToRectangles(JNIEnv *env,
(*env)->ReleaseIntArrayElements(env, boundsArray, body, 0);
}

/*
* Class: sun_awt_screencast_ScreencastHelper
* Method: closeSession
* Signature: ()V
*/
JNIEXPORT void JNICALL
Java_sun_awt_screencast_ScreencastHelper_closeSession(JNIEnv *env, jclass cls) {
DEBUG_SCREENCAST("closing screencast session\n\n", NULL);
doCleanup();
}

/*
* Class: sun_awt_screencast_ScreencastHelper
* Method: getRGBPixelsImpl
Expand All @@ -805,7 +860,7 @@ JNIEXPORT jint JNICALL Java_sun_awt_screencast_ScreencastHelper_getRGBPixelsImpl
boundsLen = (*env)->GetArrayLength(env, affectedScreensBoundsArray);
EXCEPTION_CHECK_DESCRIBE();
if (boundsLen % 4 != 0) {
DEBUG_SCREENCAST("%s:%i incorrect array length\n", __FUNCTION__, __LINE__);
DEBUG_SCREENCAST("incorrect array length\n", NULL);
return RESULT_ERROR;
}
affectedBoundsLength = boundsLen / 4;
Expand Down Expand Up @@ -896,11 +951,12 @@ JNIEXPORT jint JNICALL Java_sun_awt_screencast_ScreencastHelper_getRGBPixelsImpl

fp_pw_thread_loop_lock(pw.loop);
fp_pw_stream_set_active(screenProps->data->stream, FALSE);
fp_pw_stream_disconnect(screenProps->data->stream);
fp_pw_thread_loop_unlock(pw.loop);

screenProps->captureDataReady = FALSE;
}
}
doCleanup();

releaseToken(env, jtoken, token);
return 0;
}
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,10 @@ int portalScreenCastOpenPipewireRemote() {
}

void portalScreenCastCleanup() {
if (!portal) {
return;
}

if (portal->screenCastSessionHandle) {
gtk->g_dbus_connection_call_sync(
portal->connection,
Expand All @@ -796,9 +800,6 @@ void portalScreenCastCleanup() {
portal->screenCastSessionHandle = NULL;
}

if (!portal) {
return;
}
if (portal->connection) {
gtk->g_object_unref(portal->connection);
portal->connection = NULL;
Expand Down