Update constraints processing for video content capture.

Update constraints processing for video content capture.

The computation for refresh interval for content capture
produces bad results if it uses the implicit min/max range for
frame rate produced by the new spec-compliant algorithm.

This patch updates the spec-compliant constraints algorithm to
provide indication if the frame-rate range was explicitly set by
constraints.
MediaStreamVideoWebRtcSink is updated to use the frame-rate min/max
values to compute the refresh intervals only if they were explicitly
set by constrains, as was the case before the new constrains algorithm
was introduced.

BUG=729746
Review-Url: https://codereview.chromium.org/2922013002
Cr-Commit-Position: refs/heads/master@{#478101}
Committed: https://chromium.googlesource.com/chromium/src/+/303a5bbadb90b69780a86b10d5284ffc4c1dc165

[Guido Urdaneta, 2017-06-03 02:32:31]

The CQ bit was checked by guidou@chromium.org

[commit-bot: I haz the power, 2017-06-03 02:32:48]

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-06-03 02:32:48]

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

[commit-bot: I haz the power, 2017-06-03 02:32:49]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Guido Urdaneta, 2017-06-03 02:36:40]

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

[commit-bot: I haz the power, 2017-06-03 02:36:54]

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-06-03 03:37:50]

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

[commit-bot: I haz the power, 2017-06-03 03:37:51]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-06-03 03:52:45]

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

[Guido Urdaneta, 2017-06-03 03:52:46]

Hi, PTAL

[miu, 2017-06-05 20:14:28]

lgtm % issue about "zero min framerate":

We were discussing on the crbug what to do about a min framerate of zero: IMHO,
for the API, I think setting the min to zero is reasonable, since there are
non-WebRTC consumers that handle this. So, IMHO, all your checks should be ">=
0.0" instead of just "> 0.0".

However, we also know that WebRTC requires a certain lower-bound minimum itself
(e.g., for correct BW estimation). So, it might make sense to enforce a
lower-bound of 1 FPS in media_stream_video_webrtc_sink.cc, even if the user has
set a min framerate less than 1 FPS.

[Guido Urdaneta, 2017-06-05 21:19:34]

On 2017/06/05 20:14:28, miu wrote:
> lgtm % issue about "zero min framerate":
> 
> We were discussing on the crbug what to do about a min framerate of zero:
IMHO,
> for the API, I think setting the min to zero is reasonable, since there are
> non-WebRTC consumers that handle this. So, IMHO, all your checks should be ">=
> 0.0" instead of just "> 0.0".

Will do. That part of the code is disabled by default anyway since it
corresponds to the old constraints algorithm.
The new algorithm already accepts values >= 0, although this could change based
on your comment below.

> 
> However, we also know that WebRTC requires a certain lower-bound minimum
itself
> (e.g., for correct BW estimation). So, it might make sense to enforce a
> lower-bound of 1 FPS in media_stream_video_webrtc_sink.cc, even if the user
has
> set a min framerate less than 1 FPS.

For device capture there is already a minimum frame rate of 1 FPS.
I guess we can enforce it for all content capture as well (as we were enforcing
the problematic 1/60).
Are you OK with this?
This means that trying to set a minimum FrameRate less than 1.0 will result in a
getUserMedia error.
If not, I'll just leave it as 0.0.

[Guido Urdaneta, 2017-06-05 21:33:58]

Description was changed from

==========
Update constraints processing for video content capture.

The computation for refresh interval for content capture
produces bad results if it uses the implicit min/max range for
frame rate produced by the new spec-compliant algorithm.

This patch updates the spec-compliant constraints algorithm to
provide indication if the frame-rate range was explicitly set by
constraints.
MediaStreamVideoWebRtcSink is updated to use the frame-rate min/max
values to compute the refresh intervals only if they were explicitly
set by constrains, as was the case before the new constrains algorithm
was introduced.

BUG=725696
==========

to

==========
Update constraints processing for video content capture.

The computation for refresh interval for content capture
produces bad results if it uses the implicit min/max range for
frame rate produced by the new spec-compliant algorithm.

This patch updates the spec-compliant constraints algorithm to
provide indication if the frame-rate range was explicitly set by
constraints.
MediaStreamVideoWebRtcSink is updated to use the frame-rate min/max
values to compute the refresh intervals only if they were explicitly
set by constrains, as was the case before the new constrains algorithm
was introduced.

BUG=729746
==========

[miu, 2017-06-06 22:00:08]

On 2017/06/05 21:19:34, Guido Urdaneta wrote:
> For device capture there is already a minimum frame rate of 1 FPS.
> I guess we can enforce it for all content capture as well (as we were
enforcing
> the problematic 1/60).
> Are you OK with this?
> This means that trying to set a minimum FrameRate less than 1.0 will result in
a
> getUserMedia error.

