Be able to pass up device names from WebProcess to UIProcess for getUserMedia()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Matthew Daiter]

Need to be able to pass up names of video and audio devices through WebKit2 to Safari

[Radar WebKit Bug Importer]

<rdar://problem/21883094>

[Matthew Daiter]

Created attachment 257008 [details]
Patch

[Build Bot]

Comment on attachment 257008 [details]
Patch

Attachment 257008 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6104126979047424

New failing tests:
fast/workers/stress-js-execution.html

[Build Bot]

Created attachment 257010 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Eric Carlson]

Comment on attachment 257008 [details]
Patch

I am not sure that passing device names is the best approach to solving this problem. This will make it impossible for the UA to device info in a different language than the system default, it limits a UA to listing device name only, etc.

Can you return a vector with the device unique IDs instead and require the UA use its platform API to get whatever information it needs for it UI?

[Matthew Daiter]

Created attachment 257102 [details]
Patch

[Brent Fulgham]

Comment on attachment 257102 [details]
Patch

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

I think you're doing more copying than you should. We should only be copying when the vector reaches the objects constructor -- all argument passing should be via const reference.

> Source/WebCore/ChangeLog:3
> +        Bridged passing lists of devices between Safari and WebCore

You should rephrase this as "... between the UIProcess and the WebProcess". WebCore is used in all of the WK2 processes, and we have more clients than just Safari that use this interface.

> Source/WebKit2/ChangeLog:3
> +        Bridged passing lists of devices between Safari and WebCore

Ditto my comment "s/Safari/UIProcess/" and "s/WebCore/WebProcess".

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:74
> +    Vector<String> deviceUIDsAudio() const { return m_deviceUIDsAudio; }

These should be returned as const reference to avoid copying.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:41
> +PassRefPtr<UserMediaPermissionRequestProxy> UserMediaPermissionRequestManagerProxy::createRequest(uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String> deviceUIDsVideo, Vector<String> deviceUIDsAudio)

These should be passed in as const references.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:36
> +    PassRefPtr<UserMediaPermissionRequestProxy> createRequest(uint64_t userMediaID, bool audio, bool video, Vector<String> deviceUIDsVideo, Vector<String> deviceUIDsAudio);

These should be passed in as const references.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:27
> +UserMediaPermissionRequestProxy::UserMediaPermissionRequestProxy(UserMediaPermissionRequestManagerProxy& manager, uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String>& deviceUIDsVideo, Vector<String>& deviceUIDsAudio)

These should be passed in as *const* references. You are just copying them the constructor -- no need to require mutable versions of these.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:33
> +    static PassRefPtr<UserMediaPermissionRequestProxy> create(UserMediaPermissionRequestManagerProxy& manager, uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String>& deviceNamesVideo, Vector<String>& deviceNamesAudio)

Const references.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:47
> +    Vector<String> deviceUIDsAudio() const { return m_deviceUIDsAudio; }

Return these as const references.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:50
> +    UserMediaPermissionRequestProxy(UserMediaPermissionRequestManagerProxy&, uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String>& deviceNamesVideo, Vector<String>& deviceNamesAudio);

Const references.

[Matthew Daiter]

Created attachment 257105 [details]
Patch

[Matthew Daiter]

Created attachment 257107 [details]
Patch

[Matthew Daiter]

Comment on attachment 257102 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Bridged passing lists of devices between Safari and WebCore
> 
> You should rephrase this as "... between the UIProcess and the WebProcess". WebCore is used in all of the WK2 processes, and we have more clients than just Safari that use this interface.

Fixed.

