Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(292)

Issue 2790823002: Spec compliant video constraints for getUserMedia behind flag. (Closed)

Created:
3 years, 8 months ago by Guido Urdaneta
Modified:
3 years, 8 months ago
CC:
avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, emircan+watch+capturefromdom_chromium.org, feature-media-reviews_chromium.org, hta - Chromium, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+capturefromelement_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Spec compliant video constraints for getUserMedia behind flag. The core of this CL is a modification to classes that currently process constraints (but shouldn't): - UserMediaClientImpl - MediaStreamVideoSource - MediaStreamVideoCapturerSource - MediaStreamVideoWebRtcSink With this CL these classes, when a flag is enabled, instead of processing constraints, they process settings previously produced by a spec-compliant constraints-processing algorithm. BUG=657733 Review-Url: https://codereview.chromium.org/2790823002 Cr-Commit-Position: refs/heads/master@{#462417} Committed: https://chromium.googlesource.com/chromium/src/+/7f1e2a9882f033206ffac41e8f1b5b0ee7aee634

Patch Set 1 #

Patch Set 2 : Update other tests affected by the new constraints algorithm #

Patch Set 3 : fix tests #

Total comments: 30

Patch Set 4 : address hbos' comments #

Patch Set 5 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1731 lines, -347 lines) Patch
M content/browser/webrtc/webrtc_getusermedia_browsertest.cc View 1 2 3 6 chunks +53 lines, -6 lines 0 comments Download
M content/public/common/content_features.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_features.cc View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/renderer/media_stream_utils.cc View 1 2 3 4 1 chunk +2 lines, -5 lines 0 comments Download
M content/renderer/media/media_stream_center.cc View 1 chunk +20 lines, -11 lines 0 comments Download
M content/renderer/media/media_stream_constraints_util.cc View 1 2 3 chunks +9 lines, -3 lines 0 comments Download
M content/renderer/media/media_stream_video_capturer_source.h View 4 chunks +12 lines, -0 lines 0 comments Download
M content/renderer/media/media_stream_video_capturer_source.cc View 1 2 3 14 chunks +203 lines, -44 lines 0 comments Download
M content/renderer/media/media_stream_video_capturer_source_unittest.cc View 1 2 3 11 chunks +156 lines, -7 lines 0 comments Download
M content/renderer/media/media_stream_video_renderer_sink_unittest.cc View 1 2 chunks +6 lines, -5 lines 0 comments Download
M content/renderer/media/media_stream_video_source.h View 8 chunks +36 lines, -7 lines 0 comments Download
M content/renderer/media/media_stream_video_source.cc View 1 12 chunks +128 lines, -17 lines 0 comments Download
M content/renderer/media/media_stream_video_source_unittest.cc View 1 2 3 42 chunks +582 lines, -102 lines 0 comments Download
M content/renderer/media/media_stream_video_track.h View 1 2 3 5 chunks +62 lines, -5 lines 0 comments Download
M content/renderer/media/media_stream_video_track.cc View 1 6 chunks +95 lines, -6 lines 0 comments Download
M content/renderer/media/media_stream_video_track_unittest.cc View 1 2 3 12 chunks +83 lines, -15 lines 0 comments Download
M content/renderer/media/mock_media_stream_registry.h View 1 3 chunks +9 lines, -0 lines 0 comments Download
M content/renderer/media/mock_media_stream_registry.cc View 1 3 chunks +35 lines, -3 lines 0 comments Download
M content/renderer/media/mock_media_stream_video_source.h View 2 chunks +4 lines, -0 lines 0 comments Download
M content/renderer/media/mock_media_stream_video_source.cc View 3 chunks +19 lines, -1 line 0 comments Download
M content/renderer/media/pepper_to_video_track_adapter.cc View 1 chunk +1 line, -5 lines 0 comments Download
M content/renderer/media/remote_media_stream_impl.cc View 1 chunk +3 lines, -7 lines 0 comments Download
M content/renderer/media/rtc_peer_connection_handler_unittest.cc View 1 2 3 4 1 chunk +2 lines, -4 lines 0 comments Download
M content/renderer/media/user_media_client_impl.h View 3 chunks +2 lines, -4 lines 0 comments Download
M content/renderer/media/user_media_client_impl.cc View 11 chunks +48 lines, -19 lines 0 comments Download
M content/renderer/media/video_track_adapter.h View 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/media/video_track_adapter.cc View 1 chunk +7 lines, -0 lines 0 comments Download
M content/renderer/media/webrtc/media_stream_remote_video_source_unittest.cc View 1 5 chunks +14 lines, -17 lines 0 comments Download
M content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc View 5 chunks +56 lines, -31 lines 0 comments Download
M content/renderer/media/webrtc/media_stream_video_webrtc_sink_unittest.cc View 1 2 3 6 chunks +58 lines, -2 lines 0 comments Download
M content/renderer/media/webrtc/webrtc_media_stream_adapter_unittest.cc View 1 1 chunk +1 line, -4 lines 0 comments Download
M content/renderer/media_capture_from_element/canvas_capture_handler.cc View 2 chunks +1 line, -5 lines 0 comments Download
M content/renderer/media_recorder/video_track_recorder_unittest.cc View 1 2 3 4 2 chunks +2 lines, -3 lines 0 comments Download
M content/renderer/pepper/pepper_media_stream_video_track_host.cc View 2 chunks +15 lines, -7 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 43 (30 generated)
Guido Urdaneta
Hi, PTAL This CL still needs most tests to be updated to use the new ...
3 years, 8 months ago (2017-03-31 03:50:16 UTC) #5
tommi (sloooow) - chröme
I looked over the change and it lgtm (mostly rs) but I'd like hbos to ...
3 years, 8 months ago (2017-03-31 08:55:33 UTC) #8
Guido Urdaneta
bbudge@chromium.org: Please review changes in content/renderer/pepper mcasas@chromium.org: Please review changes in content/renderer/media_capture_from_element and content/renderer/media_recorder (feel ...
3 years, 8 months ago (2017-04-03 17:00:39 UTC) #12
mcasas
On 2017/04/03 17:00:39, Guido Urdaneta wrote: > mailto:bbudge@chromium.org: Please review changes in content/renderer/pepper > > ...
3 years, 8 months ago (2017-04-04 16:19:28 UTC) #19
bbudge
content/renderer/pepper LGTM stamp
3 years, 8 months ago (2017-04-04 19:54:22 UTC) #20
hbos_chromium
https://codereview.chromium.org/2790823002/diff/40001/content/browser/webrtc/webrtc_getusermedia_browsertest.cc File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/2790823002/diff/40001/content/browser/webrtc/webrtc_getusermedia_browsertest.cc#newcode537 content/browser/webrtc/webrtc_getusermedia_browsertest.cc:537: expected_result); ScopedFeature thingy to test both old and new ...
3 years, 8 months ago (2017-04-05 12:37:28 UTC) #21
Guido Urdaneta
hbos: PTAL https://codereview.chromium.org/2790823002/diff/40001/content/browser/webrtc/webrtc_getusermedia_browsertest.cc File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/2790823002/diff/40001/content/browser/webrtc/webrtc_getusermedia_browsertest.cc#newcode537 content/browser/webrtc/webrtc_getusermedia_browsertest.cc:537: expected_result); On 2017/04/05 12:37:27, hbos_chromium wrote: > ...
3 years, 8 months ago (2017-04-05 16:17:17 UTC) #23
hbos_chromium
lgtm https://codereview.chromium.org/2790823002/diff/40001/content/renderer/media/media_stream_video_capturer_source.cc File content/renderer/media/media_stream_video_capturer_source.cc (right): https://codereview.chromium.org/2790823002/diff/40001/content/renderer/media/media_stream_video_capturer_source.cc#newcode440 content/renderer/media/media_stream_video_capturer_source.cc:440: // VideoCaptureDelegate Implementation. On 2017/04/05 16:17:16, Guido Urdaneta ...
3 years, 8 months ago (2017-04-05 16:56:31 UTC) #26
Guido Urdaneta
kinuko@chromium.org: Can you take a look at content/public to complete the review?
3 years, 8 months ago (2017-04-05 17:00:31 UTC) #30
Guido Urdaneta
jochen: Can you take a look at content/public?
3 years, 8 months ago (2017-04-06 10:32:28 UTC) #36
jochen (gone - plz use gerrit)
lgtm
3 years, 8 months ago (2017-04-06 10:34:19 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2790823002/100001
3 years, 8 months ago (2017-04-06 10:37:21 UTC) #40
commit-bot: I haz the power
3 years, 8 months ago (2017-04-06 10:42:20 UTC) #43
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7f1e2a9882f033206ffac41e8f1b...

Powered by Google App Engine
This is Rietveld 408576698