Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668023002:
Spec-compatible algorithm for selecting video source and settings. (Closed)
|
Created:
3 years, 10 months ago by Guido Urdaneta Modified:
3 years, 10 months ago CC:
avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, tommi (sloooow) - chröme, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpec-compatible algorithm for selecting video source and settings.
This is a step towards providing spec-compliant constraints support
for getUserMedia and applyConstraints.
This CL will be followed by two CLs to integrate the algorithm into Chromium.
1. A CL that uses this algorithm to select the correct video-capture device in
getUserMedia (settings will still be chosen with the old algorithm).
2. A CL that replaces the existing algorithm for video-source settings in
getUserMedia with the spec-compliant one.
BUG=657733
Review-Url: https://codereview.chromium.org/2668023002
Cr-Commit-Position: refs/heads/master@{#448966}
Committed: https://chromium.googlesource.com/chromium/src/+/983f3e3990d1048b85275d7b455ca6ca78bf5e23
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix windows compile issue #Patch Set 3 : Add documentation and better support for the case without capabilities #Patch Set 4 : remove public kDefaultFailedConstraintName. Use empty string instead. #
Total comments: 12
Patch Set 5 : Address hta@'s comments #Patch Set 6 : rebase #
Total comments: 16
Patch Set 7 : Address hbos' comments #
Total comments: 8
Patch Set 8 : address hbos' comments #
Messages
Total messages: 46 (30 generated)
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Spec-compatible algorithm for selecting video source and settings. This is a step towards providing spec-compliant constraints support for getUserMedia and applyConstraints. This CL will be followed by two CLs to integrate the algorithm into Chromium. 1. A CL that uses this algorithm to select the correct video-capture device in getUserMedia (settings will still be chosen with the old algorithm). 2. A CL that replaces the existing algorithm for video-source settings in getUserMedia with the spec-compliant one. BUG=657733 ========== to ========== Spec-compatible algorithm for selecting video source and settings. This is a step towards providing spec-compliant constraints support for getUserMedia and applyConstraints. This CL will be followed by two CLs to integrate the algorithm into Chromium. 1. A CL that uses this algorithm to select the correct video-capture device in getUserMedia (settings will still be chosen with the old algorithm). 2. A CL that replaces the existing algorithm for video-source settings in getUserMedia with the spec-compliant one. BUG=657733 ==========
guidou@chromium.org changed reviewers: + chfremer@chromium.org, hta@chromium.org, tommi@chromium.org
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Looks pretty nice overall. I have just two small questions/recommendations. https://codereview.chromium.org/2668023002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source.h (right): https://codereview.chromium.org/2668023002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:109: // result with a valid settings value and a null |failed_constraint_name|. What about the case where no devices are present and no constraints are given? https://codereview.chromium.org/2668023002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:148: const blink::WebMediaConstraints& constraints); This algorithm performs three steps. 1. It determines a set of candidates. 2. It ranks the candidates. 3. It picks the top-ranked candidate. I recommend separating the steps and exposing them via additional public methods in this interface. This would give the following advantages: a.) It would allow simpler and more detailed testing. For example it would allow a test case to verify that there are multiple candidates before they are reduced to a single pick by steps 2-3. b.) It would make it easier for us at a later time to use ranking and tie breaking strategies different from the default. For example, we could present candidates to the app or the user and let them select.
Hi, PTAL. https://codereview.chromium.org/2668023002/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source.h (right): https://codereview.chromium.org/2668023002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:109: // result with a valid settings value and a null |failed_constraint_name|. On 2017/01/31 18:23:00, chfremer wrote: > What about the case where no devices are present and no constraints are given? Added documentation and extra test for this case. https://codereview.chromium.org/2668023002/diff/20001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:148: const blink::WebMediaConstraints& constraints); On 2017/01/31 18:23:00, chfremer wrote: > This algorithm performs three steps. > 1. It determines a set of candidates. > 2. It ranks the candidates. > 3. It picks the top-ranked candidate. > > I recommend separating the steps and exposing them via additional public methods > in this interface. This would give the following advantages: > a.) It would allow simpler and more detailed testing. For example it would allow > a test case to verify that there are multiple candidates before they are reduced > to a single pick by steps 2-3. > b.) It would make it easier for us at a later time to use ranking and tie > breaking strategies different from the default. For example, we could present > candidates to the app or the user and let them select. I think this is a good idea for the future, if we decide to implement an approach that requires an intermediate set of ranked candidates. At the moment, the current implementation does not perform the three steps separately, but at the same time by keeping in memory only the current and best candidates. Since this algorithm requires checking all possible permutations of settings, the number of valid candidates may become large if the number of capabilities is large and the number of constraints small. This would then require extra parameters and a more complex structure to keep only the top K candidates in order to avoid using too much memory. Since we currently don't need that I prefer to keep it the way it is for now. I think most of the code can be reused if we later need to separate the steps.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good. A few language nits on commentary, and some suggestions on grouping the (long) list of tests. Nice to see the tests! https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.cc:375: // frameRate constraint. Copy/paste error? https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source.h (right): https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:89: // Note that Chromium performs constraint resolution in two steps. First, a "Note that" adds no information. Remove it. https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:99: // The main different between a source and a track with regards to the spec different -> difference https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source_unittest.cc (right): https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:101: void SelectSettings() { As a matter of style, wouldn't it be prettier if SelectSettings returned the value of result_ rather than storing it in a member variable? This would make it clear that calls to SelectSettings() are independent, and that there's no issue with calling SelectSettings() multiple times from the same test (if needed). https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:207: constraint_factory_.basic().aspectRatio.setMax(0.0); This might cause an "illegal parameter" some day. Can you use 0.1, or would that result in a cropped video? https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:261: TEST_F(MediaStreamConstraintsUtilVideoSourceTest, Unconstrained) { As a matter of style, I'd put the successful test first (the base case), followed by the various ways in which it can be made to fail.
https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.cc:375: // frameRate constraint. On 2017/02/02 00:32:39, hta - Chromium wrote: > Copy/paste error? Done. https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source.h (right): https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:89: // Note that Chromium performs constraint resolution in two steps. First, a On 2017/02/02 00:32:39, hta - Chromium wrote: > "Note that" adds no information. Remove it. Done. https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source.h:99: // The main different between a source and a track with regards to the spec On 2017/02/02 00:32:39, hta - Chromium wrote: > different -> difference Done. https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_constraints_util_video_source_unittest.cc (right): https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:101: void SelectSettings() { On 2017/02/02 00:32:39, hta - Chromium wrote: > As a matter of style, wouldn't it be prettier if SelectSettings returned the > value of result_ rather than storing it in a member variable? > > This would make it clear that calls to SelectSettings() are independent, and > that there's no issue with calling SelectSettings() multiple times from the same > test (if needed). Done. https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:207: constraint_factory_.basic().aspectRatio.setMax(0.0); On 2017/02/02 00:32:39, hta - Chromium wrote: > This might cause an "illegal parameter" some day. > Can you use 0.1, or would that result in a cropped video? > > Done. 0.01 works (minimum supported is 0.05). https://codereview.chromium.org/2668023002/diff/80001/content/renderer/media/... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:261: TEST_F(MediaStreamConstraintsUtilVideoSourceTest, Unconstrained) { On 2017/02/02 00:32:39, hta - Chromium wrote: > As a matter of style, I'd put the successful test first (the base case), > followed by the various ways in which it can be made to fail. Done. Changed the order to: 1. Unconstrained 2. Overconstrained 3. Mandatory/Ideal 4. Advanced 5. NoDevices
friendly ping to reviewers
lgtm https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:47: template <typename ConstraintType> Not for this CL: If you find these functions useful in the next module you write too, perhaps they should be moved to media_stream_constraints_util.h?
guidou@chromium.org changed reviewers: + hbos@chromium.org - tommi@chromium.org
hbos@: can you take a look?
https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:47: template <typename ConstraintType> On 2017/02/07 10:57:41, hta - Chromium wrote: > Not for this CL: > > If you find these functions useful in the next module you write too, perhaps > they should be moved to media_stream_constraints_util.h? Acknowledged.
The code looks good to me, my questions is more about understanding the algorithm and names of functions. I didn't have time to look at the unittests, I can do that too if you want. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:157: // If the source value exceeds the maximum requested, penalize a Complete the sentence :) https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:259: const char** failed_constraint_name) { Should this be called DeviceConstraintSourceDistance? https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:265: failed_constraint_name); HUGE_VAL + HUGE_VAL = HUGE_VAL? DCHECK std::numeric_limits<double>::has_infinity somewhere if the assumption is that HUGE_VAL is inf? Yes, that is an assumption, std::isfinite is used in other checks. Or replace HUGE_VAL usages with std::numeric_limits<double>::infinity() to get away with this "hidden" assumption. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:270: double FormatSourceDistance( Should this be called FormatConstraintSourceDistance? https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:319: double CandidateSourceDistance( Should this be called CandidateConstraintSourceDistance? https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:449: const blink::LongConstraint& constraint) { Can you rename this to PowerLineFrequencyFitnessDistance or rename PowerLineFrequencyConstraintSourceDistance to PowerLineConstraintSourceDistance to have consistent naming of functions? *ConstraintSourceDistance and *FitnessDistance. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:643: // implementation specific and used to break ties. Do I get this correctly...? a) "SourceDistance" aka "ConstraintSourceDistance" are two things: 1) 0/1 based on this distance if infinite or not 2) a distance value Where 1) is according to spec and 2) is a custom value (for post-spec tie breaking). In this step 1) is tested from all advanced constraints. b) "FitnessDistance" from basic constraints - according to spec. c) In this step a.2) is tested. d) "NativeFitnessDistance" from basic constraints - a custom value. e) Distance from default - a custom value. I wonder if the names can be made clearer? "Constraint distance", "fitness distance", "native distance", it took me a while to wrap my head around it. Also, why are "FitnessDistance" and "NativeFitnessDistance" only compared from basic constraints and not other constraints? The more constraints you are close to the better? Or is this because that is already covered by a.2) step?
Patchset #7 (id:140001) has been deleted
https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:157: // If the source value exceeds the maximum requested, penalize a On 2017/02/07 19:17:12, hbos_chromium wrote: > Complete the sentence :) Done. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:259: const char** failed_constraint_name) { On 2017/02/07 19:17:12, hbos_chromium wrote: > Should this be called DeviceConstraintSourceDistance? The convention is that ConstraintSourceDistance is for a single constrain and SourceDistance is for aggregates of ConstraintSourceDistance. You can interpret the Constraint part as being part of the prefix. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:265: failed_constraint_name); On 2017/02/07 19:17:12, hbos_chromium wrote: > HUGE_VAL + HUGE_VAL = HUGE_VAL? DCHECK std::numeric_limits<double>::has_infinity > somewhere if the assumption is that HUGE_VAL is inf? Yes, that is an assumption, > std::isfinite is used in other checks. > > Or replace HUGE_VAL usages with std::numeric_limits<double>::infinity() to get > away with this "hidden" assumption. Done. std::numeric_limits<double>::infinity() also requires has_infinity to be true in order to work. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:270: double FormatSourceDistance( On 2017/02/07 19:17:13, hbos_chromium wrote: > Should this be called FormatConstraintSourceDistance? ditto. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:319: double CandidateSourceDistance( On 2017/02/07 19:17:12, hbos_chromium wrote: > Should this be called CandidateConstraintSourceDistance? ditto. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:449: const blink::LongConstraint& constraint) { On 2017/02/07 19:17:13, hbos_chromium wrote: > Can you rename this to PowerLineFrequencyFitnessDistance or rename > PowerLineFrequencyConstraintSourceDistance to PowerLineConstraintSourceDistance > to have consistent naming of functions? *ConstraintSourceDistance and > *FitnessDistance. Went for PowerLineFrequencyConstraintFitnessDistance. https://codereview.chromium.org/2668023002/diff/120001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:643: // implementation specific and used to break ties. On 2017/02/07 19:17:13, hbos_chromium wrote: > Do I get this correctly...? > > a) "SourceDistance" aka "ConstraintSourceDistance" are two things: > 1) 0/1 based on this distance if infinite or not > 2) a distance value > Where 1) is according to spec and 2) is a custom value (for > post-spec tie breaking). In this step 1) is tested from all > advanced constraints. > b) "FitnessDistance" from basic constraints - according to spec. > c) In this step a.2) is tested. > d) "NativeFitnessDistance" from basic constraints - a custom value. > e) Distance from default - a custom value. > Correct. You understood 100% :) > I wonder if the names can be made clearer? "Constraint distance", "fitness > distance", "native distance", it took me a while to wrap my head around it. > The convention is SourceDistance for (a) and (c), FitnessDistance for (b) and NativeFitnessDistance for (d). The ConstraintSourceDistance suffix is a particular case of the SourceDistance suffix for a single constraint. The SourceDistance suffix (without Constraint) is for aggregates of other SourceDistances (including ConstraintSourceDistances). I updated the fitness-distance function names to follow the same convention of [Constraint]SourceDistance. Also renamed FitnessDistance() (the function, not the prefix) to Distance() since it's used for both FitnessDistances and SourceDistances. > Also, why are "FitnessDistance" and "NativeFitnessDistance" only compared from > basic constraints and not other constraints? The more constraints you are close > to the better? Or is this because that is already covered by a.2) step? The spec says that fitness values should be computed using the basic set. Ideal values of advanced sets should be ignored.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:75: double Distance(double value1, double value2) { Here and other places there may be more precise links? E.g. https://w3c.github.io/mediacapture-main/#dfn-fitness-distance https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source_unittest.cc (right): https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:97: high_res_highest_format_ = &high_res_device_->formats[5]; nit: Can you set these closer to where they are defined with a comment before each block "// Default device", "// Low res device", etc. https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:129: media::VideoCaptureFormat::ToString(result.settings.format())); Do you have to do ToString to compare them? https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:398: media::VideoCaptureFormat::ToString(result.settings.format())); In the Overconstrainted tests you made sure that result.has_value() returned false for exact values. Here and other min- and max- places: Should we also add cases where we can't satisfy a min/max? Optionally also for ranges (both min and max specified), though I think that is already covered by min and max separately.
https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source.cc (right): https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source.cc:75: double Distance(double value1, double value2) { On 2017/02/08 09:45:00, hbos_chromium wrote: > Here and other places there may be more precise links? E.g. > https://w3c.github.io/mediacapture-main/#dfn-fitness-distance Done. https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... File content/renderer/media/media_stream_constraints_util_video_source_unittest.cc (right): https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:97: high_res_highest_format_ = &high_res_device_->formats[5]; On 2017/02/08 09:45:00, hbos_chromium wrote: > nit: Can you set these closer to where they are defined with a comment before > each block "// Default device", "// Low res device", etc. Added the comments, but the pointer initializations have to be made after appending all elements to the vector. Otherwise, the pointers may become invalid due to the addition of extra elements. https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:129: media::VideoCaptureFormat::ToString(result.settings.format())); On 2017/02/08 09:45:00, hbos_chromium wrote: > Do you have to do ToString to compare them? Nice catch. I hadn't realized that there was an equality operator. https://codereview.chromium.org/2668023002/diff/160001/content/renderer/media... content/renderer/media/media_stream_constraints_util_video_source_unittest.cc:398: media::VideoCaptureFormat::ToString(result.settings.format())); On 2017/02/08 09:45:01, hbos_chromium wrote: > In the Overconstrainted tests you made sure that result.has_value() returned false for exact values. > Here and other min- and max- places: Should we also add cases where we can't > satisfy a min/max? The overconstrained tests have cases where min/max can't be satisfied. > Optionally also for ranges (both min and max specified), though I think that is > already covered by min and max separately. We could add the tests, but they would not test any more than the separate min/max tests that are already there, so I prefer not to add them. Also, there are plenty of tests where min/max ranges are tested, so I think we're OK in this regard.
guidou@chromium.org changed reviewers: + jochen@chromium.org
jochen@: can you rs/look at content/renderer/BUILD.gn?
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org, hbos@chromium.org Link to the patchset: https://codereview.chromium.org/2668023002/#ps180001 (title: "address hbos' comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486557015732780,
"parent_rev": "03527510144419d0b9b7b8f0c31c4c199f93a18c", "commit_rev":
"983f3e3990d1048b85275d7b455ca6ca78bf5e23"}
Message was sent while issue was closed.
Description was changed from ========== Spec-compatible algorithm for selecting video source and settings. This is a step towards providing spec-compliant constraints support for getUserMedia and applyConstraints. This CL will be followed by two CLs to integrate the algorithm into Chromium. 1. A CL that uses this algorithm to select the correct video-capture device in getUserMedia (settings will still be chosen with the old algorithm). 2. A CL that replaces the existing algorithm for video-source settings in getUserMedia with the spec-compliant one. BUG=657733 ========== to ========== Spec-compatible algorithm for selecting video source and settings. This is a step towards providing spec-compliant constraints support for getUserMedia and applyConstraints. This CL will be followed by two CLs to integrate the algorithm into Chromium. 1. A CL that uses this algorithm to select the correct video-capture device in getUserMedia (settings will still be chosen with the old algorithm). 2. A CL that replaces the existing algorithm for video-source settings in getUserMedia with the spec-compliant one. BUG=657733 Review-Url: https://codereview.chromium.org/2668023002 Cr-Commit-Position: refs/heads/master@{#448966} Committed: https://chromium.googlesource.com/chromium/src/+/983f3e3990d1048b85275d7b455c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/983f3e3990d1048b85275d7b455c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