>> Source/WebKit2/ChangeLog:3
>> +        Bridged passing lists of devices between Safari and WebCore
> 
> Ditto my comment "s/Safari/UIProcess/" and "s/WebCore/WebProcess".

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.h:74
>> +    Vector<String> deviceUIDsAudio() const { return m_deviceUIDsAudio; }
> 
> These should be returned as const reference to avoid copying.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:41
>> +PassRefPtr<UserMediaPermissionRequestProxy> UserMediaPermissionRequestManagerProxy::createRequest(uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String> deviceUIDsVideo, Vector<String> deviceUIDsAudio)
> 
> These should be passed in as const references.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:36
>> +    PassRefPtr<UserMediaPermissionRequestProxy> createRequest(uint64_t userMediaID, bool audio, bool video, Vector<String> deviceUIDsVideo, Vector<String> deviceUIDsAudio);
> 
> These should be passed in as const references.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:27
>> +UserMediaPermissionRequestProxy::UserMediaPermissionRequestProxy(UserMediaPermissionRequestManagerProxy& manager, uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String>& deviceUIDsVideo, Vector<String>& deviceUIDsAudio)
> 
> These should be passed in as *const* references. You are just copying them the constructor -- no need to require mutable versions of these.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:33
>> +    static PassRefPtr<UserMediaPermissionRequestProxy> create(UserMediaPermissionRequestManagerProxy& manager, uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String>& deviceNamesVideo, Vector<String>& deviceNamesAudio)
> 
> Const references.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:47
>> +    Vector<String> deviceUIDsAudio() const { return m_deviceUIDsAudio; }
> 
> Return these as const references.

Fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:50
>> +    UserMediaPermissionRequestProxy(UserMediaPermissionRequestManagerProxy&, uint64_t userMediaID, bool requiresAudio, bool requiresVideo, Vector<String>& deviceNamesVideo, Vector<String>& deviceNamesAudio);
> 
> Const references.

Fixed.

[Brent Fulgham]

Comment on attachment 257107 [details]
Patch

r=me.

[Brent Fulgham]

Comment on attachment 257107 [details]
Patch

After discussing this with Eric, we need to change this:

1. WebProcess sees a request for a video (or audio) device as a media source.
2. WebProcess retrieves a set of media devices of the appropriate type (video or audio).
3. WebProcess filters the devices based on any limitations set in the HTML (e.g., specific video resolution).
4. WebProcess sends a request to UIProcess: "createRequest(enum /*audio or video*/, const Vector<String>& deviceIDs).
5. Time passes... while UIProcess presents a dialog to the user and gets permission.
6. UIProcess sends a message to WebProcess: "You are allowed to use device ID 'some string'".
7. WebProcess takes that device ID, gets the actual device, and does stuff with it.

That should be the message flow.

[Jer Noble]

(In reply to comment #12)
> Comment on attachment 257107 [details]
> Patch
> 
> After discussing this with Eric, we need to change this:
> 
> 1. WebProcess sees a request for a video (or audio) device as a media source.

The Media Capture spec says that a MediaStreamConstraints object has constraints for bot a video and an audio, and a single MediaStreamConstraints object is passed into getUserMedia(). So the WebProcess will see a simultaneous request for both audio and video devices, and the list of matching tracks will include both. So I don't see a need to bifurcate this into separate requests for video and then audio devices.

Whats more, we probably don't want to require the behavior that the UA present two sequential permissions dialogs (one for the selected video track, and one for the selected audio track), but instead want to allow a combined permission dialog ("The page wants to use your camera and your microphone. allow/deny?").

> 2. WebProcess retrieves a set of media devices of the appropriate type
> (video or audio).
> 3. WebProcess filters the devices based on any limitations set in the HTML
> (e.g., specific video resolution).

This is step 5.2.2 of getUserMedia().

> 4. WebProcess sends a request to UIProcess: "createRequest(enum /*audio or
> video*/, const Vector<String>& deviceIDs).
> 5. Time passes... while UIProcess presents a dialog to the user and gets
> permission.
> 6. UIProcess sends a message to WebProcess: "You are allowed to use device
> ID 'some string'".

This is incorrect.  The UA has to do further filtering based on the constraints passed into getUserMedia().  See step 5.6.  Multiple devices may fit the constraints, but only one can be chosen.  Currently, the constraints exist only in the WebProcess, so the UIProcess can't just choose one UUID to return (for each media type).

So if you want the UIProcess to only return one, the constraints must be passed up to the UIProcess, and all the filtering and scoring must happen there.  Otherwise, the UIProcess can only either approve or deny the request, and leave the filtering and chasing to the WebProcess.

> 7. WebProcess takes that device ID, gets the actual device, and does stuff
> with it.
> 
> That should be the message flow.