I may be missing something, but why would we error out? Let's say, as a web
developer, I use:

  { frameRate: { min: 0, max: 60, ideal:60 }, ... }

...and the resulting MediaStream is then used with WebRTC (e.g.,
PeerConnection). Once WebRTC becomes the consumer of the MediaStream's video we
now know we really have a 1 FPS requirement (and this will be driven by the
timer in media_stream_video_webrtc_sink.cc, if needed). However, it should be
perfectly fine for video capture to operate at a 1 FPS minimum since it is
within the user's required 0-60 range, right?

[Guido Urdaneta, 2017-06-07 10:32:24]

On 2017/06/06 22:00:08, miu wrote:
> On 2017/06/05 21:19:34, Guido Urdaneta wrote:
> > For device capture there is already a minimum frame rate of 1 FPS.
> > I guess we can enforce it for all content capture as well (as we were
> enforcing
> > the problematic 1/60).
> > Are you OK with this?
> > This means that trying to set a minimum FrameRate less than 1.0 will result
in
> a
> > getUserMedia error.
> 
> I may be missing something, but why would we error out? Let's say, as a web
> developer, I use:
> 
>   { frameRate: { min: 0, max: 60, ideal:60 }, ... }
> 
> ...and the resulting MediaStream is then used with WebRTC (e.g.,
> PeerConnection). Once WebRTC becomes the consumer of the MediaStream's video
we
> now know we really have a 1 FPS requirement (and this will be driven by the
> timer in media_stream_video_webrtc_sink.cc, if needed). However, it should be
> perfectly fine for video capture to operate at a 1 FPS minimum since it is
> within the user's required 0-60 range, right?

If the user specifies [0,60] it's OK to provide [1,60] without violating
anything.
The question is what should we do if the application requests things like [0,
0.05] or [0,0].
Even if the application requests [0-1] what would be the point of actually
providing [1-1]?
Wouldn't it be simpler to enforce a minimum of 1?
That would not affect the case when we want to give different treatment to no
explicit minimum.

[Guido Urdaneta, 2017-06-08 15:22:49]

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

[commit-bot: I haz the power, 2017-06-08 15:23:08]

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-06-08 15:43:07]

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

[commit-bot: I haz the power, 2017-06-08 15:43:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Guido Urdaneta, 2017-06-08 16:48:15]

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

[commit-bot: I haz the power, 2017-06-08 16:48:40]

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-06-08 18:02:08]

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

[commit-bot: I haz the power, 2017-06-08 18:02:09]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-06-08 22:05:52]

I'm going to land the patch, since it is orthogonal to the discussion on what
the right minimum frame rate should be.
I filed crbug.com/731299 to continue the discussion about minimum frame rate.

[Guido Urdaneta, 2017-06-08 22:05:58]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2017-06-08 22:05:58]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2922013002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-08 22:06:26]

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-06-08 22:14:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496959558534440,
"parent_rev": "bd89f28f0a3ff7885bb28f74fbc77338d8653193", "commit_rev":
"303a5bbadb90b69780a86b10d5284ffc4c1dc165"}

[commit-bot: I haz the power, 2017-06-08 22:14:17]

Description was changed from

==========
Update constraints processing for video content capture.

The computation for refresh interval for content capture
produces bad results if it uses the implicit min/max range for
frame rate produced by the new spec-compliant algorithm.

This patch updates the spec-compliant constraints algorithm to
provide indication if the frame-rate range was explicitly set by
constraints.
MediaStreamVideoWebRtcSink is updated to use the frame-rate min/max
values to compute the refresh intervals only if they were explicitly
set by constrains, as was the case before the new constrains algorithm
was introduced.

BUG=729746
==========

to

==========
Update constraints processing for video content capture.

The computation for refresh interval for content capture
produces bad results if it uses the implicit min/max range for
frame rate produced by the new spec-compliant algorithm.

This patch updates the spec-compliant constraints algorithm to
provide indication if the frame-rate range was explicitly set by
constraints.
MediaStreamVideoWebRtcSink is updated to use the frame-rate min/max
values to compute the refresh intervals only if they were explicitly
set by constrains, as was the case before the new constrains algorithm
was introduced.

BUG=729746

Review-Url: https://codereview.chromium.org/2922013002
Cr-Commit-Position: refs/heads/master@{#478101}
Committed:
https://chromium.googlesource.com/chromium/src/+/303a5bbadb90b69780a86b10d528...
==========

[commit-bot: I haz the power, 2017-06-08 22:14:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/303a5bbadb90b69780a86b10d528...