On Fri, 1 Dec 2023 09:34:35 GMT, Anton Bobrov <d...@openjdk.org> wrote:
>> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 930: >> >>> 928: fp_pw_thread_loop_unlock(pw.loop); >>> 929: releaseToken(env, jtoken, token); >>> 930: return RESULT_ERROR; >> >> I think that the fix can be improved a bit. >> Right now it just gives a black image on failure. We can try to add another >> attempt to get the image we need. > > @azvegint Thanks for reviewing this Alexander! I dont think re-trying here is > gonna work at all. The libpipewire docs leave a lot to be desired so I was > using their code as reference for this patch and according to their current > implementation if it gets to a state of core error or if it transitions to > PW_STREAM_STATE_ERROR or PW_STREAM_STATE_UNCONNECTED from any other state > that means a fatal (unrecoverable) error meaning you have to bail and start > from scratch eg re-init, get new stream etc ie there is no transition from > such state to a normal state where you could make a re-try. > > The problem with blank screen in this case stems from the API design here. > Currently, when an error here the higher level API, as far as I can tell, > only covers the permissions case by throwing security exception but > completely ignores any other possible errors instead of propagating them to > the caller, either as exception or return value, and so the caller gets an > empty/black image on failure but is simply not aware of any encountered > error/s and thus could incorrectly assume the data is legit. > > What should be done to address that is to propagate those errors all the way > back to the API caller but doing so would require changing related API method > signatures or introducing new throws that the callers would need to catch. I > dunno about the stability levels and contracts on those API involved but if > they are mendable I can look into changing them tho I think it is better to > do it as separate patch. Tell me what you think. Sorry for not being clear, I meant by retry to start everything with a new session. And of course, changing the internal API is not a problem as it is quite small and only used in one place. Separate patch is fine, I submitted [JDK-8321176](https://bugs.openjdk.org/browse/JDK-8321176) for this, I can do it, but later. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16794#discussion_r1412138087
