On Wed, 6 Dec 2023 09:08: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.
>
> 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
