Bug 215806 – enumerateDevices should expose audiooutput devices that are tied to an audio input device

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

enumerateDevices should expose audiooutput devices that are tied to an audio input device

[youenn fablet]

Created attachment 407187 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[youenn fablet]

Created attachment 407195 [details]
Patch

[Eric Carlson]

Comment on attachment 407195 [details]
Patch

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

r=me once the bots are happy

> Source/WebCore/html/FeaturePolicy.cpp:181
> +            updateList(document, policy.m_speakerSelectionRule, item.substring(18));

`sizeof("speaker-selection")` would be safer.

> Source/WebCore/html/FeaturePolicy.cpp:191
> +            updateList(document, policy.m_syncXHRRule, item.substring(9));

`sizeof("sync-xhr")` would have avoided this problem.

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:234
> +        auto relatedDeviceIDs = CoreAudioCaptureDevice::relatedAudioDeviceIDs(deviceID);

This isn't may not be used, so you could initialize it just before the loop below.

> Source/WebCore/platform/mock/MockMediaDevice.h:81
> +    String microphoneId;

s/microphone/speaker/

> Source/WebCore/platform/mock/MockMediaDevice.h:82
> +    int defaultSampleRate { 44100 };

Does a speaker have a sample rate?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:601
> +static inline bool isMicrophoneDevice(const Vector<WebCore::CaptureDevice>& devices, const String& deviceID)

Nit: `haveMicrophoneDevice` might be a better name.

> LayoutTests/fast/mediastream/enumerate-speaker.html:21
> +        await navigator.mediaDevices.getUserMedia({ audio:true, video:true })
> +        const devices = await navigator.mediaDevices.enumerateDevices();
> +        assert_true(devices.length > 2, "before getting permission, at most 1 camera and 1 microphone are exposed");

"before getting permission..." is the wrong comment for this test.

Shouldn't there also be a test to ensure that speakers aren't exposed before a gUM call?

> LayoutTests/http/tests/media/media-stream/resources/enumerate-devices-iframe.html:26
> +        // Speakers are currently only exposed after getUserMedia.
> +        try {
> +            await navigator.mediaDevices.getUserMedia({ audio : true });
> +        } catch (e) {
> +        }
> +        try {
> +            await navigator.mediaDevices.getUserMedia({ video : true });
> +        } catch (e) {
> +        }
> +        return checkDeviceKind('audiooutput');

Why two separate gUM calls?

> LayoutTests/http/tests/media/media-stream/resources/enumerate-devices-iframe.html:41
> +        result.innerHTML = 'result: "' + visible + '"';

Does this test need new results?

[youenn fablet]

Thanks for the review.
> `sizeof("speaker-selection")` would be safer.

Will change it here and below.

> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:234
> > +        auto relatedDeviceIDs = CoreAudioCaptureDevice::relatedAudioDeviceIDs(deviceID);
> 
> This isn't may not be used, so you could initialize it just before the loop
> below.

OK

> > Source/WebCore/platform/mock/MockMediaDevice.h:81
> > +    String microphoneId;
> 
> s/microphone/speaker/

Changed to relatedMicrophoneId.

> > Source/WebCore/platform/mock/MockMediaDevice.h:82
> > +    int defaultSampleRate { 44100 };
> 
> Does a speaker have a sample rate?

Will remove it for now since it is not used.

> > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:601
> > +static inline bool isMicrophoneDevice(const Vector<WebCore::CaptureDevice>& devices, const String& deviceID)
> 
> Nit: `haveMicrophoneDevice` might be a better name.

OK

> > LayoutTests/fast/mediastream/enumerate-speaker.html:21
> > +        await navigator.mediaDevices.getUserMedia({ audio:true, video:true })
> > +        const devices = await navigator.mediaDevices.enumerateDevices();
> > +        assert_true(devices.length > 2, "before getting permission, at most 1 camera and 1 microphone are exposed");
> 
> "before getting permission..." is the wrong comment for this test.

OK

> Shouldn't there also be a test to ensure that speakers aren't exposed before
> a gUM call?

There is already one that checks that there are two devices, one camera and one microphone. But I will add one to check that there is no audiooutput.

> > LayoutTests/http/tests/media/media-stream/resources/enumerate-devices-iframe.html:26
> > +        // Speakers are currently only exposed after getUserMedia.
> > +        try {
> > +            await navigator.mediaDevices.getUserMedia({ audio : true });
> > +        } catch (e) {
> > +        }
> > +        try {
> > +            await navigator.mediaDevices.getUserMedia({ video : true });
> > +        } catch (e) {
> > +        }
> > +        return checkDeviceKind('audiooutput');
> 
> Why two separate gUM calls?

Depending on the policy, either one of them may fail/succeed.

> > LayoutTests/http/tests/media/media-stream/resources/enumerate-devices-iframe.html:41
> > +        result.innerHTML = 'result: "' + visible + '"';
> 
> Does this test need new results?

This is to debug in the browser, where you can see the iframe body and seeing result means the iframe sent the results.

[youenn fablet]

Created attachment 407285 [details]
Patch for landing

[youenn fablet]

WPT PR: https://github.com/web-platform-tests/wpt/pull/25231

[EWS]

Committed r266166: <https://trac.webkit.org/changeset/266166>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407285 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67795667>