Summary: | [MediaStream] Resolve constraints and enumerate devices in the UI process | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||||||||||||||||||||||||
Component: | Media | Assignee: | 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
Eric Carlson
2016-09-19 06:27:44 PDT
Created attachment 289234 [details]
Patch for the bots.
Created attachment 289261 [details]
Another patch for the bots
Created attachment 289263 [details]
Updated patch.
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 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. 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
Created attachment 290765 [details]
Proposed patch.
Created attachment 290768 [details]
Proposed patch.
Created attachment 290769 [details]
Proposed patch.
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 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
Created attachment 291014 [details]
Updated patch.
Created attachment 291015 [details]
Updated patch.
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 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
Created attachment 291549 [details]
Updated patch.
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 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). Created attachment 291792 [details]
Updated patch.
Created attachment 291826 [details]
Updated patch.
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 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. Created attachment 291832 [details]
Patch for landing.
Committed r207463: https://trac.webkit.org/r207463 (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. I think it is a similar compiler related problem as in bug163255 (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. (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. (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. 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. I think that (part of?) the solution would be to implement a CaptureDeviceManager for the OpenWebRTC backend. Filed https://bugs.webkit.org/show_bug.cgi?id=164010 about this. |