Bug 162147

Summary: [MediaStream] Resolve constraints and enumerate devices in the UI process
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alex, buildbot, cgarcia, clopez, commit-queue, dino, jer.noble, ossy, pnormand, rniwa, thiago.lacerda, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 162571    
Bug Blocks: 162154    
Attachments:
Description Flags
Patch for the bots.
none
Another patch for the bots
none
Updated patch.
buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Proposed patch.
none
Proposed patch.
none
Proposed patch.
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Updated patch.
none
Updated patch.
buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Updated patch.
none
Updated patch.
none
Updated patch.
darin: review+
Patch for landing. none

Description Eric Carlson 2016-09-19 06:27:44 PDT
On OSX and iOS, the WebCore sandbox profile does not allow it to access media capture devices. The UI process will grant sandbox extensions giving access to capture devices after the user has consented to the gUM request, but this is a problem in two cases:

- evaluating the initial constraints passed to gUM to see if the user should be prompted (eg. there is no reason to ask the user for permission if the constraints can't be satisfied at all).
- when MediaDevices.enumerateDevices is called before gUM.

To fix this, both of these tasks should be performed in the UI process.
Comment 1 Eric Carlson 2016-09-19 09:46:49 PDT
Created attachment 289234 [details]
Patch for the bots.
Comment 2 Eric Carlson 2016-09-19 14:45:50 PDT
Created attachment 289261 [details]
Another patch for the bots
Comment 3 Eric Carlson 2016-09-19 15:08:48 PDT
Created attachment 289263 [details]
Updated patch.
Comment 4 WebKit Commit Bot 2016-09-19 15:11:26 PDT
Attachment 289263 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/FeatureDefines.h:582:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2016-09-19 16:25:19 PDT
Comment on attachment 289263 [details]
Updated patch.

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

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2016-09-19 16:25:22 PDT
Created attachment 289276 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Eric Carlson 2016-10-05 17:31:45 PDT
Created attachment 290765 [details]
Proposed patch.
Comment 8 Eric Carlson 2016-10-05 17:59:02 PDT
Created attachment 290768 [details]
Proposed patch.
Comment 9 Eric Carlson 2016-10-05 18:24:32 PDT
Created attachment 290769 [details]
Proposed patch.
Comment 10 Build Bot 2016-10-05 21:08:55 PDT
Comment on attachment 290769 [details]
Proposed patch.

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

New failing tests:
fast/mediastream/MediaDevices-getUserMedia.html
fast/mediastream/getusermedia.html
fast/mediastream/mock-media-source.html
Comment 11 Build Bot 2016-10-05 21:08:58 PDT
Created attachment 290779 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 12 Eric Carlson 2016-10-08 09:45:35 PDT
Created attachment 291014 [details]
Updated patch.
Comment 13 Eric Carlson 2016-10-08 10:28:48 PDT
Created attachment 291015 [details]
Updated patch.
Comment 14 Build Bot 2016-10-08 11:35:25 PDT
Comment on attachment 291015 [details]
Updated patch.

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

New failing tests:
fast/mediastream/MediaDevices-getUserMedia.html
fast/mediastream/getusermedia.html
Comment 15 Build Bot 2016-10-08 11:35:29 PDT
Created attachment 291018 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 16 Eric Carlson 2016-10-13 17:57:06 PDT
Created attachment 291549 [details]
Updated patch.
Comment 17 Darin Adler 2016-10-14 15:40:30 PDT
Comment on attachment 291549 [details]
Updated patch.

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

We should really make a class derived from ContextDestructionObserver called DocumentDestructionObserver so we don’t need all these casts from scriptExecutionContext to Document. That base class would have a document() function.

Really not fond of the MedaiaConstraintsImpl terminology.

Did not finish reviewing. But here are my first batch of comments.

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.cpp:49
> +Ref<MediaConstraintsImpl> MediaConstraintsImpl:: create(const MediaConstraintsData& data)

Stray space here before the word "create".

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:50
> +public:
> +    MediaConstraintsData(MediaTrackConstraintSetMap&& mandatory, Vector<MediaTrackConstraintSetMap>&& advanced, bool valid)
> +    {
> +        mandatoryConstraints = WTFMove(mandatory);
> +        advancedConstraints = WTFMove(advanced);
> +        isValid = valid;
> +    }
> +    explicit MediaConstraintsData() { }

None of this code should be needed. This should all work with { } syntax without defining any constructors. Also, "explicit" has no meaning on a constructor unless it has exactly one non-optional argument, so there’s no value in saying it on an empty constructor like this.

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:68
> +    MediaConstraintsData data() const { return m_data; }

This needs to return const MediaConstraintsData&, otherwise we will copy the entire vector every time this is called!

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:74
> +    MediaConstraintsImpl(const MediaConstraintsData& data)
> +        : m_data(data)
>      {
>      }

This should take an rvalue reference and should use WTFMove.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:62
> +    if (m_scriptExecutionContext)
> +        return m_scriptExecutionContext->securityOrigin();
> +
> +    return nullptr;

WebKit coding style uses early return, so this code would be written in the reverse order.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:70
> +    if (Frame* frame = downcast<Document>(*scriptExecutionContext()).frame()) {

Not clear why this call site calls scriptExecutionContext() but the rest of the same function uses m_scriptExecutionContext. I suggest using the same thing consistently.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:89
> +    UserMediaController* controller = UserMediaController::from(document.page());

Would be better to use auto* here.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:125
> +        Ref<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, WTFMove(privateStream));

Should use auto.

> Source/WebCore/platform/mediastream/CaptureDevice.h:46
> +    explicit CaptureDevice() { }

The use of explicit here is incorrect. Please just omit it. Could also write:

    CaptureDevice() = default;

Most people on the project prefer that style, I think.

> Source/WebCore/platform/mediastream/MediaConstraints.h:90
> +    MediaConstraint() { }
> +    ~MediaConstraint() { }

Could write:

    MediaConstraint() = default;
    ~MediaConstraint() = default;

> Source/WebCore/platform/mediastream/MediaConstraints.h:268
> +    NumericConstraint()

Can we just write:

    NumericConstraint() = default;

> Source/WebCore/platform/mediastream/MediaConstraints.h:269
> +        : MediaConstraint()

Why is this line needed?

> Source/WebCore/platform/mediastream/MediaConstraints.h:313
> +    IntConstraint()
> +        : NumericConstraint()

Same comments as above.

> Source/WebCore/platform/mediastream/MediaConstraints.h:332
> +    DoubleConstraint()
> +        : NumericConstraint()

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:351
> +    BooleanConstraint()
> +        : MediaConstraint()

Ditto.

> Source/WebCore/platform/mediastream/MediaConstraints.h:453
> +    StringConstraint()
> +        : MediaConstraint()

Ditto.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:57
> +    using ValidConstaintsHandler = std::function<void(const Vector<RefPtr<RealtimeMediaSource>>&& audioTracks, const Vector<RefPtr<RealtimeMediaSource>>&& videoTracks)>;

Spelling error: Should be constraints, not "constaints".

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:58
> +    using InvalidConstaintsHandler = std::function<void(const String& invalidConstraint)>;

Ditto.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2257
> +    for (size_t i = 0; i < advancedCount; ++i) {
> +        MediaTrackConstraintSetMap map;
> +        if (!decoder.decode(map))
> +            return false;
> +
> +        advancedConstraints.append(WTFMove(map));
> +    }

Should use reserveInitialCapacity and uncheckedAppend here.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2275
> +    encoder << static_cast<uint64_t>(device.kind());

Why use a 64-bit integer to encode two bits of data? Is that a common pattern in ArgumentEncoder?
Comment 18 Eric Carlson 2016-10-16 21:38:39 PDT
Comment on attachment 291549 [details]
Updated patch.

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

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.cpp:49
>> +Ref<MediaConstraintsImpl> MediaConstraintsImpl:: create(const MediaConstraintsData& data)
> 
> Stray space here before the word "create".

Fixed.

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:50
>> +    explicit MediaConstraintsData() { }
> 
> None of this code should be needed. This should all work with { } syntax without defining any constructors. Also, "explicit" has no meaning on a constructor unless it has exactly one non-optional argument, so there’s no value in saying it on an empty constructor like this.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:68
>> +    MediaConstraintsData data() const { return m_data; }
> 
> This needs to return const MediaConstraintsData&, otherwise we will copy the entire vector every time this is called!

Fixed!

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:74
>>      }
> 
> This should take an rvalue reference and should use WTFMove.

