Summary: | enumerateDevices should expose audiooutput devices that are tied to an audio input device | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, changseok, clopez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Local Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
youenn fablet
2020-08-25 06:45:50 PDT
Created attachment 407187 [details]
Patch
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 Created attachment 407195 [details]
Patch
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? 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. Created attachment 407285 [details]
Patch for landing
Committed r266166: <https://trac.webkit.org/changeset/266166> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407285 [details]. |