LibWebRTCCodecs eagerly launches the GPUProcess and always relaunches it on exit. The GPUProcess should only be (re-)launched when needed. In the case of the LibWebRTCCodecs, it seems it only needs a GPUProcess connection if it has m_decoders / m_encoders are non-empty.
Created attachment 426303 [details] Patch
Created attachment 426304 [details] Patch
Looks like WebKit2.CrashGPUProcessWhileCapturingAndCalling is failing. I will need to investigate.
Created attachment 426312 [details] Patch
Created attachment 426314 [details] Patch
Comment on attachment 426314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426314&action=review > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:172 > +void LibWebRTCCodecs::ensureGPUProcessConnectionOnMainThread(Locker<Lock>&) Just idly wondering: Would you want to add something like this? ASSERT_UNUSED(locker, locker.lockable() == &m_connectionLock); Adding that would require adding a lockable() member function to the Locker class template. > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:496 > + std::exchange(m_connection, nullptr)->removeThreadMessageReceiver(Messages::LibWebRTCCodecs::messageReceiverName()); Is there a firm guarantee that m_connection is always non-null here? No race or edge case where it could be null? > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.h:68 > + static Ref<LibWebRTCCodecs> create() > { > - auto instance = std::unique_ptr<LibWebRTCCodecs>(new LibWebRTCCodecs); > - instance->startListeningForIPC(); > - return instance; > + return adoptRef(*new LibWebRTCCodecs); > } This shouldn’t be inlined. I suggest moving it out of the header file. Normally it’s better that we inline the call to the constructor, and have the call to this create function be a normal function call. And it also reduces churn in the header file a little.
Comment on attachment 426314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426314&action=review >> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:172 >> +void LibWebRTCCodecs::ensureGPUProcessConnectionOnMainThread(Locker<Lock>&) > > Just idly wondering: Would you want to add something like this? > > ASSERT_UNUSED(locker, locker.lockable() == &m_connectionLock); > > Adding that would require adding a lockable() member function to the Locker class template. Ok >> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:496 >> + std::exchange(m_connection, nullptr)->removeThreadMessageReceiver(Messages::LibWebRTCCodecs::messageReceiverName()); > > Is there a firm guarantee that m_connection is always non-null here? No race or edge case where it could be null? gpuProcessConnectionDidClose() can only get called if we registered ourselves as a client of the connection. This happens in ensureGPUProcessConnectionOnMainThread() and we initialize m_connection then. m_connection is only cleared here in gpuProcessConnectionDidClose() and gpuProcessConnectionDidClose() can only be called one time per connection. I believe this is safe. >> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.h:68 >> } > > This shouldn’t be inlined. I suggest moving it out of the header file. Normally it’s better that we inline the call to the constructor, and have the call to this create function be a normal function call. And it also reduces churn in the header file a little. OK.
Created attachment 426354 [details] Patch
Created attachment 426355 [details] Patch
Committed r276214 (236696@main): <https://commits.webkit.org/236696@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426355 [details].
This patch probably introduces some races, for instance: - creation of the encoder is now sometimes doing: webrtc thread -> main thread -> codec thread. - deletion of the encoder is doing: webrtc thread -> codec thread. We might sometimes delete the encoder before it is actually added in the map. Ditto for decoder creation/deletion. The same applies for init of the encoder: - creation of the encoder is now doing: webrtc thread -> main thread -> codec thread. - initialisation of the encoder is doing: webrtc thread -> encoder thread. We should probably fix these issues. Simplest approach is probably to always do "webrtc thread -> main thread -> codec thread" for encoder creation/initialization/deletion and decoder creation/deletion. We also widen the interval where we might miss decoding/encoding frames due to connection not be set yet. This is pre-existing and is probably fine since this might not happen often in practice.
(In reply to youenn fablet from comment #11) > This patch probably introduces some races, for instance: > - creation of the encoder is now sometimes doing: webrtc thread -> main > thread -> codec thread. > - deletion of the encoder is doing: webrtc thread -> codec thread. > We might sometimes delete the encoder before it is actually added in the map. > Ditto for decoder creation/deletion. > > The same applies for init of the encoder: > - creation of the encoder is now doing: webrtc thread -> main thread -> > codec thread. > - initialisation of the encoder is doing: webrtc thread -> encoder thread. > > We should probably fix these issues. > Simplest approach is probably to always do "webrtc thread -> main thread -> > codec thread" for encoder creation/initialization/deletion and decoder > creation/deletion. > > We also widen the interval where we might miss decoding/encoding frames due > to connection not be set yet. > This is pre-existing and is probably fine since this might not happen often > in practice. Thanks for checking the patch and finding this out. I agree this is racy. I will follow-up shortly.
<rdar://problem/77110079>