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/classes/sun/awt/screencast/ScreencastHelper.java line 223:
> 221: } else if (retVal == ERROR) {
> 222: throw new RuntimeException(
> 223: "Screen capture has failed due to desktop environment
> error");
Please remove this
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16978#discussion_r1416236955