We need a version that takes a const MediaConstraintsData and WTFMove doesn't like const qualified references, so I left this but added a version that takes rvalue references to the MediaConstraintsData fields.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:62
>> +    return nullptr;
> 
> WebKit coding style uses early return, so this code would be written in the reverse order.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:70
>> +    if (Frame* frame = downcast<Document>(*scriptExecutionContext()).frame()) {
> 
> Not clear why this call site calls scriptExecutionContext() but the rest of the same function uses m_scriptExecutionContext. I suggest using the same thing consistently.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.cpp:89
>> +    UserMediaController* controller = UserMediaController::from(document.page());
> 
> Would be better to use auto* here.

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:125
>> +        Ref<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, WTFMove(privateStream));
> 
> Should use auto.

Fixed.

>> Source/WebCore/platform/mediastream/CaptureDevice.h:46
>> +    explicit CaptureDevice() { }
> 
> The use of explicit here is incorrect. Please just omit it. Could also write:
> 
>     CaptureDevice() = default;
> 
> Most people on the project prefer that style, I think.

Changed to use "= default" throughout.

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:57
>> +    using ValidConstaintsHandler = std::function<void(const Vector<RefPtr<RealtimeMediaSource>>&& audioTracks, const Vector<RefPtr<RealtimeMediaSource>>&& videoTracks)>;
> 
> Spelling error: Should be constraints, not "constaints".

