Bug 222888 - [GStreamer] CaptureDevice monitor used from UIProcess
Summary: [GStreamer] CaptureDevice monitor used from UIProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks: 192697 222889
  Show dependency treegraph
 
Reported: 2021-03-07 09:18 PST by Philippe Normand
Modified: 2021-04-13 01:39 PDT (History)
12 users (show)

See Also:


Attachments
Patch (5.47 KB, patch)
2021-03-07 10:56 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (6.43 KB, patch)
2021-03-23 07:58 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (6.71 KB, patch)
2021-03-23 09:47 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (6.87 KB, patch)
2021-03-28 06:34 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (15.92 KB, patch)
2021-03-29 10:55 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (19.42 KB, patch)
2021-04-07 09:10 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (19.41 KB, patch)
2021-04-08 06:15 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (19.33 KB, patch)
2021-04-12 09:03 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (19.08 KB, patch)
2021-04-12 09:40 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (18.86 KB, patch)
2021-04-12 09:42 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2021-03-07 09:18:08 PST
If the webapp starts listening on the devicechange event on the MediaDevice (hello jitsi), a RELEASE_ASSERT is hit, because the UserMediaPermissionRequestManager (WebProcess) sends a BeginMonitoringCaptureDevices message to the UIProcess...

(gdb) bt
#0  0x00007f1c1adf394e in WTFCrash() () at ../../Source/WTF/wtf/Assertions.cpp:295
#1  0x00007f1c277f1b6b in WTFCrashWithInfo(int, char const*, char const*, int) () at DerivedSources/ForwardingHeaders/wtf/Assertions.h:671
#2  0x00007f1c2945df7b in WebCore::ensureGStreamerInitialized() () at ../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:242
#3  0x00007f1c2d50d119 in WebCore::GStreamerCaptureDeviceManager::captureDevices() (this=0x7f1c34089080 <WebCore::GStreamerAudioCaptureDeviceManager::singleton()::manager>)
    at ../../Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.cpp:90
#4  0x00007f1c2c22ea24 in WebCore::CaptureDeviceManager::getCaptureDevices(WTF::CompletionHandler<void (WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)>&&)
    (this=0x7f1c34089080 <WebCore::GStreamerAudioCaptureDeviceManager::singleton()::manager>, callback=...) at ../../Source/WebCore/platform/mediastream/CaptureDeviceManager.h:38
#5  0x00007f1c2c241461 in WebCore::RealtimeMediaSourceCenter::getMediaStreamDevices(WTF::CompletionHandler<void (WTF::Vector<WebCore::CaptureDevice, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)>&&)
    (this=0x7f1c3407e4f0 <WebCore::RealtimeMediaSourceCenter::singleton()::center>, completion=...) at ../../Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:153
#6  0x00007f1c2875593d in WebKit::UserMediaProcessManager::updateCaptureDevices(WebKit::UserMediaProcessManager::ShouldNotify)
    (this=0x7f1c34060400 <WebKit::UserMediaProcessManager::singleton()::manager>, shouldNotify=WebKit::UserMediaProcessManager::ShouldNotify::No) at ../../Source/WebKit/UIProcess/UserMediaProcessManager.cpp:222
#7  0x00007f1c287698a4 in WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27::operator()() const (this=0x7ffd8ca5a5b0) at ../../Source/WebKit/UIProcess/UserMediaProcessManager.cpp:261
#8  0x00007f1c2876986d in std::__invoke_impl<void, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::__invoke_other, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&) (__f=...) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/invoke.h:60
#9  0x00007f1c2876982d in std::__invoke<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&) (__fn=...)
    at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/invoke.h:95
#10 0x00007f1c28769800 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&)::{lambda()#1}::operator()() const (this=0x7ffd8ca5a588) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:717
#11 0x00007f1c287697d7 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&)::{lambda()#2}::operator()() const (this=0x3) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:722
#12 0x00007f1c287697a9 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&)::{lambda()#2}::__invoke() () at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:722
#13 0x00007f1c14d34aaf in __pthread_once_slow (once_control=0x7f1c34060450 <WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::onceFlag>, init_routine=0x7f1c136bd810 <std::__once_proxy()>)
    at pthread_once.c:116