[Eric Carlson]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 257107 [details]
> > Patch
> > 
> > After discussing this with Eric, we need to change this:
> > 
> > 4. WebProcess sends a request to UIProcess: "createRequest(enum /*audio or
> > video*/, const Vector<String>& deviceIDs).
> > 5. Time passes... while UIProcess presents a dialog to the user and gets
> > permission.
> > 6. UIProcess sends a message to WebProcess: "You are allowed to use device
> > ID 'some string'".
> 
> This is incorrect.  The UA has to do further filtering based on the
> constraints passed into getUserMedia().  See step 5.6.  Multiple devices may
> fit the constraints, but only one can be chosen.  Currently, the constraints
> exist only in the WebProcess, so the UIProcess can't just choose one UUID to
> return (for each media type).
> 
> So if you want the UIProcess to only return one, the constraints must be
> passed up to the UIProcess, and all the filtering and scoring must happen
> there.  Otherwise, the UIProcess can only either approve or deny the
> request, and leave the filtering and chasing to the WebProcess.
> 
  The WebProcess has to have the ability to evaluate constraints so my idea is that the WebProcess can pre-filter the available devices based on the constraints passed to getUserMedia, so the UIProcess will only ever see a list of eligible devices and therefore will not need to evaluate constraints at all.

[Matthew Daiter]

So now that we can sort lists of UIDs based off of fitness score before any vector is sent to the UIProcess, should we pass this?

[Eric Carlson]

(In reply to comment #15)
> So now that we can sort lists of UIDs based off of fitness score before any
> vector is sent to the UIProcess, should we pass this?

Yes. I think passing a single vector of UIDs (no need to separate audio and video), sorted by fitness, is the way to go.

[Matthew Daiter]

@Eric Carlson & Jer Noble: Then should this be patched?

[Matthew Daiter]

(In reply to comment #16)
> (In reply to comment #15)
> > So now that we can sort lists of UIDs based off of fitness score before any
> > vector is sent to the UIProcess, should we pass this?
> 
> Yes. I think passing a single vector of UIDs (no need to separate audio and
> video), sorted by fitness, is the way to go.
You DO need to separate, though, due to the different selection screens and combinations of video and audio input. For example:
getUserMedia({video : true, audio : true}, function(){}, function(){}); is called, and I have three video cameras on my machine and one audio. We *must* separate out the audio and video devices into different vectors, so that the screen that presents user selection knows which devices it presents as audio to the user and which devices it presents as video. Therefore, we *MUST* separate these vectors.
*Necessary if the UI is kept in the current state.

[Eric Carlson]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > So now that we can sort lists of UIDs based off of fitness score before any
> > > vector is sent to the UIProcess, should we pass this?
> > 
> > Yes. I think passing a single vector of UIDs (no need to separate audio and
> > video), sorted by fitness, is the way to go.
> You DO need to separate, though, due to the different selection screens and
> combinations of video and audio input. For example:
> getUserMedia({video : true, audio : true}, function(){}, function(){}); is
> called, and I have three video cameras on my machine and one audio. We
> *must* separate out the audio and video devices into different vectors, so
> that the screen that presents user selection knows which devices it presents
> as audio to the user and which devices it presents as video. Therefore, we
> *MUST* separate these vectors.
> *Necessary if the UI is kept in the current state.

If all of the UIDs passed to the UI process meet the constraints, I think the UI process really only needs to know if the the request is for audio+video, audio-only, or video-only, so wouldn't it work to use an enum and a single vector?

[Eric Carlson]

(In reply to comment #19)
> 
> If all of the UIDs passed to the UI process meet the constraints, I think
> the UI process really only needs to know if the the request is for
> audio+video, audio-only, or video-only, so wouldn't it work to use an enum
> and a single vector?

Actually, I guess it is possible for some devices to provide both audio and video but to meet only one constraint, so I guess we do need to pass vectors of both audio and video devices.

[Brent Fulgham]

Comment on attachment 257107 [details]
Patch

We've decided this is good!

[WebKit Commit Bot]

Comment on attachment 257107 [details]
Patch

Clearing flags on attachment: 257107

Committed r187258: <http://trac.webkit.org/changeset/187258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.