On Tue, 5 Dec 2023 16:48:55 GMT, Anton Bobrov <d...@openjdk.org> wrote:
> This patch adds re-try logic to libpipewire screencast error handling as
> discussed in PR #16794 and also brings some additional error handling and
> thread safety improvements. Specifically around cleanup order where incorrect
> ordering lead to native memory corruption issues, and lock/unlock accounting
> that while mostly harmless (with the current libpipewire implementation) did
> pollute the stderr on jtreg tests, making some tests (expecting clean stderr)
> to fail.
>
> The real major change here however is the throw of the RuntimeException which
> can propagate to public
>
> java.awt.Robot. createMultiResolutionScreenCapture, createScreenCapture,
> getPixelColor methods. I'm not sure the plain RuntimeException is the way to
> go here so this is just a placeholder of sorts. A separate/specific runtime
> exception can be created for this BUT *something* needs to be done here as
> the current implementation fails to convey libpipewire failures to those
> public API callers and since they have no way of detecting such errors
> otherwise they have no way of knowing that the data returned by those API is
> in fact invalid (eg black screens etc). The reason for using an unchecked
> exception here is driven mainly by the following factors:
>
> 1) Since those are public API, their contracts can potentially make it
> difficult to introduce specific additional checked exceptions or return
> values (as appropriate) as those could potentially break existing API use.
>
> 2) The libpipewire errors of that kind are rare and usually indicate there is
> something wrong with the desktop stack eg some fatal configuration or run
> time error that is normally not supposed to happen and given this patch now
> goes extra step re-trying on such failures it stands to reason runtime
> unchecked exception makes sense when that fails as well.
>
> 3) Creating checked exceptions for such specific native implementation
> dependent errors and propagating such exceptions thru the call tree does not
> make much sense as most public API users won't even know how to handle them
> without knowing native implementation specifics.
src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 26:
> 24: */
> 25:
> 26: #include "debug_util.h"
Looks like this header is not used.
src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 891:
> 889: if (hasPipewireFailed) {
> 890: doCleanup();
> 891: fp_pw_thread_loop_unlock(pw.loop);
All other `doCleanup` calls are made without the lock, let's be consistent.
src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 959:
> 957: token, &requestedArea, affectedScreenBounds,
> affectedBoundsLength);
> 958:
> 959: if (attemptResult) {
I don't think we should make another attempt if there is no saved token and the
user rejected the first screen capture request:
Suggestion:
if (attemptResult) {
if (attemptResult == RESULT_DENIED) {
releaseToken(env, jtoken, token);
return RESULT_DENIED;
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16978#discussion_r1416611308
PR Review Comment: https://git.openjdk.org/jdk/pull/16978#discussion_r1416612933
PR Review Comment: https://git.openjdk.org/jdk/pull/16978#discussion_r1416618136