#14 0x00007f1c2876977b in __gthread_once(int*, void (*)()) (__once=0x7f1c34060450 <WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::onceFlag>, __func=0x7f1c136bd810 <std::__once_proxy()>)
    at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/x86_64-unknown-linux-gnu/bits/gthr-default.h:700
#15 0x00007f1c28755a68 in std::call_once<WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27>(std::once_flag&, WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices()::$_27&&)
    (__once=..., __f=...) at /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../include/c++/10.2.0/mutex:729
#16 0x00007f1c287559e4 in WebKit::UserMediaProcessManager::beginMonitoringCaptureDevices() (this=0x7f1c34060400 <WebKit::UserMediaProcessManager::singleton()::manager>)
    at ../../Source/WebKit/UIProcess/UserMediaProcessManager.cpp:260
#17 0x00007f1c287dbaba in WebKit::WebPageProxy::beginMonitoringCaptureDevices() (this=0x7f07bc0f9000) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:8228
#18 0x00007f1c27be7b84 in IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(), std::tuple<>>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(), std::tuple<>&&, std::integer_sequence<unsigned long>) (object=0x7f07bc0f9000, function=(void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const)) 0x7f1c287dba90 <WebKit::WebPageProxy::beginMonitoringCaptureDevices()>, args=...)
    at ../../Source/WebKit/Platform/IPC/HandleMessage.h:43
#19 0x00007f1c27be7af0 in IPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(), std::tuple<>, std::integer_sequence<unsigned long> >(std::tuple<>&&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)()) (args=..., object=0x7f07bc0f9000, function=(void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const)) 0x7f1c287dba90 <WebKit::WebPageProxy::beginMonitoringCaptureDevices()>)
    at ../../Source/WebKit/Platform/IPC/HandleMessage.h:49
#20 0x00007f1c27bd9126 in IPC::handleMessage<Messages::WebPageProxy::BeginMonitoringCaptureDevices, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(IPC::Decoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)()) (decoder=..., object=0x7f07bc0f9000, function=(void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy * const)) 0x7f1c287dba90 <WebKit::WebPageProxy::beginMonitoringCaptureDevices()>)
    at ../../Source/WebKit/Platform/IPC/HandleMessage.h:121
#21 0x00007f1c27bd04bc in WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc0f9000, connection=..., decoder=...)
    at DerivedSources/WebKit/WebPageProxyMessageReceiver.cpp:1493
#22 0x00007f1c2859f481 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc2fc4b8, connection=..., decoder=...)
    at ../../Source/WebKit/Platform/IPC/MessageReceiverMap.cpp:123
#23 0x00007f1c286ea52e in WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc2fc480, connection=..., decoder=...)
    at ../../Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:221
#24 0x00007f1c288b4eef in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f07bc2fc480, connection=..., decoder=...) at ../../Source/WebKit/UIProcess/WebProcessProxy.cpp:819
#25 0x00007f1c2857976c in IPC::Connection::dispatchMessage(IPC::Decoder&) (this=0x7f1c104a8198, decoder=...) at ../../Source/WebKit/Platform/IPC/Connection.cpp:1010
#26 0x00007f1c28579c2c in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) (this=0x7f1c104a8198, message=std::unique_ptr<IPC::Decoder> = {...})
    at ../../Source/WebKit/Platform/IPC/Connection.cpp:1055
#27 0x00007f1c28578c11 in IPC::Connection::dispatchIncomingMessages() (this=0x7f1c104a8198) at ../../Source/WebKit/Platform/IPC/Connection.cpp:1159
#28 0x00007f1c2857e352 in IPC::Connection::enqueueIncomingMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >)::$_8::operator()() (this=0x7f1c104a18a8)
    at ../../Source/WebKit/Platform/IPC/Connection.cpp:977
