Bug 188521 – [MediaStream] Move capture device monitoring to WebKit

• Component: WebRTC
• Status: NEW

[Eric Carlson]

Monitor capture device changes in the UI process and inform the web process when changes are detected.

[Eric Carlson]

Created attachment 347035 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/43251787>

[youenn fablet]

Comment on attachment 347035 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDevices.cpp:189
> +                    return;

Do we need weak pointers there? It seems not if we are unregistering the callback when MediaDevices is being stopped and/or destroyed.

> Source/WebCore/Modules/mediastream/MediaDevices.cpp:191
> +                m_scheduledEventTimer.startOneShot(Seconds(randomNumber() / 2));

It is not totally clear to me what this protects against. Spec says "These events are potentially triggered simultaneously across browsing contexts on different origins; user agents may add fuzzing on the timing of events to avoid cross-origin activity correlation."

> Source/WebCore/Modules/mediastream/UserMediaClient.h:52
> +    using DeviceChangeObserverToken = unsigned;

You can use something like:
enum DeviceChangeObserverTokenType { };
using DeviceChangeObserverToken = ObjectIdentifier<DeviceChangeObserverTokenType>;

> Source/WebCore/Modules/mediastream/UserMediaClient.h:53
> +    virtual DeviceChangeObserverToken addDeviceChangeObserver(std::function<void()>&&) = 0;

WTF::Function?

> Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:57
> +            return;

Do we need that weakThis check?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.h:58
> +    void refreshAudioCaptureDevices(bool);

Maybe an enum would be clearer?

> Source/WebCore/platform/mock/MockRealtimeMediaSourceCenter.cpp:-58
> -    singleton().captureDevicesChanged();

We should probably keep these and make sure they trigger the necessary IPC calls.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:98
> +    m_pendingDeviceChangeEvent = false;

This pending logic seems better suited in the web process.
Since the spec is about the active document, it probably goes well in Document.

I am not sure there is a need to go through UserMediaPermissionRequestManagerProxy.
Maybe UserMediaProcessManager could directly fo the IPC to WebProcess.

There is the issue with a document that has persistent access.
One possibility would be for UIProcess/UserMediaProcessManager to get a list of all documents that have MediaDevices.ondevicechange being registered.
Then UIProcess could send a Messages::CaptureDevicesChanged with a boolean stating whether the document has (persistent) access or not.

> Source/WebKit/WebProcess/WebProcess.cpp:1735
> +    captureDevicesChanged();

These should not be needed.
The MockRealtimeMediaSourceCenter singleton in UIProcess should send captureDevicesChanged and trigger IPC calls like would do the regular RealtimeMediaSourceCenter.
That will allow us to validate the whole IPC path.

[Eric Carlson]

Created attachment 347229 [details]
Patch

[youenn fablet]

Comment on attachment 347229 [details]
Patch

Nice patch!
A couple of nits and questions below.

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:252
> +        }

This is another patch so should probably be removed ;)

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:199
> +    m_deviceChangedObserver = WTFMove(observer);

Maybe we should ASSERT(!m_deviceChangedObserver)

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:103
> +    std::function<void()> m_deviceChangedObserver;

Maybe WTF::Function in case we need it in the future?

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:145
> +    deviceChanged();

Potential preexisting issue and probably not a big deal, but is there a risk that we generate device change events more than needed?
Let's say we call refreshAudioCaptureDevices but m_devices/m_audioSessionCaptureDevices remain the same for instance.

> Source/WebCore/platform/mock/MockRealtimeMediaSource.cpp:100
> +    RealtimeMediaSourceCenter::singleton().captureDevicesChanged();

Why is this change needed? Ditto for other changes.

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:286
> +            auto oldDevices = m_captureDevices;

WTFMove(m_captureDevices)

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:295
> +                    auto oldDevice = std::find_if(oldDevices.begin(), oldDevices.end(), [&newDevice] (auto& device) {

can be written as:
if (oldDevices.findMatching([&newDevice] (auto& device) {...} == notFound)
  haveChanges = true;

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:43
> +static WebCore::ActivityState::Flags focusedActiveWindow = WebCore::ActivityState::IsFocused | WebCore::ActivityState::WindowIsActive | WebCore::ActivityState::IsVisible;

We are adding IsVisible while the spec and name of the variable sats focused and active. Is there a reason for that?

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:232
> +    m_deviceChangeObserverMap.set(identifier, WTFMove(observer));

s/set/add/

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:275
> +    }

I guess the idea is to protect for the case of the map being modified while iterating through it.
s/it/iterator.
Another approach would be to make modifications to the map happening asynchronously in addDeviceChangeObserver/removeDeviceChangeObserver.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:289
> +    });

