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

[Anton Bobrov]

On Fri, 1 Dec 2023 15:00:50 GMT, Alexander Zvegintsev <azvegint at openjdk.org> wrote:

>> @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, affectedBoundsLengt...

@azvegint let me have a look next week and i'll try to come up with another PR for this. the pipewire is a bit tricky to test in my experience as there are quite a few moving parts/layers there and on some error conditions it even hangs bc they chose to use no timeouts in their implementation so it just gets stuck there forever sometimes, so i would need to come up with some way/s to test this.

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

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