Bug 192268 – [MediaStream] 'devicechange' event when more capture device information are revealed.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

Before the user grants access to the camera and/or microphone, mediaDevices.enumerateDevices() does not reveal device IDs and only includes default capture devices. A 'devicechange' event should fire after the user allows capture to begin if we have previously returned a filtered capture list.

[Eric Carlson]

Created attachment 356324 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/46399460>

[youenn fablet]

Comment on attachment 356324 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:108
> +    auto completion = [this, protectedThis = makeRef(*this), canAccessMicrophone, canAccessCamera] (const Vector<CaptureDevice>& captureDevices, const String& deviceIdentifierHashSalt, bool) mutable {

I guess a follow-up patch will remove the second parameter of the completion handler.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:189
> +    m_hasFilteredDeviceList = false;

Should it be m_hasFilteredDeviceList = true?
Maybe we should add a test, something like a page which calls getUserMedia, reload, and does enumerateDevice.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414
> +            continue;

I wonder whether we should update this check.
It seems that if we have something like:
1. a page A with a same origin frame B
2. frame B does getUserMedia and user agrees.
3. frame B is navigated to same origin and does again getUserMedia (or page A makes a getUserMedia request).
4. second getUserMedia will create a reprompt.

Maybe frameID check should be applied to the main frame only?
Probably not to be changed in this patch.
Or maybe we should always pass the mainFrameId and no longer the frameId?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:428
> +    static const int defaultCameraCount = 2;

Is IOS_FAMILY too large for 2 cameras?
I am thinking watch/iosmac.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:452
> +        for (auto& device : devices) {

const auto&?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:459
> +                if (device.type() == WebCore::CaptureDevice::DeviceType::Microphone && ++microphoneCount > defaultMicrophoneCount)

Ah so maybe defaultMicrophoneCount should be renamed to defaultMaxMicrophoneCount.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:472
> +            filteredDevices.append(CaptureDevice(id, device.type(), label, groupId));

We could make CaptureDevice constructor take String&&

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:477
> +        m_page.process().send(Messages::WebPage::DidCompleteMediaDeviceEnumeration(userMediaID, WTFMove(filteredDevices), WTFMove(deviceIDHashSalt), WTFMove(originHasPersistentAccess)), m_page.pageID());

We do not need to move originHasPersistentAccess.
We could try removing it.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:546
> +    m_hasFilteredDeviceList = false;

Should it be m_hasFilteredDeviceList = true?

> LayoutTests/fast/mediastream/enumerate-devices-change-event.html:25
> +                resolve();

Should we call reject("navigator.mediaDevices.ondevicechange took too long") instead of console.log/resolve?
I guess resolve() allows to continue running the test but assert_equals(eventCount, 1) will always fail so we do not gain much.

> LayoutTests/fast/mediastream/enumerate-devices-change-event.html:30
> +        assert_equals(eventCount, 1, "one event fired");

Maybe this eventCount should be checked at the end of the test, after the second enumerateDevices call.

[youenn fablet]

Comment on attachment 356324 [details]
Patch

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

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:546
>> +    m_hasFilteredDeviceList = false;
> 
> Should it be m_hasFilteredDeviceList = true?

Nope, m_hasFilteredDeviceList should be set to false as the patch is doing.

[Eric Carlson]

Comment on attachment 356324 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:108
>> +    auto completion = [this, protectedThis = makeRef(*this), canAccessMicrophone, canAccessCamera] (const Vector<CaptureDevice>& captureDevices, const String& deviceIdentifierHashSalt, bool) mutable {
> 
> I guess a follow-up patch will remove the second parameter of the completion handler.

Yes, and from the IPC as well.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:189
>> +    m_hasFilteredDeviceList = false;
> 
> Should it be m_hasFilteredDeviceList = true?
> Maybe we should add a test, something like a page which calls getUserMedia, reload, and does enumerateDevice.

I don't think so. m_hasFilteredDeviceList is only used to determine if we should file a 'devicechange' event after the user grants access to a capture device, so I think we want to reset it when the main page changes.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:414
>> +            continue;
> 
> I wonder whether we should update this check.
> It seems that if we have something like:
> 1. a page A with a same origin frame B
> 2. frame B does getUserMedia and user agrees.
> 3. frame B is navigated to same origin and does again getUserMedia (or page A makes a getUserMedia request).
> 4. second getUserMedia will create a reprompt.
> 
> Maybe frameID check should be applied to the main frame only?
> Probably not to be changed in this patch.
> Or maybe we should always pass the mainFrameId and no longer the frameId?

This is called from enumerateMediaDevicesForFrame, which is only passed the ID of the frame making the request so I *think* we want to compare it with the IDs of frames which have been granted device access, not with their main frame ID.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:428
>> +    static const int defaultCameraCount = 2;
> 
> Is IOS_FAMILY too large for 2 cameras?
> I am thinking watch/iosmac.

We can deal with that when/if we add support for capture to those platforms.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:452
>> +        for (auto& device : devices) {
> 
> const auto&?

Fixed.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:459
>> +                if (device.type() == WebCore::CaptureDevice::DeviceType::Microphone && ++microphoneCount > defaultMicrophoneCount)
> 
> Ah so maybe defaultMicrophoneCount should be renamed to defaultMaxMicrophoneCount.

Sure, changed.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:472
>> +            filteredDevices.append(CaptureDevice(id, device.type(), label, groupId));
> 
> We could make CaptureDevice constructor take String&&

Good idea. We will want to change a bunch more code so I will do that in a followup.

>> LayoutTests/fast/mediastream/enumerate-devices-change-event.html:25
>> +                resolve();
> 
> Should we call reject("navigator.mediaDevices.ondevicechange took too long") instead of console.log/resolve?
> I guess resolve() allows to continue running the test but assert_equals(eventCount, 1) will always fail so we do not gain much.

Fixed.

>> LayoutTests/fast/mediastream/enumerate-devices-change-event.html:30
>> +        assert_equals(eventCount, 1, "one event fired");
> 
> Maybe this eventCount should be checked at the end of the test, after the second enumerateDevices call.

Good idea, fixed.

[Eric Carlson]

Created attachment 356379 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 356379 [details]:

inspector/debugger/search-scripts.html bug 192311 (authors: [e-mail], [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 356379 [details]
Patch for landing

Clearing flags on attachment: 356379

Committed r238796: <https://trac.webkit.org/changeset/238796>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.