Bug 224704 – LibWebRTCCodecs eagerly launches the GPUProcess and always relaunches it on exit

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 426303 [details]
Patch

[Chris Dumez]

Created attachment 426304 [details]
Patch

[Chris Dumez]

Looks like WebKit2.CrashGPUProcessWhileCapturingAndCalling is failing. I will need to investigate.

[Chris Dumez]

Created attachment 426312 [details]
Patch

[Chris Dumez]

Created attachment 426314 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 426354 [details]
Patch

[Chris Dumez]

Created attachment 426355 [details]
Patch

[EWS]

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].

[youenn fablet]

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.

[Chris Dumez]

(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.

[Radar WebKit Bug Importer]

<rdar://problem/77110079>