Introduce SelectSettings algorithm for MediaStream video tracks.

Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Use a single class VideoCaptureSettings for the results of both 
  content and device capture. They differed only in the facing_mode
  field.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so in terms of
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Replaces rtc::Optional with base::Optional.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const), or to better 
  express intent.
* Fixes minor lint issues in existing source files.

BUG=657733
Review-Url: https://codereview.chromium.org/2777703002
Cr-Commit-Position: refs/heads/master@{#460403}
Committed: https://chromium.googlesource.com/chromium/src/+/eff69cf4c3028e0bb4e15c78b137c6bb26966974

[Guido Urdaneta, 2017-03-25 19:22:34]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 19:22:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 20:37:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 20:37:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Guido Urdaneta, 2017-03-25 20:55:24]

Description was changed from

==========
Introduce SelectSettings algorithm for video tracks.

This CL also:
* Renames existing functions for selecting source settings using a
  SelectSettings prefix and dropping "Source" from the name, since now
  they also compute track settings.
* Introduces a VideoTrackAdapter::Settings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Makes the default width and height parameters in
  ResolutionSet::SelectClosestPointToIdeal instead of using
  MediaStreamVideoSource constants. This gives more flexibility, such as
  allowing the use source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards not using MediaStreamVideoSource defaults
  in screen capture. See http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Fixes minor lint issues in existing source files.

BUG=657733
==========

to

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames existing functions for selecting source settings using a
  SelectSettings prefix and dropping "Source" from the name, since now
  they also compute track settings.
* Introduces a VideoTrackAdapter::Settings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Makes the default width and height parameters in
  ResolutionSet::SelectClosestPointToIdeal instead of using
  MediaStreamVideoSource constants. This gives more flexibility, such as
  allowing the use source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards not using MediaStreamVideoSource defaults
  in screen capture. See http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Fixes minor lint issues in existing source files.

BUG=657733
==========

[Guido Urdaneta, 2017-03-26 00:49:08]

Patchset #1 (id:1) has been deleted

[Guido Urdaneta, 2017-03-26 00:49:30]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 00:49:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 01:59:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 01:59:04]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-03-26 02:05:34]

Patchset #1 (id:20001) has been deleted

[Guido Urdaneta, 2017-03-26 02:09:06]

Description was changed from

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames existing functions for selecting source settings using a
  SelectSettings prefix and dropping "Source" from the name, since now
  they also compute track settings.
* Introduces a VideoTrackAdapter::Settings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Makes the default width and height parameters in
  ResolutionSet::SelectClosestPointToIdeal instead of using
  MediaStreamVideoSource constants. This gives more flexibility, such as
  allowing the use source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards not using MediaStreamVideoSource defaults
  in screen capture. See http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Fixes minor lint issues in existing source files.

BUG=657733
==========

to

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapter::Settings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Makes the default width and height in
  ResolutionSet::SelectClosestPointToIdeal parameters instead of using
  MediaStreamVideoSource constants. This gives more flexibility, such
  as allowing the use source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Fixes minor lint issues in existing source files.

BUG=657733
==========

[Guido Urdaneta, 2017-03-26 02:09:17]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 02:09:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 03:50:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 03:50:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Guido Urdaneta, 2017-03-27 10:08:03]

Description was changed from

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapter::Settings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Makes the default width and height in
  ResolutionSet::SelectClosestPointToIdeal parameters instead of using
  MediaStreamVideoSource constants. This gives more flexibility, such
  as allowing the use source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Fixes minor lint issues in existing source files.

BUG=657733
==========

to

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Various cleanups and renames to easy readability.
* Fixes minor lint issues in existing source files.

BUG=657733
==========

[Guido Urdaneta, 2017-03-27 10:08:26]

Description was changed from

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Various cleanups and renames to easy readability.
* Fixes minor lint issues in existing source files.

BUG=657733
==========

to

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Various minor cleanups and renames to better express intent.
* Fixes minor lint issues in existing source files.

BUG=657733
==========

