On Fri, 1 Dec 2023 14:26:39 GMT, Anton Bobrov <d...@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