Bug 171877 – [MediaStream] deviceId constraint doesn't work with getUserMedia

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

deviceId constraint doesn't work with getUserMedia

[Eric Carlson]

<rdar://problem/31899730>

[Eric Carlson]

Created attachment 309533 [details]
Proposed patch.

[Jer Noble]

Comment on attachment 309533 [details]
Proposed patch.

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

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:319
> +        MediaConstraintsData localAudioConstraintsData = MediaConstraintsData(audioConstraintsData);
> +        localAudioConstraintsData.deviceIDHashSalt = deviceIdentifierHashSalt;
> +
> +        MediaConstraintsData localVideoConstraintsData = MediaConstraintsData(videoConstraintsData);
> +        localVideoConstraintsData.deviceIDHashSalt = deviceIdentifierHashSalt;
> +
> +        auto audioConstraints = MediaConstraintsImpl::create(WTFMove(localAudioConstraintsData));
> +        auto videoConstraints = MediaConstraintsImpl::create(WTFMove(localVideoConstraintsData));

Nit: this is really wordy. Could we maybe add some way of creating an Impl with just a MediaConstraintsData and a salt?

[Eric Carlson]

Comment on attachment 309533 [details]
Proposed patch.

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

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:319
>> +        auto videoConstraints = MediaConstraintsImpl::create(WTFMove(localVideoConstraintsData));
> 
> Nit: this is really wordy. Could we maybe add some way of creating an Impl with just a MediaConstraintsData and a salt?

Good idea, fixed.

[Eric Carlson]

Created attachment 309540 [details]
Patch for landing.

[youenn fablet]

Looks good to me too.
Main question is I am not sure why we need both a userMediaDocumentSecurityOrigin and a topLevelDocumentSecurityOrigin.
Some nits below that could be addressed as follow-ups.

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

> Source/WebCore/ChangeLog:31
> +        (WebCore::RealtimeMediaSource::selectSettings): Special case DeviceId because it

s/it//

> Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.cpp:42
> +    , m_topLevelDocumentSecurityOrigin(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin())

We have these two security origins. Why isn't it sufficient to have one?

> Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:55
> +    WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin() { return &m_topLevelDocumentSecurityOrigin.get(); }

We should return references.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:68
> +void FrameAuthorizationState::ensureSecurityOriginsAreEqual(WebCore::SecurityOrigin* userMediaDocumentSecurityOrigin, WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin)

Is it ok to pass null pointers for both of these?
Probably should pass as const.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:87
> +FrameAuthorizationState& UserMediaPermissionRequestManagerProxy::stateForRequest(RequestType& request)

Pass a const RequestType&?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:188
> +    auto frameState = stateForRequest(*request);

