Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1617243005:
Apply new-style constraints to video_source. (Closed)
|
Created:
4 years, 11 months ago by hta - Chromium Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, philipj_slow, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionApply new-style constraints to video_source.
This CL reimplements the algorithms presently used for selection
in terms of the new form constraints.
A new mock constraints factory is also added.
BUG=543997
Committed: https://crrev.com/b9487f883540ecb571ca4c6163753770db6b8639
Cr-Commit-Position: refs/heads/master@{#372082}
Patch Set 1 : Passing all unit tests. #Patch Set 2 : Add <vector> include #
Total comments: 41
Patch Set 3 : Review comments addressed #
Total comments: 14
Patch Set 4 : Addressing review comments #Patch Set 5 : Rebased #Messages
Total messages: 39 (19 generated)
Description was changed from ========== Apply new-style constraints to video_source. This CL reimplements the algorithms presently used for selection in terms of the new form constraints. A new mock constraints factory is also added. BUG=543997 ========== to ========== Apply new-style constraints to video_source. This CL reimplements the algorithms presently used for selection in terms of the new form constraints. A new mock constraints factory is also added. BUG=543997 ==========
hta@chromium.org changed reviewers: + jochen@chromium.org, tommi@chromium.org
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617243005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617243005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617243005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617243005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617243005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617243005/40001
I think it's time to review. Worthy of discussion: - I scattered a number of DVLOG(3) in the code while trying to understand what's happening. Should I take them all out again? - There's some usage of lists of names around here that led to some vector operations that don't look pretty. Are there ways more recommended? But it sure passes a lot of tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617243005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617243005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Did a drive-by and had a few comments, mostly Chromium-isms. Don't we have WebKit LayoutTests to adapt for this CL? Oh you can also remove all PS#s before the one you sent for review, it'll make things clearer. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:52: // const char kGooglePrefix[] = "goog"; Remove. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:124: // Returns true if |constraint| is fulfilled. |format| can be changed by a Update comment: |constraint| is no longer there. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:141: DVLOG(3) << "Constraint width min is " << constraints.width.min(); Prefer DVLOG_IF(3, constraints.width.hasMin()) << "Constraint width min is " << constraints.width.min(); so the whole statement is defined-out if !DCHECK_IS_ON() https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:155: double value = constraints.frameRate.max(); const https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:166: return true; If one side of the conditional has {}, the other must have it too. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:171: blink::WebString::fromUTF8(failing_constraint_name); Multi-line if bodies must have {} https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:174: << failing_constraint_name; What about DLOG(ERROR) or DLOG(WARNING)? https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:179: // |constraint|. Update comment. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:195: } Couldn't we use something like: std::remove_if(formats.begin(), formats.end(), UpdateFormatForConstraints), formats.end(); Plus, UpdateFormatForConstraints already prints plenty to the Chrome console so I'd remove this one here. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:230: if (min_aspect_ratio > max_aspect_ratio || max_aspect_ratio < 0.05f) { Shouldn't |0.05f| be a constant defined in an anonymous namespace on top of the file and rename like a |kAllowedMinAspectRatio| ? https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:235: blink::WebString::fromUTF8(basic.aspectRatio.name()); Multi line bodies should be inside {} https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:261: // http://dev.w3.org/2011/webrtc/editor/getusermedia.html#idl-def-Constraints Perhaps update this ref, might be stale. (2011 :? ) https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:441: DVLOG(3) << "MediastreamVideoSource::OnSupportedFormats()"; I'd rather use DVLOG(3) << __FUNCTION__; and in that case move it to the first statement of the method's body. Remove all those before committing. Same elsewhere, prefer __FUNCTION__. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/mock_constraint_factory.cc (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. s/2014/2016/, but probably this file will merge in media_stream_video_source_unittest.cc https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/mock_constraint_factory.h (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:15: class MockConstraintFactory { This Mock is quite small and I only see it used in media_stream_video_source_unittest.cc. Do you have plans to use it immediately elsewhere? If not, I'd move it inside said file. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_tracker.cc (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_tracker.cc:53: #if 0 Please remove this before committing. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_tracker.cc:55: // For now, return as if constraints were empty. TODO()s must have a http://crbug.com/... bug associated. https://codereview.chromium.org/1617243005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/exported/WebMediaConstraints.cpp (right): https://codereview.chromium.org/1617243005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/exported/WebMediaConstraints.cpp:395: if (result == goodNames.end()) { No need for |result|?
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
seems like mcases has this covered https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:147: (constraints.width.hasMax() && constraints.width.max() <= 0)) please add {} around bodies https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/mock_constraint_factory.h (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:22: blink::WebMediaTrackConstraintSet& add_advanced(); AddAdvanced() https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:27: }; disallow copy/assign
PTAL - addressed comments. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:52: // const char kGooglePrefix[] = "goog"; On 2016/01/26 16:08:02, mcasas wrote: > Remove. Done. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:124: // Returns true if |constraint| is fulfilled. |format| can be changed by a On 2016/01/26 16:08:01, mcasas wrote: > Update comment: |constraint| is no longer there. Done. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:141: DVLOG(3) << "Constraint width min is " << constraints.width.min(); On 2016/01/26 16:08:02, mcasas wrote: > Prefer > DVLOG_IF(3, constraints.width.hasMin()) << "Constraint width min is " << > constraints.width.min(); > > so the whole statement is defined-out if !DCHECK_IS_ON() Removed. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:147: (constraints.width.hasMax() && constraints.width.max() <= 0)) On 2016/01/26 17:10:13, jochen wrote: > please add {} around bodies Removed instead. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:155: double value = constraints.frameRate.max(); On 2016/01/26 16:08:01, mcasas wrote: > const Done. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:171: blink::WebString::fromUTF8(failing_constraint_name); On 2016/01/26 16:08:01, mcasas wrote: > Multi-line if bodies must have {} It wasn't a multiline body until git cl format touched it :-) https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:174: << failing_constraint_name; On 2016/01/26 16:08:01, mcasas wrote: > What about DLOG(ERROR) or DLOG(WARNING)? Removed. It's a normal thing - happens for every format that doesn't satisfy the constraints. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:179: // |constraint|. On 2016/01/26 16:08:02, mcasas wrote: > Update comment. Done. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:195: } On 2016/01/26 16:08:01, mcasas wrote: > Couldn't we use something like: > > std::remove_if(formats.begin(), formats.end(), UpdateFormatForConstraints), > formats.end(); UpdateFormatsForConstraints has two parameters, remove_if requires that it's unary - and the otherwise tempting "format_it : formats" will barf at the in-loop erase. This is actually pretty simple. > > Plus, UpdateFormatForConstraints already prints plenty to > the Chrome console so I'd remove this one here. Removed. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:230: if (min_aspect_ratio > max_aspect_ratio || max_aspect_ratio < 0.05f) { On 2016/01/26 16:08:02, mcasas wrote: > Shouldn't |0.05f| be a constant defined in an anonymous > namespace on top of the file and rename like a > |kAllowedMinAspectRatio| ? Probably should. And 1:20 is a strange limit - there are specs that fixate on 1:8, and there are specs that fixate on "minimum 8 pixels high", but 1:20 seems to come out of nowhere. But this is from previous code. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:235: blink::WebString::fromUTF8(basic.aspectRatio.name()); On 2016/01/26 16:08:01, mcasas wrote: > Multi line bodies should be inside {} Done. But those 3 lines should really be a helper. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:261: // http://dev.w3.org/2011/webrtc/editor/getusermedia.html#idl-def-Constraints On 2016/01/26 16:08:01, mcasas wrote: > Perhaps update this ref, might be stale. (2011 :? ) The 2011 is part of the W3C naming system for stable specs. But it's now a redirect, and there's a better place to point it anyway. Updated. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:441: DVLOG(3) << "MediastreamVideoSource::OnSupportedFormats()"; On 2016/01/26 16:08:01, mcasas wrote: > I'd rather use DVLOG(3) << __FUNCTION__; and in > that case move it to the first statement of the method's > body. Remove all those before committing. Same > elsewhere, prefer __FUNCTION__. Removed. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/mock_constraint_factory.cc (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/01/26 16:08:02, mcasas wrote: > s/2014/2016/, but probably this file will merge in > media_stream_video_source_unittest.cc Done. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/mock_constraint_factory.h (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:15: class MockConstraintFactory { On 2016/01/26 16:08:02, mcasas wrote: > This Mock is quite small and I only see it used in > media_stream_video_source_unittest.cc. Do you > have plans to use it immediately elsewhere? If > not, I'd move it inside said file. I intend to nuke mock_media_constraint_factory when I'm done converting all its users to using the new APIs. I eventually expect all the current users of mock_media_constraint_factory (7 unittests and a couple of other files) to be users of this factory. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:22: blink::WebMediaTrackConstraintSet& add_advanced(); On 2016/01/26 17:10:13, jochen wrote: > AddAdvanced() Done. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:27: }; On 2016/01/26 17:10:13, jochen wrote: > disallow copy/assign Done. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_tracker.cc (right): https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_tracker.cc:53: #if 0 On 2016/01/26 16:08:02, mcasas wrote: > Please remove this before committing. Change undone - will be done properly in later CL. https://codereview.chromium.org/1617243005/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_tracker.cc:55: // For now, return as if constraints were empty. On 2016/01/26 16:08:02, mcasas wrote: > TODO()s must have a http://crbug.com/... bug associated. Acknowledged. https://codereview.chromium.org/1617243005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/exported/WebMediaConstraints.cpp (right): https://codereview.chromium.org/1617243005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/exported/WebMediaConstraints.cpp:395: if (result == goodNames.end()) { On 2016/01/26 16:08:02, mcasas wrote: > No need for |result|? I thought so, but it turned out not to be the case. Folded.
LGTM with some comments. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:139: failing_constraint_name = constraints.width.name(); {} here and around l. 143 https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:175: // Modify the format_it to fulfill the constraint if possible. Modify |format_it| to fulfill the constraints ... https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:190: blink::WebString* unsatisfied_constraint) { nit: plenty of checks for if (unsatisfied_constraint) should be done at the highest level of this file and dealt with properly. I.e. is it a legit call if there is no WebString to leave the failing constraint's name? If so we might consider working with an std::string in this file's private functions and wrapping it in the WebString at the entry level, WDYT? https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:254: for (const auto& constraint_set : advanced) { s/advanced/constraints.advanced()/ ? https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:256: FilterFormatsByConstraints(constraint_set, ¤t_candidates, nullptr); Just to be sure, no s/nullptr/unsatisfied_constraint/ here ? https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:355: // should be disregarded even if it's nonsensical. nit: is this a TODO() or just a comment...? Note that there is an InvalidOptionalConstraint, so feel free to remove this test. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... File content/renderer/media/mock_constraint_factory.h (right): https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:25: blink::WebMediaTrackConstraintSet basic_; const?
The CQ bit was checked by hta@chromium.org to run a CQ dry run
I think all comments are addressed now. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:139: failing_constraint_name = constraints.width.name(); On 2016/01/28 01:57:57, mcasas wrote: > {} here and around l. 143 https://google.github.io/styleguide/cppguide.html#Conditionals "However, if one part of an if-else statement uses curly braces, the other part must too" Done. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:175: // Modify the format_it to fulfill the constraint if possible. On 2016/01/28 01:57:57, mcasas wrote: > Modify |format_it| to fulfill the constraints ... Done. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:190: blink::WebString* unsatisfied_constraint) { On 2016/01/28 01:57:57, mcasas wrote: > nit: plenty of checks for if (unsatisfied_constraint) > should be done at the highest level of this file and > dealt with properly. I.e. is it a legit call if there is > no WebString to leave the failing constraint's name? > If so we might consider working with an std::string > in this file's private functions and wrapping it in the > WebString at the entry level, WDYT? absolutely. The caller knows if it's going to report it (basic) or ignore it (advanced), the callee doesn't need to know. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:254: for (const auto& constraint_set : advanced) { On 2016/01/28 01:57:57, mcasas wrote: > s/advanced/constraints.advanced()/ ? Done. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:256: FilterFormatsByConstraints(constraint_set, ¤t_candidates, nullptr); On 2016/01/28 01:57:57, mcasas wrote: > Just to be sure, no s/nullptr/unsatisfied_constraint/ here ? It throws the information away, but it can do that using a std::string. Changed. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:355: // should be disregarded even if it's nonsensical. On 2016/01/28 01:57:57, mcasas wrote: > nit: is this a TODO() or just a comment...? > Note that there is an InvalidOptionalConstraint, > so feel free to remove this test. I'm making a change in behavior, and I think the old behavior (which is what this test tested for) was wrong - the comment was intended to elicit comment on this point :-) I'll delete the test. https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... File content/renderer/media/mock_constraint_factory.h (right): https://codereview.chromium.org/1617243005/diff/80001/content/renderer/media/... content/renderer/media/mock_constraint_factory.h:25: blink::WebMediaTrackConstraintSet basic_; On 2016/01/28 01:57:57, mcasas wrote: > const? No. The whole point of it is that it can be changed (unlike the one inside a WebMediaConstraints, which is only changed inside the Blink create() methods). (the class might have been called a MockConstraintBuilder rather than a MockConstraintFactory, but I did not want to depart that far from previous naming.)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617243005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617243005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1617243005/#ps120001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617243005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617243005/120001
Message was sent while issue was closed.
Description was changed from ========== Apply new-style constraints to video_source. This CL reimplements the algorithms presently used for selection in terms of the new form constraints. A new mock constraints factory is also added. BUG=543997 ========== to ========== Apply new-style constraints to video_source. This CL reimplements the algorithms presently used for selection in terms of the new form constraints. A new mock constraints factory is also added. BUG=543997 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Apply new-style constraints to video_source. This CL reimplements the algorithms presently used for selection in terms of the new form constraints. A new mock constraints factory is also added. BUG=543997 ========== to ========== Apply new-style constraints to video_source. This CL reimplements the algorithms presently used for selection in terms of the new form constraints. A new mock constraints factory is also added. BUG=543997 Committed: https://crrev.com/b9487f883540ecb571ca4c6163753770db6b8639 Cr-Commit-Position: refs/heads/master@{#372082} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b9487f883540ecb571ca4c6163753770db6b8639 Cr-Commit-Position: refs/heads/master@{#372082} |