[Guido Urdaneta, 2017-03-27 10:21:50]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 10:22:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Guido Urdaneta, 2017-03-27 11:52:41]

Patchset #1 (id:40001) has been deleted

[Guido Urdaneta, 2017-03-27 11:52:52]

Patchset #1 (id:60001) has been deleted

[Guido Urdaneta, 2017-03-27 11:53:17]

Patchset #1 (id:80001) has been deleted

[Guido Urdaneta, 2017-03-27 11:53:33]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 11:53:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Guido Urdaneta, 2017-03-27 11:56:09]

guidou@chromium.org changed reviewers:
+ hbos@chromium.org

[Guido Urdaneta, 2017-03-27 11:56:26]

Hi, PTAL

[commit-bot: I haz the power, 2017-03-27 12:46:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 12:46:11]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-03-28 08:48:26]

Please wait until the next patchset for the review.

[Guido Urdaneta, 2017-03-28 09:42:18]

Description was changed from

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so that it uses
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const).
* Various minor cleanups and renames to better express intent.
* Fixes minor lint issues in existing source files.

BUG=657733
==========

to

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Use a single class VideoCaptureSettings for the results of both 
  content and device capture. They differed only in the facing_mode
  field.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so in terms of
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Replaces rtc::Optional with base::Optional.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const), or to better 
  express intent.
* Fixes minor lint issues in existing source files.

BUG=657733
==========

[Guido Urdaneta, 2017-03-28 09:46:28]

Patchset #2 (id:120001) has been deleted

[Guido Urdaneta, 2017-03-28 09:51:39]

Ready for review now. Please ignore the older patchset.

