RFR: 8321176: [Screencast] make a second attempt on screencast failure [v2]

[Phil Race]

On Wed, 6 Dec 2023 09:08:55 GMT, Anton Bobrov <duke at 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.
>
> Anton Bobrov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8321176: remove RuntimeException and address review comments

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 102:

> 100:             if (screenProps->data->stream) {
> 101:                 fp_pw_thread_loop_lock(pw.loop);
> 102:                 fp_pw_stream_disconnect(screenProps->data->stream);

I had this disconnect inside the lock in an earlier PR 
https://github.com/openjdk/jdk/pull/14428/files?diff=unified&w=0#diff-3ba5df79cdd3da36b21bf29b4e8de462dd61e6a21dfe0816e4d84c18bbfb76b2R92
but then it was moved out of the lock in another PR to fix a bug https://bugs.openjdk.org/browse/JDK-8310334
https://github.com/openjdk/jdk/pull/15250/files#diff-3ba5df79cdd3da36b21bf29b4e8de462dd61e6a21dfe0816e4d84c18bbfb76b2


So how can it be right to put it back inside the lock ?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16978#discussion_r1417874272