RFR: 8320655: awt screencast robot spin and sync issues with native libpipewire api

[Alexander Zvegintsev]

On Fri, 1 Dec 2023 14:26:39 GMT, Anton Bobrov <duke at openjdk.org> wrote:

>> Sorry for not being clear, I meant by retry to start everything with a new session.
>> And of course, changing the internal API is not a problem as it is quite small and only used in one place.
>> 
>> Separate patch is fine, I submitted [JDK-8321176](https://bugs.openjdk.org/browse/JDK-8321176) for this, I can do it, but later.
>
> @azvegint do you think it would make sense to do it in the native code or in the upper level Java code ? AND also how many times do you think it would make sense to retry provided this could be a permanent pipewire error and so it wont be sitting there re-trying with the same error forever ?  (EDIT): i see you have mentioned in the bug just 1 retry. i think it would still make sense to do something about the case where even the 2nd attempt does not succeed ie notify the caller somehow (retval/exception) that it has failed.

I think it's better to do it in the native, to avoid extra native-java hops.

As for the number of extra attempts, I think one-two is enough.

Something like this (I haven't tested it thoroughly yet):


diff --git a/src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c b/src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c
index 6a7c4124935..a7c5aadb1ae 100644
--- a/src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c
+++ b/src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c
@@ -855,6 +855,32 @@ static void arrayToRectangles(JNIEnv *env,
     (*env)->ReleaseIntArrayElements(env, boundsArray, body, 0);
 }
 
+static int makeAnAttempt(
+        const gchar *token,
+        GdkRectangle *requestedArea,
+        GdkRectangle *affectedScreenBounds,
+        gint affectedBoundsLength) {
+    if (!initScreencast(token, affectedScreenBounds, affectedBoundsLength)) {
+        return pw.pwFd;
+    }
+
+    if (!doLoop(*requestedArea)) {
+        return RESULT_ERROR;
+    }
+
+    while (!isAllDataReady()) {
+        fp_pw_thread_loop_lock(pw.loop);
+        fp_pw_thread_loop_wait(pw.loop);
+        if (hasPipewireFailed) {
+            fp_pw_thread_loop_unlock(pw.loop);
+            doCleanup();
+            return RESULT_ERROR;
+        }
+        fp_pw_thread_loop_unlock(pw.loop);
+    }
+    return RESULT_OK;
+}
+
 /*
  * Class:     sun_awt_screencast_ScreencastHelper
  * Method:    closeSession
@@ -911,25 +937,17 @@ JNIEXPORT jint JNICALL Java_sun_awt_screencast_ScreencastHelper_getRGBPixelsImpl
             jx, jy, jwidth, jheight, token
     );
 
-    if (!initScreencast(token, affectedScreenBounds, affectedBoundsLength)) {
-        releaseToken(env, jtoken, token);
-        return pw.pwFd;
-    }
-
-    if (!doLoop(requestedArea)) {
-        releaseToken(env, jtoken, token);
-        return RESULT_ERROR;
-    }
+    int attemptResult = makeAnAttempt(token, &requestedArea,
+                                      affectedScreenBounds, affectedBoundsLength);
 
-    while (!isAllDataReady()) {
-        fp_pw_thread_loop_lock(pw.loop);
-        fp_pw_thread_loop_wait(pw.loop);
-        if (hasPipewireFailed) {
-            fp_pw_thread_loop_unlock(pw.loop);
+    if (attemptResult) {
+        DEBUG_SCREENCAST("First attempt failed with %i, re-trying...\n", attemptResult);
+        attemptResult = makeAnAttempt(token, &requestedArea,
+                                      affectedScreenBounds, affectedBoundsLength);
+        if (attemptResult) {
             releaseToken(env, jtoken, token);
-            return RESULT_ERROR;
+            return attemptResult;
         }
-        fp_pw_thread_loop_unlock(pw.loop);
     }
 
     DEBUG_SCREENCAST("\nall data ready\n", NULL);

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16794#discussion_r1412217992