[Guido Urdaneta, 2017-03-28 11:41:37]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 11:41:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 12:39:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 12:39:32]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2017-03-28 12:52:53]

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util.cc (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.cc:116:
min_frame_rate_(min_frame_rate) {}
Should we DCHECK that the video capture params are within the track adapter
settings min/max ranges?
No, right, cropping means we can have a lower track max than capture params...
Do we support up-scaling as well or should we DCHECK that track min <= capture
params max?

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util.h (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:109:
VideoTrackAdapterSettings track_adapter_settings_;
VideoCaptureParams has w,h,fps.
VideoTrackAdapterSettings has max w,h,fps and min/max ratios.
Then there are ResolutionSets and stuff from other CLs.

I find it a little confusing to have several different classes for resolutions
etc.

Can you add a comment to say what this class (VideoCaptureSettings) is used for,
what its usage is compared to its component classes / why it has both a "value"
(capture_params_) and a "range" (track_adapter_settings_).

I also wonder if it makes sense to rename any of these classes or substitute
VideoTrackAdapterSettings with something more generic and descriptive like
"ResolutionRange" or something similar.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:204: //   there will be
no frame-rate adjustment.
Should Optional be used instead of 0.0?

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:214: // |frame_rate_set|
are empty.
Undefined behavior? Why not DCHECK that they aren't empty?

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:215:
VideoTrackAdapterSettings CONTENT_EXPORT SelectVideoTrackAdapterSettings(
Why do we express the desired result settings as a maximum and not an actual
value?

Can multiple VideoTrackAdapterSettings be applied on the same track to give it
the s.max_blah = min(s1.max_blah, s2.max_blah)?

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util_unittest.cc (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_unittest.cc:138: int
kIdealHeight = 400;
If you use this naming convention make it const int (here and elsewhere).

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_unittest.cc:245: // Ideal
height < min.
You could static_assert these type of things.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_unittest.cc:480: // as the
source.
In the larger context of sources and settings: ...and the source is guaranteed
not to change its frame rate if not constraining the frame rate with a non-zero
value?

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util_video_device.cc
(right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_video_device.cc:180: //    
 constrained_format.native_frame_rate());
Remove the commented-out lines.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_video_device.cc:795:
DCHECK(!constrained_format.constrained_frame_rate().IsEmpty());
(IsEmpty not possible due to isfinite checks, right?)

[Guido Urdaneta, 2017-03-28 18:35:43]

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util.cc (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.cc:116:
min_frame_rate_(min_frame_rate) {}
On 2017/03/28 12:52:53, hbos_chromium wrote:
> Should we DCHECK that the video capture params are within the track adapter
> settings min/max ranges?
> No, right, cropping means we can have a lower track max than capture params...
> Do we support up-scaling as well or should we DCHECK that track min <= capture
> params max?

We don't support upscaling. The source must provide frames that satisfy the
constraint limits. The adapter could in principle upscale them, but it currently
doesn't do that.
Added a few DCHECKs that are relevant, though.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util.h (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:109:
VideoTrackAdapterSettings track_adapter_settings_;
On 2017/03/28 12:52:53, hbos_chromium wrote:
> VideoCaptureParams has w,h,fps.
> VideoTrackAdapterSettings has max w,h,fps and min/max ratios.
> Then there are ResolutionSets and stuff from other CLs.
> 
> I find it a little confusing to have several different classes for resolutions
> etc.
> 

It can be quite confusing. Added a detailed comment that hopefully clarifies
things.
The general approach is that SelectSettings is a function that provides a
mapping from constraints to input values for other Chromium components.
Those input values are grouped in VideoCaptureSettings. VideoCaptureSettings is
not intended to make much sense on its own, but in the context of having values
that are inputs to other existing classes that are not (or should not be) aware
of constraints. For example, VideoTrackAdapterSettings is for VideoTrackAdapter,
which all tracks need,
VideoCaptureParams is an existing class to configure the video capturer, the
3-state noise_reduction is an input for MediaStreamVideoWebRtcSink, and so on.

Other classes related to SelectSettings such as ResolutionSet and similar are
just internal implementation details of SelectSettings.

> Can you add a comment to say what this class (VideoCaptureSettings) is used
for,
> what its usage is compared to its component classes / why it has both a
"value"
> (capture_params_) and a "range" (track_adapter_settings_).

Done. As explained above, VideoCaptureSettings is intended to make it easier to
configure other existing Chromium components.

> 
> I also wonder if it makes sense to rename any of these classes or substitute
> VideoTrackAdapterSettings with something more generic and descriptive like
> "ResolutionRange" or something similar.

I don't think it does.
Those values are intended as input to other classes, and their (sometimes
existing) naming reflects that.

BTW, removed |facing_mode| from VideoCaptureSettings. It was originally going to
be used for getSettings, but that was implemented in a different way already.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:204: //   there will be
no frame-rate adjustment.
On 2017/03/28 12:52:53, hbos_chromium wrote:
> Should Optional be used instead of 0.0?

No. It is the way it is because that is how VideoTrackAdapter interprets its
input.
Otherwise we would need a conversion layer.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:214: // |frame_rate_set|
are empty.
On 2017/03/28 12:52:53, hbos_chromium wrote:
> Undefined behavior? Why not DCHECK that they aren't empty?

They are DCHECKed, and this is a way to document it.
DCHECK is defined in debug mode (crash), but since it doesn't run in release
mode, all bets are off as to what this function does in that case.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:215:
VideoTrackAdapterSettings CONTENT_EXPORT SelectVideoTrackAdapterSettings(
Because that's the way VideoTrackAdapter works. 
I general, for this work, I prefer to minimize changes to VideoTrackAdapter and
other classes that have nothing to do with constraints.
VTA interpretation of its max resolution has nothing to do with the max value in
constraints.

VTA, once configured, works by transforming input frames into frames that are no
larger than its (max_width, max_height). If the input frames are smaller than
its max, VTA leaves them alone.

We configure the source to not provide smaller input frames, but VTA doesn't
know that, and IMO that's OK.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util_unittest.cc (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_unittest.cc:138: int
kIdealHeight = 400;
On 2017/03/28 12:52:53, hbos_chromium wrote:
> If you use this naming convention make it const int (here and elsewhere).

Done. That's what I originally intended :)

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_unittest.cc:245: // Ideal
height < min.
On 2017/03/28 12:52:53, hbos_chromium wrote:
> You could static_assert these type of things.

Done.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_unittest.cc:480: // as the
source.
On 2017/03/28 12:52:53, hbos_chromium wrote:
> In the larger context of sources and settings: ...and the source is guaranteed
> not to change its frame rate if not constraining the frame rate with a
non-zero
> value?

0.0 is a special value that VideoTrackAdapter interprets as no frame-rate
adjustment.
This field is not intended to exactly express what constraints say. In
particular, it rarely coincides with the max frame rate as specified in the
constraints.
It is just the input to VideoTrackAdapter that makes it work in a way that is
closest to the spec.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util_video_device.cc
(right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_video_device.cc:180: //    
 constrained_format.native_frame_rate());
On 2017/03/28 12:52:53, hbos_chromium wrote:
> Remove the commented-out lines.

Done.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_video_device.cc:795:
DCHECK(!constrained_format.constrained_frame_rate().IsEmpty());
On 2017/03/28 12:52:53, hbos_chromium wrote:
> (IsEmpty not possible due to isfinite checks, right?)

That's right.

[hbos_chromium, 2017-03-29 14:54:53]

lgtm

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util.h (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:109:
VideoTrackAdapterSettings track_adapter_settings_;
On 2017/03/28 18:35:43, Guido Urdaneta wrote:
> On 2017/03/28 12:52:53, hbos_chromium wrote:
> > VideoCaptureParams has w,h,fps.
> > VideoTrackAdapterSettings has max w,h,fps and min/max ratios.
> > Then there are ResolutionSets and stuff from other CLs.
> > 
> > I find it a little confusing to have several different classes for
resolutions
> > etc.
> > 
> 
> It can be quite confusing. Added a detailed comment that hopefully clarifies
> things.
> The general approach is that SelectSettings is a function that provides a
> mapping from constraints to input values for other Chromium components.
> Those input values are grouped in VideoCaptureSettings. VideoCaptureSettings
is
> not intended to make much sense on its own, but in the context of having
values
> that are inputs to other existing classes that are not (or should not be)
aware
> of constraints. For example, VideoTrackAdapterSettings is for
VideoTrackAdapter,
> which all tracks need,
> VideoCaptureParams is an existing class to configure the video capturer, the
> 3-state noise_reduction is an input for MediaStreamVideoWebRtcSink, and so on.
> 
> Other classes related to SelectSettings such as ResolutionSet and similar are
> just internal implementation details of SelectSettings.
> 
> > Can you add a comment to say what this class (VideoCaptureSettings) is used
> for,
> > what its usage is compared to its component classes / why it has both a
> "value"
> > (capture_params_) and a "range" (track_adapter_settings_).
> 
> Done. As explained above, VideoCaptureSettings is intended to make it easier
to
> configure other existing Chromium components.
> 
> > 
> > I also wonder if it makes sense to rename any of these classes or substitute
> > VideoTrackAdapterSettings with something more generic and descriptive like
> > "ResolutionRange" or something similar.
> 
> I don't think it does.
> Those values are intended as input to other classes, and their (sometimes
> existing) naming reflects that.
> 
> BTW, removed |facing_mode| from VideoCaptureSettings. It was originally going
to
> be used for getSettings, but that was implemented in a different way already.

Acknowledged.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:204: //   there will be
no frame-rate adjustment.
On 2017/03/28 18:35:43, Guido Urdaneta wrote:
> On 2017/03/28 12:52:53, hbos_chromium wrote:
> > Should Optional be used instead of 0.0?
> 
> No. It is the way it is because that is how VideoTrackAdapter interprets its
> input.
> Otherwise we would need a conversion layer.

Acknowledged.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:214: // |frame_rate_set|
are empty.
On 2017/03/28 18:35:43, Guido Urdaneta wrote:
> On 2017/03/28 12:52:53, hbos_chromium wrote:
> > Undefined behavior? Why not DCHECK that they aren't empty?
> 
> They are DCHECKed, and this is a way to document it.
> DCHECK is defined in debug mode (crash), but since it doesn't run in release
> mode, all bets are off as to what this function does in that case.

Acknowledged.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util.h:215:
VideoTrackAdapterSettings CONTENT_EXPORT SelectVideoTrackAdapterSettings(
On 2017/03/28 18:35:43, Guido Urdaneta wrote:
> Because that's the way VideoTrackAdapter works. 
> I general, for this work, I prefer to minimize changes to VideoTrackAdapter
and
> other classes that have nothing to do with constraints.
> VTA interpretation of its max resolution has nothing to do with the max value
in
> constraints.
> 
> VTA, once configured, works by transforming input frames into frames that are
no
> larger than its (max_width, max_height). If the input frames are smaller than
> its max, VTA leaves them alone.
> 
> We configure the source to not provide smaller input frames, but VTA doesn't
> know that, and IMO that's OK.

Acknowledged.

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
File content/renderer/media/media_stream_constraints_util_unittest.cc (right):

https://codereview.chromium.org/2777703002/diff/140001/content/renderer/media...
content/renderer/media/media_stream_constraints_util_unittest.cc:480: // as the
source.
On 2017/03/28 18:35:43, Guido Urdaneta wrote:
> On 2017/03/28 12:52:53, hbos_chromium wrote:
> > In the larger context of sources and settings: ...and the source is
guaranteed
> > not to change its frame rate if not constraining the frame rate with a
> non-zero
> > value?
> 
> 0.0 is a special value that VideoTrackAdapter interprets as no frame-rate
> adjustment.
> This field is not intended to exactly express what constraints say. In
> particular, it rarely coincides with the max frame rate as specified in the
> constraints.
> It is just the input to VideoTrackAdapter that makes it work in a way that is
> closest to the spec.

Acknowledged.

[Guido Urdaneta, 2017-03-29 15:00:03]

The CQ bit was checked by guidou@chromium.org

[commit-bot: I haz the power, 2017-03-29 15:00:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 16:00:36]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490799603223440,
"parent_rev": "7be36d31cf92f725808f0260e565ed093541505f", "commit_rev":
"eff69cf4c3028e0bb4e15c78b137c6bb26966974"}

[commit-bot: I haz the power, 2017-03-29 16:01:28]

Description was changed from

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Use a single class VideoCaptureSettings for the results of both 
  content and device capture. They differed only in the facing_mode
  field.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so in terms of
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Replaces rtc::Optional with base::Optional.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const), or to better 
  express intent.
* Fixes minor lint issues in existing source files.

BUG=657733
==========

to

==========
Introduce SelectSettings algorithm for MediaStream video tracks.

This CL also:
* Renames functions and types related to selecting settings, dropping
  "Source" from the name, since now they also deal with track settings.
* Introduces a VideoTrackAdapterSettings struct to pass arguments to
  VideoTrackAdapter and to return track settings in a more convenient
  manner.
* Use a single class VideoCaptureSettings for the results of both 
  content and device capture. They differed only in the facing_mode
  field.
* The default width and height in 
  ResolutionSet::SelectClosestPointToIdeal are passed as parameters
  instead of using MediaStreamVideoSource constants. This gives more
  flexibility, such as allowing the use of source settings as defaults.
* Makes default width and height constants for content capture public.
  This is a first step towards better defaults for screen capture. See
  http://crbug.com/257097.
* Reimplements the internal ConstrainedFormat class in
  media_stream_constraints_util_video_device.cc so in terms of
  ResolutionSet and NumericRangeSet. This removes code that is largely
  duplicated and facilitates integration with the code that computes
  track settings.
* Replaces rtc::Optional with base::Optional.
* Makes minor changes to be more consistent with other parts of the
  codebase (e.g., some constexpr changed to const), or to better 
  express intent.
* Fixes minor lint issues in existing source files.

BUG=657733

Review-Url: https://codereview.chromium.org/2777703002
Cr-Commit-Position: refs/heads/master@{#460403}
Committed:
https://chromium.googlesource.com/chromium/src/+/eff69cf4c3028e0bb4e15c78b137...
==========

[commit-bot: I haz the power, 2017-03-29 16:01:29]

Committed patchset #4 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/eff69cf4c3028e0bb4e15c78b137...