On Tue, 5 Dec 2023 20:22:32 GMT, Phil Race <p...@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
@prrace have you read my description of this PR? If so, and based on that, what
can you propose to address the problem described?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16978#discussion_r1416258184