--Type <RET> for more, q to quit, c to continue without paging--
#29 0x00007f1c2857e2fe in WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >)::$_8, void>::call() (this=0x7f1c104a18a0)
    at DerivedSources/ForwardingHeaders/wtf/Function.h:52
#30 0x00007f1c277f1332 in WTF::Function<void ()>::operator()() const (this=0x7ffd8ca5b5b8) at DerivedSources/ForwardingHeaders/wtf/Function.h:83
#31 0x00007f1c1ae2c395 in WTF::RunLoop::performWork() (this=0x7f1c104f9000) at ../../Source/WTF/wtf/RunLoop.cpp:128
#32 0x00007f1c1aec571c in WTF::RunLoop::RunLoop()::$_1::operator()(void*) const (this=0x7f1c104f9000, userData=0x7f1c104f9000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:80
#33 0x00007f1c1aec56f5 in WTF::RunLoop::RunLoop()::$_1::__invoke(void*) (userData=0x7f1c104f9000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:79
#34 0x00007f1c1aec56a9 in WTF::RunLoop::$_0::operator()(_GSource*, int (*)(void*), void*) const
    (this=0xb13450, source=0xb13450, callback=0x7f1c1aec56e0 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)>, userData=0x7f1c104f9000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:53
#35 0x00007f1c1aec4725 in WTF::RunLoop::$_0::__invoke(_GSource*, int (*)(void*), void*) (source=0xb13450, callback=0x7f1c1aec56e0 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)>, userData=0x7f1c104f9000)
    at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:45
#36 0x00007f1c3418adbf in g_main_dispatch (context=0xb31460) at ../glib/gmain.c:3337
#37 g_main_context_dispatch (context=0xb31460) at ../glib/gmain.c:4055
#38 0x00007f1c3418b168 in g_main_context_iterate (context=context@entry=0xb31460, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4131
#39 0x00007f1c3418b233 in g_main_context_iteration (context=context@entry=0xb31460, may_block=may_block@entry=1) at ../glib/gmain.c:4196
#40 0x00007f1c13be962d in g_application_run (application=0xb97210 [GtkApplication], argc=-1935296492, argv=<optimized out>) at ../gio/gapplication.c:2560
#41 0x000000000041c23d in main (argc=1, argv=0x7ffd8ca5ba18) at ../../Tools/MiniBrowser/gtk/main.c:833
Comment 1 Philippe Normand 2021-03-07 10:56:04 PST
Created attachment 422532 [details]
Patch
Comment 2 Carlos Garcia Campos 2021-03-08 00:46:16 PST
Comment on attachment 422532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422532&action=review

> Source/WebKit/UIProcess/UserMediaProcessManager.h:61
> +    DevicesChangedCallback m_devicesChangedCallback;

Maybe we should make this gstreamer only.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:40
> +#if USE(GSTREAMER)
> +#include "UserMediaProcessManager.h"
> +#endif

It's a bit weird to use UI process API from the web process directly. Maybe we should move it to Shared (not necessarily as part of this patch, though).
Comment 3 Philippe Normand 2021-03-08 01:02:16 PST
Comment on attachment 422532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422532&action=review

>> Source/WebKit/UIProcess/UserMediaProcessManager.h:61
>> +    DevicesChangedCallback m_devicesChangedCallback;
> 
> Maybe we should make this gstreamer only.

Why? Then we'd also need to ifdef the beginMonitoringCaptureDevices changes?

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:40
>> +#endif
> 
> It's a bit weird to use UI process API from the web process directly. Maybe we should move it to Shared (not necessarily as part of this patch, though).

I agree
Comment 4 Philippe Normand 2021-03-15 02:32:13 PDT
So, does this patch needs an update?
Comment 5 youenn fablet 2021-03-23 07:18:53 PDT
Comment on attachment 422532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422532&action=review

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:216
> +    m_devicesChangedCallback();