Fixed throughout.

>> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2257
>> +    }
> 
> Should use reserveInitialCapacity and uncheckedAppend here.

Fixed.

>> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2275
>> +    encoder << static_cast<uint64_t>(device.kind());
> 
> Why use a 64-bit integer to encode two bits of data? Is that a common pattern in ArgumentEncoder?

It is a common pattern, but I changed to use encodeEnum/decodeEnum for type safety (which uses a 64-bit integer).
Comment 19 Eric Carlson 2016-10-16 21:40:11 PDT
Created attachment 291792 [details]
Updated patch.
Comment 20 Eric Carlson 2016-10-17 09:54:53 PDT
Created attachment 291826 [details]
Updated patch.
Comment 21 Darin Adler 2016-10-17 10:03:02 PDT
Comment on attachment 291826 [details]
Updated patch.

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

Looks good overall.

> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:71
> +    MediaConstraintsData m_data { };

I’m surprised these braces are needed. I would expect this to work the same without the { }.

> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.h:34
> +#include <wtf/RefCounted.h>
> +#include <wtf/text/WTFString.h>

Don’t need to include RefCounted.h if we are also including WTFString.h.

> Source/WebCore/Modules/mediastream/MediaDevicesRequest.h:46
> +class MediaDevicesRequest : public RefCounted<MediaDevicesRequest>, public ContextDestructionObserver {

Might want to derive privately from ContextDestructionObserver instead of publicly.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107
> +        deny(MediaAccessDenialReason::OtherFailure, "");

Should use emptyString() instead of "".

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:121
> +            deny(MediaAccessDenialReason::HardwareError, "");

Should use emptyString() instead of "".

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:127
> +            deny(MediaAccessDenialReason::HardwareError, "");

Should use emptyString() instead of "".

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:163
> +        m_promise.reject(OverconstrainedError::create(invalidConstraint, "Invalid constraint").get());

Should use ASCIILiteral.

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:56
> +class UserMediaRequest : public RefCounted<UserMediaRequest>, public ContextDestructionObserver {

Consider deriving privately from ContextDestructionObserver.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:87
> +    RefPtr<UserMediaRequest> request = m_idToUserMediaRequestMap.take(requestID);

I suggest using auto here instead of writing out RefPtr<UserMediaRequest>.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:137
> +    RefPtr<MediaDevicesEnumerationRequest> request = m_idToMediaDevicesEnumerationRequestMap.take(requestID);

Ditto.
Comment 22 Eric Carlson 2016-10-17 10:55:48 PDT
Comment on attachment 291826 [details]
Updated patch.

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

>> Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:71
>> +    MediaConstraintsData m_data { };
> 
> I’m surprised these braces are needed. I would expect this to work the same without the { }.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesEnumerationRequest.h:34
>> +#include <wtf/text/WTFString.h>
> 
> Don’t need to include RefCounted.h if we are also including WTFString.h.

Fixed.

>> Source/WebCore/Modules/mediastream/MediaDevicesRequest.h:46
>> +class MediaDevicesRequest : public RefCounted<MediaDevicesRequest>, public ContextDestructionObserver {
> 
> Might want to derive privately from ContextDestructionObserver instead of publicly.

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107
>> +        deny(MediaAccessDenialReason::OtherFailure, "");
> 
> Should use emptyString() instead of "".

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:121
>> +            deny(MediaAccessDenialReason::HardwareError, "");
> 
> Should use emptyString() instead of "".

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:127
>> +            deny(MediaAccessDenialReason::HardwareError, "");
> 
> Should use emptyString() instead of "".

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:163
>> +        m_promise.reject(OverconstrainedError::create(invalidConstraint, "Invalid constraint").get());
> 
> Should use ASCIILiteral.

Fixed.

>> Source/WebCore/Modules/mediastream/UserMediaRequest.h:56
>> +class UserMediaRequest : public RefCounted<UserMediaRequest>, public ContextDestructionObserver {
> 
> Consider deriving privately from ContextDestructionObserver.

Fixed.

>> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:87
>> +    RefPtr<UserMediaRequest> request = m_idToUserMediaRequestMap.take(requestID);
> 
> I suggest using auto here instead of writing out RefPtr<UserMediaRequest>.

Fixed.

>> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:137
>> +    RefPtr<MediaDevicesEnumerationRequest> request = m_idToMediaDevicesEnumerationRequestMap.take(requestID);
> 
> Ditto.

Fixed.
Comment 23 Eric Carlson 2016-10-17 10:56:48 PDT
Created attachment 291832 [details]
Patch for landing.
Comment 24 Eric Carlson 2016-10-18 07:58:35 PDT
Committed r207463: https://trac.webkit.org/r207463
Comment 25 Csaba Osztrogonác 2016-10-18 08:23:27 PDT
(In reply to comment #24)
> Committed r207463: https://trac.webkit.org/r207463

It broke the GTK build, see build.webkit.org for details.

Additionally it would be great to have exactly same configuration on 
the GTK-EWS as buildbots on build.webkit.org to avoid similar breakages.
Comment 26 Csaba Osztrogonác 2016-10-18 08:29:43 PDT
I think it is a similar compiler related problem as in bug163255
Comment 27 Eric Carlson 2016-10-18 08:46:16 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > Committed r207463: https://trac.webkit.org/r207463
> 
> It broke the GTK build, see build.webkit.org for details.
> 

Sorry about that!

> Additionally it would be great to have exactly same configuration on 
> the GTK-EWS as buildbots on build.webkit.org to avoid similar breakages.

Yes, having a different configurations makes it very difficult to avoid breakage for those of us not set up to build every port.
Comment 28 Carlos Garcia Campos 2016-10-19 00:19:54 PDT
(In reply to comment #26)
> I think it is a similar compiler related problem as in bug163255

Yes, it's similar issue, I've just committed a build fix, because the build has been broken for long time. If the fix is not good, please file a new bug report to discuss how to properly fix it. Now I have more build issues to fix that were introduced after this one. I've already asked the GTK+ EWS bot maintainers to downgrade the GCC version there, sorry about this.
Comment 29 Csaba Osztrogonác 2016-10-20 09:11:32 PDT
(In reply to comment #24)
> Committed r207463: https://trac.webkit.org/r207463

Additionally it broke the Apple Mac cmake build too:

/Volumes/Data/slave/elcapitan-cmake-debug/build/Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:24:10: fatal error: 'WebCore/UserMediaRequest.h' file not found
#include <WebCore/UserMediaRequest.h>
         ^
1 error generated.

I have no idea how Apple Mac cmake port 
generates forwarding headers, cc-ing Alex.
Comment 30 Philippe Normand 2016-10-26 02:58:35 PDT
This broke the GTK port. I can now see two different RealtimeMediaSourceCenter instances, one in the webprocess and one in the UI process, I suppose.

This is an issue because in one instance we check the constraints (validateRequestConstraints) and in the other we try to create sources without validating constraints, and that fails because the m_sourceMap hashmap of the the instance running in the webprocess is empty.
Comment 31 Philippe Normand 2016-10-26 03:31:15 PDT
I think that (part of?) the solution would be to implement a CaptureDeviceManager for the OpenWebRTC backend.
Comment 32 Philippe Normand 2016-10-26 04:11:22 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=164010 about this.