Summary: | [MediaStream] apply constraints passed to getUserMedia() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, dino, jer.noble, pnormand, rniwa, thiago.lacerda, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Other | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Eric Carlson
2016-11-09 14:38:26 PST
Created attachment 294366 [details]
Proposed patch.
Attachment 294366 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:47: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:55: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:55: More than one command on the same line [whitespace/newline] [4]
Total errors found: 3 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 294366 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=294366&action=review > Source/WebCore/platform/mediastream/MediaConstraints.h:279 > + value = m_ideal.value(); > + if (value < min) > + value = min; > + if (value > max) > + value = max; You could write this as value = std::max(max, std::min(min, m_ideal.value())); > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:168 > + if (fitnessDistance(*widthConstraint) == std::numeric_limits<double>::infinity()) { We traditionally use isinf) to compare against infinity. Created attachment 294372 [details]
Updated patch.
Attachment 294372 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:47: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/mediastream/mac/MockRealtimeVideoSourceMac.mm:55: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:55: More than one command on the same line [whitespace/newline] [4]
Total errors found: 3 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #4) > Comment on attachment 294366 [details] > Proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294366&action=review > > > Source/WebCore/platform/mediastream/MediaConstraints.h:279 > > + value = m_ideal.value(); > > + if (value < min) > > + value = min; > > + if (value > max) > > + value = max; > > You could write this as value = std::max(max, std::min(min, > m_ideal.value())); > Fixed. > > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:168 > > + if (fitnessDistance(*widthConstraint) == std::numeric_limits<double>::infinity()) { > > We traditionally use isinf) to compare against infinity. > Fixed. Comment on attachment 294372 [details] Updated patch. Attachment 294372 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2490805 New failing tests: fast/mediastream/apply-constraints-audio.html fast/mediastream/apply-constraints-advanced.html fast/mediastream/apply-constraints-video.html Created attachment 294377 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 294372 [details] Updated patch. r=me once bots are all happy. Some nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=294372&action=review > Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:113 > + if (RealtimeMediaSource* mediaSource = createMediaSourceForCaptureDeviceWithConstraints(captureDevice, constraints, invalidConstraint)) I would use auto for mediaSource. It seems strange to retrieve a raw pointer also. Why createMediaSourceForCaptureDeviceWithConstraints is not returning a RefPtr<>? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:164 > + RealtimeMediaSourceCapabilities& capabilities = *this->capabilities(); We probably should return a RealtimeMediaSourceCapabilities& directly. Maybe add an ASSERT here in the meantime? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:199 > + // Each of the values is supported individually, see of they all can be applied at the same time. s/of/if/ > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:587 > + { Why do we need that indenting? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:588 > + RealtimeMediaSourceCapabilities& capabilities = *this->capabilities(); ASSERT again? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:629 > void RealtimeMediaSource::applyConstraints(const MediaConstraints& constraints, SuccessHandler successHandler, FailureHandler failureHandler) It seems strange to pass callbacks. I was expecting applyConstraints to work asynchronously, but it seems everything is done synchronously. Can we simplify this? It seems this function could just return Optional<String> or String if a null string means no error? > Source/WebCore/platform/mediastream/openwebrtc/RealtimeMediaSourceCenterOwr.cpp:101 > + // FIXME: Actually apply constraints to newly created sources. two spaces after to Created attachment 294397 [details]
Patch for landing.
Comment on attachment 294397 [details] Patch for landing. Clearing flags on attachment: 294397 Committed r208559: <http://trac.webkit.org/changeset/208559> All reviewed patches have been landed. Closing bug. |