What if m_devicesChangedCallback is not set?
Might not happen in practice but still.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:163
> +        UserMediaProcessManager::singleton().beginMonitoringCaptureDevices([weakThis = makeWeakPtr(*this)]() {

Maybe you should create your own class here.
UserMediaProcessManager is in UIProcess folder.
And beginMonitoringCaptureDevices is not doing a lot of things anyway.
Maybe you could use WebCore::RealtimeMediaSourceCenter::singleton() directly
Comment 6 Philippe Normand 2021-03-23 07:58:52 PDT
Created attachment 424017 [details]
Patch
Comment 7 Alex Christensen 2021-03-23 09:10:16 PDT
Comment on attachment 424017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424017&action=review

> Source/WebKit/ChangeLog:8
> +        GStreamer ports aim to reduce usage of the GStreamer library in the UIProcess and thus

Why are you doing this?  Our view is that the web process is untrusted so we moved capturing things like this to the trusted UI process.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:163
> +    WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, shouldNotify] (auto&& newDevices) mutable {

This looks like it could use the this pointer after it is freed.

if (!weakThis)
    return;

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:71
> +    enum class ShouldNotify { Yes, No };

: bool
Comment 8 Philippe Normand 2021-03-23 09:47:15 PDT
Created attachment 424030 [details]
Patch
Comment 9 Philippe Normand 2021-03-23 09:49:50 PDT
Comment on attachment 424017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424017&action=review

>> Source/WebKit/ChangeLog:8
>> +        GStreamer ports aim to reduce usage of the GStreamer library in the UIProcess and thus
> 
> Why are you doing this?  Our view is that the web process is untrusted so we moved capturing things like this to the trusted UI process.

I expanded the explanation. We unfortunately can't move out of WebProcess yet, and we try to not initialize GStreamer from the UIProcess either, due to performance concerns on embedded platforms.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:163
>> +    WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, shouldNotify] (auto&& newDevices) mutable {
> 
> This looks like it could use the this pointer after it is freed.
> 
> if (!weakThis)
>     return;

Right, weak ptr now captured in lambda.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:71
>> +    enum class ShouldNotify { Yes, No };
> 
> : bool

I wonder why an enum was used in the UserMediaProcess now ;)
Comment 10 Philippe Normand 2021-03-28 06:34:11 PDT
Created attachment 424496 [details]
Patch
Comment 11 youenn fablet 2021-03-28 12:16:49 PDT
Comment on attachment 424496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424496&action=review

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:173
> +        if (m_captureDevices.size() == oldDevices.size()) {

We could add to CaptureDevice.h a free function like:
bool haveDevicesChanged(const Vector< CaptureDevice >& oldDevices, const Vector< CaptureDevice >& newDevices).

And RealtimeMediaSourceCenter could do the debouceTimer before calling the observer.
We would share more code with UserMediaProcessManager and rewrite this callback as:

if (! haveDevicesChanged(m_captureDevices, newDevices))
    return;
m_captureDevices = WTFMove(newDevices);
if (shouldNotify == ShouldNotify::Yes)
    captureDevicesChanged();

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:210
> +        WebCore::RealtimeMediaSourceCenter::singleton().setDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() {

UserMediaPermissionRequestManager is per page, but RealtimeMediaSourceCenter is taking only one callback so far.
This probably does not work for processes with more than one page.
Comment 12 Philippe Normand 2021-03-28 13:00:38 PDT
Comment on attachment 424496 [details]
Patch

I'll rework this then. Thanks Youenn :)
Comment 13 Philippe Normand 2021-03-29 10:55:55 PDT
Created attachment 424547 [details]
Patch
Comment 14 Philippe Normand 2021-04-02 02:46:34 PDT
Does it look better Youenn? :)
Comment 15 youenn fablet 2021-04-02 02:58:26 PDT
Comment on attachment 424547 [details]
Patch

Looks good to me, a few suggestions below.

View in context: https://bugs.webkit.org/attachment.cgi?id=424547&action=review

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:89
> +    WEBCORE_EXPORT UserMediaClient::DeviceChangeObserverToken addDevicesChangedObserver(std::function<void()>&&);

I do not remember why we use std::function. We probably want Function here.

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:205
> +    WebCore::RealtimeMediaSourceCenter::singleton().removeDevicesChangedObserver(m_deviceChangeObserverToken);

I don't think we want this one,  or we should add the observer back when enabled is set to true.
Given it is a singleton, let's register once and for all.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:181
> +        m_deviceChangeObserverToken = WebCore::RealtimeMediaSourceCenter::singleton().addDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() {

We probably want to remove m_deviceChangeObserverToken in the destructor if it is set, otherwise it will grow over time.
If we do so, we do not need to use weakThis here.

Another possibility would be to use a WeakHashSet<Observer> which would remove the explicit use of weakThis and observer removal.
Comment 16 Philippe Normand 2021-04-07 08:45:07 PDT
Comment on attachment 424547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424547&action=review

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:181
>> +        m_deviceChangeObserverToken = WebCore::RealtimeMediaSourceCenter::singleton().addDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() {
> 
> We probably want to remove m_deviceChangeObserverToken in the destructor if it is set, otherwise it will grow over time.
> If we do so, we do not need to use weakThis here.
> 
> Another possibility would be to use a WeakHashSet<Observer> which would remove the explicit use of weakThis and observer removal.

Trying to understand what you mean here, are you suggesting to wrap a new Observer class in the RealtimeSourceCenter and then make UserMediaPermissionRequestManager (in gst case) and UserMediaProcessManager inherit from it?

I´ve tried that but that triggers build errors in UserMediaPermissionRequestManager:

WTF/Headers/wtf/WeakPtr.h:249:19: error: member 'weakPtrFactory' found in multiple base classes of different types
    return object.weakPtrFactory().template createWeakPtr<T>(object);
                  ^
../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:158:87: note: in instantiation of function template specialization 'WTF::makeWeakPtr<WebKit::UserMediaPermissionRequestManager>' requested here
    WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([weakThis = makeWeakPtr(*this), this, shouldNotify](auto&& newDevices) mutable {
                                                                                      ^
WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup
    const WeakPtrFactory<T, Counter>& weakPtrFactory() const { return m_weakPtrFactory; }
                                      ^
WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup
In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-26.cpp:2:
../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:159:14: error: use of undeclared identifier 'weakThis'
        if (!weakThis)
             ^
2 errors generated.
Comment 17 Philippe Normand 2021-04-07 08:58:31 PDT
Comment on attachment 424547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424547&action=review

>>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:181
>>> +        m_deviceChangeObserverToken = WebCore::RealtimeMediaSourceCenter::singleton().addDevicesChangedObserver([weakThis = makeWeakPtr(*this)]() {
>> 
>> We probably want to remove m_deviceChangeObserverToken in the destructor if it is set, otherwise it will grow over time.
>> If we do so, we do not need to use weakThis here.
>> 
>> Another possibility would be to use a WeakHashSet<Observer> which would remove the explicit use of weakThis and observer removal.
> 
> Trying to understand what you mean here, are you suggesting to wrap a new Observer class in the RealtimeSourceCenter and then make UserMediaPermissionRequestManager (in gst case) and UserMediaProcessManager inherit from it?
> 
> I´ve tried that but that triggers build errors in UserMediaPermissionRequestManager:
> 
> WTF/Headers/wtf/WeakPtr.h:249:19: error: member 'weakPtrFactory' found in multiple base classes of different types
>     return object.weakPtrFactory().template createWeakPtr<T>(object);
>                   ^
> ../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:158:87: note: in instantiation of function template specialization 'WTF::makeWeakPtr<WebKit::UserMediaPermissionRequestManager>' requested here
>     WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([weakThis = makeWeakPtr(*this), this, shouldNotify](auto&& newDevices) mutable {
>                                                                                       ^
> WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup
>     const WeakPtrFactory<T, Counter>& weakPtrFactory() const { return m_weakPtrFactory; }
>                                       ^
> WTF/Headers/wtf/WeakPtr.h:200:39: note: member found by ambiguous name lookup
> In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-26.cpp:2:
> ../../Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:159:14: error: use of undeclared identifier 'weakThis'
>         if (!weakThis)
>              ^
> 2 errors generated.

Ok, trying to decouple the observer from the UserMediaPermissionRequestManager class might work.
Comment 18 Philippe Normand 2021-04-07 09:10:08 PDT
Created attachment 425406 [details]
Patch
Comment 19 Philippe Normand 2021-04-08 06:15:05 PDT
Created attachment 425503 [details]
Patch
Comment 20 youenn fablet 2021-04-12 07:47:16 PDT
Comment on attachment 425503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425503&action=review

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:71
> +}

I would use = default for consistency with above destructor.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:64
> +        virtual void devicesChanged() { }

I would make it virtual pure since there is no point for an observe with devicesChanged being a no-op

> Source/WebKit/UIProcess/UserMediaProcessManager.h:53
> +    void devicesChanged() final;

Can it be made private?

> Source/WebKit/UIProcess/UserMediaProcessManager.h:63
> +    WebCore::UserMediaClient::DeviceChangeObserverToken m_deviceChangeObserverToken;

No longer needed?

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57
> +    class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer {

Can be made private probably.
You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:59
> +        DeviceObserver(UserMediaPermissionRequestManager& manager)

explicit.
Comment 21 Philippe Normand 2021-04-12 08:58:40 PDT
Comment on attachment 425503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425503&action=review

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57
>> +    class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer {
> 
> Can be made private probably.
> You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer.

I tried that already, would incur duplicate inheritance on CanMakeWeakPtr.
Comment 22 youenn fablet 2021-04-12 09:01:48 PDT
(In reply to Philippe Normand from comment #21)
> Comment on attachment 425503 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425503&action=review
> 
> >> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57
> >> +    class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer {
> > 
> > Can be made private probably.
> > You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer.
> 
> I tried that already, would incur duplicate inheritance on CanMakeWeakPtr.

Right, you can probably do something like:
    using Class::weakPtrFactory;
    using WeakValueType = Class::WeakValueType;

With Class be one of the two CanMakeWeakPtr.
Comment 23 Philippe Normand 2021-04-12 09:03:28 PDT
Created attachment 425753 [details]
Patch
Comment 24 Philippe Normand 2021-04-12 09:28:55 PDT
Comment on attachment 425503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425503&action=review

>>>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:57
>>>> +    class DeviceObserver : public WebCore::RealtimeMediaSourceCenter::Observer {
>>> 
>>> Can be made private probably.
>>> You could also make UserMediaPermissionRequestManager a RealtimeMediaSourceCenter::Observer.
>> 
>> I tried that already, would incur duplicate inheritance on CanMakeWeakPtr.
> 
> Right, you can probably do something like:
>     using Class::weakPtrFactory;
>     using WeakValueType = Class::WeakValueType;
> 
> With Class be one of the two CanMakeWeakPtr.

Ah, nice! I didn't know about this trick. I'll upload a new version of the patch. Thanks!
Comment 25 Philippe Normand 2021-04-12 09:40:02 PDT
Created attachment 425759 [details]
Patch
Comment 26 Philippe Normand 2021-04-12 09:42:17 PDT
Created attachment 425760 [details]
Patch
Comment 27 EWS 2021-04-13 01:38:21 PDT
Committed r275871 (236436@main): <https://commits.webkit.org/236436@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425760 [details].
Comment 28 Radar WebKit Bug Importer 2021-04-13 01:39:21 PDT
<rdar://problem/76581284>