Bug 147171 – Fill vectors in UserMediaRequest with only UIDs of devices that satisfy constraints

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Matthew Daiter]

We currently send a list of devices, no matter what, to Safari for requesting user media. There should be an easy way to create a list of only devices that can satisfy constraints.

[Radar WebKit Bug Importer]

<rdar://problem/21931121>

[Matthew Daiter]

Created attachment 257218 [details]
Patch

[Darin Adler]

Comment on attachment 257218 [details]
Patch

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

Doesn’t build because you forgot to update the other implementations of MediaStreamCreationClient, specifically MockRealtimeMediaSourceCenter.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:126
> +    for (auto audioTrack : audioTracks)

This will churn reference counts unnecessarily as we iterate each item and copy it into a RefPtr. To fix that, use auto& instead of auto.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:128
> +    for (auto videoTrack : videoTracks)

Ditto.

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:74
> +    Vector<String> filteredListUIDsVideo() const { return m_filteredListUIDsVideo; }
> +    Vector<String> filteredListUIDsAudio() const { return m_filteredListUIDsAudio; }

It’s inefficient to copy these vectors just to return them. Please consider returning const Vector<String>& instead.

> Source/WebCore/platform/mediastream/MediaStreamCreationClient.h:41
> +    virtual void constraintsValidated(Vector<RefPtr<RealtimeMediaSource>>, Vector<RefPtr<RealtimeMediaSource>>) = 0;

It’s inefficient to copy these vectors as part of calling this function. Please consider const Vector&. Also, it’s not clear what these two vectors represent, so these arguments need names.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:383
> +    Vector<RefPtr<RealtimeMediaSource>> bestSourcesList = Vector<RefPtr<RealtimeMediaSource>>();

No need to do the "=" part here. Vectors are empty by default without any explicit initialization.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:389
>      Vector<CaptureDevice>& devices = captureDeviceList();
>      size_t count = devices.size();

This code should really use a modern for loop, like this:

    for (auto& device : captureDeviceList()) {

Then below use “device” wherever the old code used “devices[i]”. Only issue is that we’d need to rename the local variables named device below.

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:102
> +        // FIXME: consider the constraints when choosing among multiple devices. For now just select the first available

Please capitalize the sentence in a comment like this. The word "consider".

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:117
> +        // FIXME: consider the constraints when choosing among multiple devices. For now just select the first available

Please capitalize the sentence in a comment like this. The word "consider".

[Matthew Daiter]

Created attachment 257221 [details]
Patch

[Eric Carlson]

Comment on attachment 257221 [details]
Patch

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

Looks good overall, but it obviously needs to be rebased so it applies to trunk.

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

Nit: I think audioDeviceUIDs and videoDeviceUIDs would be better. Also for the instance variables.

[Matthew Daiter]

Comment on attachment 257221 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/UserMediaRequest.h:74
>> +    const Vector<String>& deviceUIDsAudio() const { return m_deviceUIDsAudio; }
> 
> Nit: I think audioDeviceUIDs and videoDeviceUIDs would be better. Also for the instance variables.

Fixed.

[Matthew Daiter]

Created attachment 257231 [details]
Patch

[Matthew Daiter]

Created attachment 257236 [details]
Patch

[Eric Carlson]

Comment on attachment 257236 [details]
Patch

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:80
> +    virtual void constraintsValidated(const Vector<RefPtr<RealtimeMediaSource>>&, const Vector<RefPtr<RealtimeMediaSource>>&) override final;

As Darin noted earlier, it is not clear what these two vectors represent so these arguments need names.

[WebKit Commit Bot]

Comment on attachment 257236 [details]
Patch

Clearing flags on attachment: 257236

Committed r187168: <http://trac.webkit.org/changeset/187168>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.