Bug 180934 – [MediaStream] Clean up RealtimeMediaSource interfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

Clean up RealtimeMediaSource interfaces

[Radar WebKit Bug Importer]

<rdar://problem/36108648>

[Eric Carlson]

Created attachment 329702 [details]
Patch

[EWS Watchlist]

Comment on attachment 329702 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-default-feature-policy.https.html
http/tests/ssl/media-stream/get-user-media-nested.html
http/tests/media/media-stream/disconnected-frame.html
http/tests/ssl/media-stream/get-user-media-different-host.html

[EWS Watchlist]

Created attachment 329718 [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

[EWS Watchlist]

Comment on attachment 329702 [details]
Patch

Attachment 329702 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5735692

New failing tests:
imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-default-feature-policy.https.html
http/tests/ssl/media-stream/get-user-media-nested.html
http/tests/ssl/media-stream/get-user-media-different-host.html

[EWS Watchlist]

Created attachment 329719 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Eric Carlson]

Created attachment 329773 [details]
Patch

[youenn fablet]

Comment on attachment 329773 [details]
Patch

Some minor comments.

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:226
> +    m_request.videoConstraints.deviceIDHashSalt = WTFMove(deviceIdentifierHashSalt);

Probably not for this patch, but since we have a request there, maybe it should contain the deviceIdentifierHashSalt directly.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:280
> +        return;

I am not sure we can do that.
If we are in contextDestroyed() it means document is in its destructor, so I am not sure we can do things right there.
I am about to change it to an ActiveDOMObject and change it to stop(). In that case, the implementation is probably safe.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:286
> +        controller->cancelUserMediaAccessRequest(*this);

Why do we need to keep a ref there? Should we instead protect it outside the if?

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:94
> +        Ref<UserMediaRequest> m_userRequest;

Should it be m_userMediaRequest then?

> Source/WebCore/platform/mediastream/MediaStreamRequest.h:49
> +        return (decoder.decodeEnum(request.type) && decoder.decode(request.audioConstraints) && decoder.decode(request.videoConstraints));

We now try to use the modern IPC decoder.
The signature is
std::optional<MediaStreamRequest> decode(Decoder& decoder).

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:141
> +            result.append(device);

We should probably have done this? Were we listing/using devices that are not enabled?

> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:57
> +    for (auto& device : captureDevices()) {

Maybe you could use findMatching.

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:50
>      CaptureSourceOrError createVideoCaptureSource(const CaptureDevice& device, const MediaConstraints* constraints) final

Maybe we could pass the persistentId instead of the CaptureDevice?

> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:182
>          capabilities.addFacingMode(RealtimeMediaSourceSettings::Environment);

Use a ternary?

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:243
> +void UserMediaPermissionRequestManagerProxy::requestUserMediaPermissionForFrame(uint64_t userMediaID, uint64_t frameID, Ref<SecurityOrigin>&& userMediaDocumentOrigin, Ref<SecurityOrigin>&& topLevelDocumentOrigin, const MediaStreamRequest& userRequest)

You can probably use a MediaStreamRequest&& by changing also the signature in WebPageProxy (IPC allows taking an &&).
A follow-up might be better since tis patch is pretty big.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:98
> +    SecurityOrigin* topLevelDocumentOrigin = userRequest.topLevelDocumentOrigin();

auto* ?

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:99
>      ASSERT(topLevelDocumentOrigin);

Not sure we need this assert, or we should add one for userMediaDocumentOrigin as well?

[Eric Carlson]

Comment on attachment 329773 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:226
>> +    m_request.videoConstraints.deviceIDHashSalt = WTFMove(deviceIdentifierHashSalt);
> 
> Probably not for this patch, but since we have a request there, maybe it should contain the deviceIdentifierHashSalt directly.

OK

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:286
>> +        controller->cancelUserMediaAccessRequest(*this);
> 
> Why do we need to keep a ref there? Should we instead protect it outside the if?

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.h:94
>> +        Ref<UserMediaRequest> m_userRequest;
> 
> Should it be m_userMediaRequest then?

Fixed.

>> Source/WebCore/platform/mediastream/MediaStreamRequest.h:49
>> +        return (decoder.decodeEnum(request.type) && decoder.decode(request.audioConstraints) && decoder.decode(request.videoConstraints));
> 
> We now try to use the modern IPC decoder.
> The signature is
> std::optional<MediaStreamRequest> decode(Decoder& decoder).

Fixed.

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:141
>> +            result.append(device);
> 
> We should probably have done this? Were we listing/using devices that are not enabled?

No, but this will make sense in the next patch.

>> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:57
>> +    for (auto& device : captureDevices()) {
> 
> Maybe you could use findMatching.

I will fix this in a future patch.

>> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:50
>>      CaptureSourceOrError createVideoCaptureSource(const CaptureDevice& device, const MediaConstraints* constraints) final
> 
> Maybe we could pass the persistentId instead of the CaptureDevice?

Good idea, I will do that i the next patch.

>> Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:182
>>          capabilities.addFacingMode(RealtimeMediaSourceSettings::Environment);
> 
> Use a ternary?

This will also make sense in the next patch.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:243
>> +void UserMediaPermissionRequestManagerProxy::requestUserMediaPermissionForFrame(uint64_t userMediaID, uint64_t frameID, Ref<SecurityOrigin>&& userMediaDocumentOrigin, Ref<SecurityOrigin>&& topLevelDocumentOrigin, const MediaStreamRequest& userRequest)
> 
> You can probably use a MediaStreamRequest&& by changing also the signature in WebPageProxy (IPC allows taking an &&).
> A follow-up might be better since tis patch is pretty big.

Will do.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:98
>> +    SecurityOrigin* topLevelDocumentOrigin = userRequest.topLevelDocumentOrigin();
> 
> auto* ?

Fixed.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:99
>>      ASSERT(topLevelDocumentOrigin);
> 
> Not sure we need this assert, or we should add one for userMediaDocumentOrigin as well?

Fixed.

[Eric Carlson]

Created attachment 329807 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329807 [details]
Patch

Rejecting attachment 329807 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 329807, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
urce/WebKit/UIProcess/WebPageProxy.cpp
Hunk #3 succeeded at 5987 (offset 4 lines).
patching file Source/WebKit/UIProcess/WebPageProxy.h
patching file Source/WebKit/UIProcess/WebPageProxy.messages.in
patching file Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp
patching file Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/5758601

[Eric Carlson]

Created attachment 329820 [details]
Patch

[Eric Carlson]

Created attachment 329853 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329853 [details]
Patch

Clearing flags on attachment: 329853

Committed r226160: <https://trac.webkit.org/changeset/226160>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Plus r226178 to fix builds when MEDIA_STREAM is not defined (bug 181026).

[Alex Christensen]

http://trac.webkit.org/r226197

[Philippe Normand]

(In reply to Alex Christensen from comment #17)
> http://trac.webkit.org/r226197

There's also a Source/WebKit/UIProcess/WebPageProxy.cpp.orig fwiw

[Alex Christensen]

Thanks!
http://trac.webkit.org/r226334