auto&?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:309
> +    auto validateConstraintsHandler = [this, userMediaID, validHandler, invalidHandler, audioConstraintsData, videoConstraintsData](const String& deviceIdentifierHashSalt) {

In theory, we should probably do something like validHandler = WTFMove(validHandler) and ditto for invalidHander.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:321
> +        RealtimeMediaSourceCenter::singleton().validateRequestConstraints(validHandler, invalidHandler, audioConstraints, videoConstraints);

Then we should move validHandler and validHandler. Might need mark  validateConstraintsHandler as mutable.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:324
> +    auto haveDeviceSaltHandler = [this, userMediaID, frameID, validateConstraintsHandler](uint64_t userMediaID, const String& deviceIdentifierHashSalt, bool originHasPersistentAccess) {

Ditto for moving validateConstraintsHandler I think.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:357
> +void UserMediaPermissionRequestManagerProxy::getUserMediaPermissionInfo(uint64_t userMediaID, uint64_t frameID, UserMediaPermissionCheckProxy::CompletionHandler&& handler, const String&& userMediaDocumentOriginIdentifier, const String&& topLevelDocumentOriginIdentifier)

Why const String&&?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:379
> +void UserMediaPermissionRequestManagerProxy::enumerateMediaDevicesForFrame(uint64_t userMediaID, uint64_t frameID, String userMediaDocumentOriginIdentifier, String topLevelDocumentOriginIdentifier)

Should be String&& probably

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:49
> +    void setDeviceIdentifierHashSalt(const String& hashSalt) { m_deviceIdentifierHashSalt = hashSalt; }

Should be String&&.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:50
> +    String deviceIdentifierHashSalt() const { return m_deviceIdentifierHashSalt; }

Can we make it a const String&?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:94
> +    void getUserMediaPermissionInfo(uint64_t userMediaID, uint64_t frameID, UserMediaPermissionCheckProxy::CompletionHandler&&, const String&&userMediaDocumentOriginIdentifier, const String&& topLevelDocumentOriginIdentifier);

String&& and space

[Build Bot]

Comment on attachment 309533 [details]
Proposed patch.

Attachment 309533 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3707616

New failing tests:
fast/mediastream/MediaDevices-enumerateDevices.html

[Build Bot]

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

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

[WebKit Commit Bot]

Comment on attachment 309540 [details]
Patch for landing.

Clearing flags on attachment: 309540

Committed r216545: <http://trac.webkit.org/changeset/216545>

[Eric Carlson]

Patch was rolled out in r216550.

[Eric Carlson]

Created attachment 309575 [details]
Update patch.

[WebKit Commit Bot]

Comment on attachment 309575 [details]
Update patch.

Clearing flags on attachment: 309575

Committed r216563: <http://trac.webkit.org/changeset/216563>

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #12)
> Comment on attachment 309575 [details]
> Update patch.
> 
> Clearing flags on attachment: 309575
> 
> Committed r216563: <http://trac.webkit.org/changeset/216563>

This change caused 2 API test failures as well as an assertion failure:

WebKit2.UserMediaBasic and MediaCaptureDisabledTest.EnableAndDisable
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/1389/steps/run-api-tests/logs/stdio

fast/mediastream/apply-constraints-video.html
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r216581%20(1025)/results.html

[Matt Lewis]

Reverted r216563 for reason:

Revision caused 2 api failures

Committed r216596: <http://trac.webkit.org/changeset/216596>

[Eric Carlson]

Created attachment 309717 [details]
Another patch.

[Eric Carlson]

The GTK breakage looks unrelated:
gtk-wk2-ews/WebKit/Source/WebKit2/WebKit2Prefix.h -Wno-unused-parameter -MMD -MT Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp.o -MF Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp.o.d -o Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp.o -c ../../Source/WebKit2/UIProcess/API/gtk/WebKitInstallMissingMediaPluginsPermissionRequest.cpp
g++: internal compiler error: Killed (program cc1plus)
libbacktrace could not find executable to open
Please submit a full bug report,

[Build Bot]

Comment on attachment 309717 [details]
Another patch.

Attachment 309717 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3718906

New failing tests:
fast/mediastream/get-user-media-device-id.html
fast/mediastream/MediaDevices-getUserMedia.html

[Build Bot]

Created attachment 309727 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

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

[Eric Carlson]

Created attachment 309744 [details]
Updated patch.

[Build Bot]

Comment on attachment 309744 [details]
Updated patch.

Attachment 309744 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3719927

New failing tests:
fast/mediastream/MediaDevices-getUserMedia.html

[Build Bot]

Created attachment 309766 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

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

[Eric Carlson]

Created attachment 309770 [details]
Updated patch.

[youenn fablet]

Some ideas for code improvements as a follow-up patch.
There is one straightfroward issue that needs to be fixed though (moving a variable and reusing it afterwards).

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

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:49
> +    MediaConstraintsData(const MediaConstraints& constraints)

Shouldn't it be marked explicit?

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:56
> +    MediaConstraintsData(const MediaConstraintsData& constraints, const String& hashSalt)

hashSalt should be a String&&, and we could do something like String&& = { } to remove the above constructor.
I am not sure where it is used by the way.
Maybe we should just beef up MediaConstraintsData(MediaTrackConstraintSetMap&&, Vector<MediaTrackConstraintSetMap>&&, bool) to take a String&& instead?

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:85
> +    String deviceIDHashSalt() const final { return m_data.deviceIDHashSalt; }

Should be a const String& I think.

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:86
> +    void setDeviceIDHashSalt(const String& salt) final { m_data.deviceIDHashSalt = salt; }

Should be taking a String&&

> Source/WebCore/Modules/mediastream/MediaDevices.h:85
> +    std::optional<RealtimeMediaSourceCenter::DevicesChangedObserverToken> m_deviceChangedToken;

I do not understand that change. Having 0 as a special 'unset' value seems fine with me.
What is the issue with the previous code?

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:159
>      m_allowedVideoDeviceUID = videoDeviceUID;

audioDeviceUID and videoDeviceUID should be String&&.

> Source/WebCore/platform/mediastream/MediaConstraints.h:814
> +    virtual String deviceIDHashSalt() const = 0;

Ditto on changes the signature here.

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:502
> +bool RealtimeMediaSource::selectSettings(const MediaConstraints& constraints, FlattenedConstraint& candidates, String& failedConstraint, SelectType type)

I guess the alternative approach would be for selectSettings to remove deviceId constraints when not being useful.
But this conflicts with const& signature. Seems fine.

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:552
> +            double constraintDistance = downcast<StringConstraint>(constraint).fitnessDistance(hashedID);

If we have several downcast<> like this, maybe we should encapsulate this in MediaConstraint, for instance constraint.asStringConstraint().
Or a templated fitnessDistance, but maybe that would cause more harm.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:183
> +    static NeverDestroyed<HashMap<unsigned, std::function<void()>>> map;

I am not sure this is healthy to add more static variables.
We should at least move this code closer to RealtimeMediaSourceCenter::singleton() so that if want to attach RealtimeMediaSourceCenter to the page, or to the process later on, we do not forget about this one.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:202
> +    auto callbacks = observerMap();

Do we want to make a separate copy of the map? Should it be auto&.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:203
>      for (auto it : callbacks)

Probably want auto& here? and use iterator instead of it.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:253
> +            audioSourceIds.uncheckedAppend(info.id);

Nit: since we do not reuse audioDeviceInfo, we could probably take a for(auto& info..) and do audioSourceIds.uncheckedAppend(WTFMove(info.id))

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:261
> +            videoSourceIds.uncheckedAppend(info.id);

Ditto here.

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:169
> +    invalidConstraint = emptyString();

Are we adding that for safety reasons? Since sourceUIDs is not empty, the value should be ignored, right?
The new pattern would be to add a VectorOfStringOrErrorMessage.

Btw, if we fail to create several sources on several devices, we will only pick the last invalid constraint. Is it ok?

> Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.cpp:42
> +    , m_topLevelDocumentSecurityOrigin(SecurityOriginData::fromDatabaseIdentifier(topLevelDocumentOriginIdentifier)->securityOrigin())

Should probably directly take Ref<SecurityOrigin>&& instead of strings here.
Future refactoring probably.

> Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:55
> +    WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin() { return &m_topLevelDocumentSecurityOrigin.get(); }

Should return references.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:44
> +    , m_topLevelDocumentSecurityOrigin(topLevelDocumentSecurityOrigin)

These might well be references and not pointers.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:68
> +void FrameAuthorizationState::ensureSecurityOriginsAreEqual(WebCore::SecurityOrigin* userMediaDocumentSecurityOrigin, WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin)

Probably here as well. Should take const& as well.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:87
> +FrameAuthorizationState& UserMediaPermissionRequestManagerProxy::stateForRequest(RequestType& request)

Can probably take a const RequestType&

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:275
> +        auto userMediaOrigin = API::SecurityOrigin::create(SecurityOriginData::fromDatabaseIdentifier(userMediaDocumentOriginIdentifier).value_or(SecurityOriginData()).securityOrigin());

RequestUserMediaPermissionForFrame should take WebCore::SecurityOriginData as input parameter not strings.
This would remove the need for conversions.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:315
> +        RealtimeMediaSourceCenter::singleton().validateRequestConstraints(WTFMove(validHandler), WTFMove(invalidHandler), audioConstraints, videoConstraints);

I don't know if this is always the case, but here we could move the constraints so that the underlying implementation would be fine modifying these constraints.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:325
> +            return;

Should we do this check before taking the request?
If so, can request be null in some cases? Probably but asking the question anyway.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:350
> +    UserMediaPermissionCheckProxy::CompletionHandler failureHandler = handler;

Since you move handler in the request, failureHandler will be empty here.
Probably not good.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:354
> +        failureHandler(userMediaID, String(), false);

Can you do something like request->reportFailure(...) to handle the case above?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:368
> +            return;

Same question/comment as above.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:374
> +        auto deviceInfo = RealtimeMediaSourceCenter::singleton().getMediaStreamDevices();

We are creating a Vector for each call to getMediaStreamDevices.
DidCompleteMediaDeviceEnumeration expecting a const&, this Vector allocation seems unnecessary.
Might be worth changing the signature in the future.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:47
> +    void ensureSecurityOriginsAreEqual(WebCore::SecurityOrigin* userMediaDocumentSecurityOrigin, WebCore::SecurityOrigin* topLevelDocumentSecurityOrigin);

Probably want references here and Ref<SecurityOrigin> below.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:49
> +    void setDeviceIdentifierHashSalt(const String& hashSalt) { m_deviceIdentifierHashSalt = hashSalt; }

Should be String&&

[Eric Carlson]

Created attachment 309942 [details]
Patch for landing.

[Eric Carlson]

Created attachment 309954 [details]
Patch for landing.

[Eric Carlson]

Created attachment 310056 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 310056 [details]
Patch for landing.

Clearing flags on attachment: 310056

Committed r216836: <http://trac.webkit.org/changeset/216836>