Why does it need to be asynchronously removed? We could also do that lazily and never remove the observer until it gets destroyed.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:72
> +    void activityStateDidChange(WebCore::ActivityState::Flags oldActivityState, WebCore::ActivityState::Flags newActivityState) override;

final?

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:91
> +    bool m_monitoringStateChanges { false };

m_monitoringStateChanges without an s? maybe m_monitoringActivityStateChange?

> LayoutTests/fast/mediastream/device-change-event-2.html:74
> +        await new Promise((resolve, reject) => {

Maybe add assert_true(document.hidden) here.

> LayoutTests/fast/mediastream/device-change-event-2.html:108
> +        assert_equals(eventCount, 1, "one one event fired");

s/one one/one

[Eric Carlson]

Comment on attachment 347229 [details]
Patch

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

>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:252
>> +        }
> 
> This is another patch so should probably be removed ;)

Oops!

>> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:145
>> +    deviceChanged();
> 
> Potential preexisting issue and probably not a big deal, but is there a risk that we generate device change events more than needed?
> Let's say we call refreshAudioCaptureDevices but m_devices/m_audioSessionCaptureDevices remain the same for instance.

Rather than doing it here and in every capture manager, the device change observer in UserMediaProcessManager::beginMonitoringCaptureDevices does filtering.

>> Source/WebCore/platform/mock/MockRealtimeMediaSource.cpp:100
>> +    RealtimeMediaSourceCenter::singleton().captureDevicesChanged();
> 
> Why is this change needed? Ditto for other changes.

As we discussed, this is needed so the device change observer in UserMediaProcessManager::beginMonitoringCaptureDevices is notified of the change. This code also runs in the web process, but it is a noop.

>> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:286
>> +            auto oldDevices = m_captureDevices;
> 
> WTFMove(m_captureDevices)

Fixed.

>> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:295
>> +                    auto oldDevice = std::find_if(oldDevices.begin(), oldDevices.end(), [&newDevice] (auto& device) {
> 
> can be written as:
> if (oldDevices.findMatching([&newDevice] (auto& device) {...} == notFound)
>   haveChanges = true;

Fixed.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:43
>> +static WebCore::ActivityState::Flags focusedActiveWindow = WebCore::ActivityState::IsFocused | WebCore::ActivityState::WindowIsActive | WebCore::ActivityState::IsVisible;
> 
> We are adding IsVisible while the spec and name of the variable sats focused and active. Is there a reason for that?

Good point, fixed.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:232
>> +    m_deviceChangeObserverMap.set(identifier, WTFMove(observer));
> 
> s/set/add/

Fixed.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:275
>> +    }
> 
> I guess the idea is to protect for the case of the map being modified while iterating through it.
> s/it/iterator.
> Another approach would be to make modifications to the map happening asynchronously in addDeviceChangeObserver/removeDeviceChangeObserver.

Fixe /it/iterator, but I think it is important for removeDeviceChangeObserver to be synchronous so I left the rest the same

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:289
>> +    });
> 
> Why does it need to be asynchronously removed? We could also do that lazily and never remove the observer until it gets destroyed.

It needs to be removed asynchronously because observers are called from a loop that iterates over a HashSet member variable (see Page::setActivityState), so removing it synchronously corrupts the hash and causes a crash. I decided to remove immediately because activityStateDidChange is called *very* frequently and we only care when we have a deferred notification.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:72
>> +    void activityStateDidChange(WebCore::ActivityState::Flags oldActivityState, WebCore::ActivityState::Flags newActivityState) override;
> 
> final?

Yep, fixed.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:91
>> +    bool m_monitoringStateChanges { false };
> 
> m_monitoringStateChanges without an s? maybe m_monitoringActivityStateChange?

Fixed.

>> LayoutTests/fast/mediastream/device-change-event-2.html:74
>> +        await new Promise((resolve, reject) => {
> 
> Maybe add assert_true(document.hidden) here.

Fixed.

>> LayoutTests/fast/mediastream/device-change-event-2.html:108
>> +        assert_equals(eventCount, 1, "one one event fired");
> 
> s/one one/one

Fixed.

[Eric Carlson]

Created attachment 347366 [details]
Patch for landing

[Eric Carlson]

Created attachment 347382 [details]
Patch for landing

[Eric Carlson]

Created attachment 347496 [details]
Patch for landing

[Eric Carlson]

Created attachment 347501 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 347501 [details]
Patch for landing

Clearing flags on attachment: 347501

Committed r235086: <https://trac.webkit.org/changeset/235086>

[youenn fablet]

Looking at landed patch, it seems it reverted part of https://trac.webkit.org/changeset/234949 in RealtimeMediaSource.
Can we get these changes back in?

[Eric Carlson]

(In reply to youenn fablet from comment #12)
> Looking at landed patch, it seems it reverted part of
> https://trac.webkit.org/changeset/234949 in RealtimeMediaSource.
> Can we get these changes back in?

r235422