Update constraints algorithm for video content capture.

Update constraints algorithm for video content capture.

1. Update default from 640x480 to 2880x1800. The goal is to use the native
screen resolution as default, but this is an improvement over the current
situation.
2. If a maximum resolution constraint is given, use that as default instead
of the hardcoded default. This makes the algorithm more compatible with
existing applications without breaking compatibility with the spec.

BUG=257097, 709345
Review-Url: https://codereview.chromium.org/2806703002
Cr-Commit-Position: refs/heads/master@{#462876}
Committed: https://chromium.googlesource.com/chromium/src/+/258527ddc93825a5feac1bd3de5f5efb8d975856

[Guido Urdaneta, 2017-04-07 12:21:19]

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

[commit-bot: I haz the power, 2017-04-07 12:21:27]

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-04-07 12:22:07]

Description was changed from

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

1. Update default from 640x480 to 2880x1800. The goal is to use the native
screen resolution as default, but this is an improvement over the current
situation.
2. If a maximum resolution constraint is given, use that as default instead
of the hardcoded default. This makes the algorithm more compatible with
existing applications without breaking compatibility with the spec.

BUG=257097,709345
==========

to

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

1. Update default from 640x480 to 2880x1800. The goal is to use the native
screen resolution as default, but this is an improvement over the current
situation.
2. If a maximum resolution constraint is given, use that as default instead
of the hardcoded default. This makes the algorithm more compatible with
existing applications without breaking compatibility with the spec.

BUG=257097,709345
==========

[Guido Urdaneta, 2017-04-07 12:22:07]

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

[Guido Urdaneta, 2017-04-07 12:22:50]

Hi, PTAL

[hbos_chromium, 2017-04-07 13:16:30]

lgtm

https://codereview.chromium.org/2806703002/diff/20001/content/renderer/media/...
File
content/renderer/media/media_stream_constraints_util_video_content_unittest.cc
(right):

https://codereview.chromium.org/2806703002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_constraints_util_video_content_unittest.cc:303:
// kSmallHeight is less that the default, so expect kSmallHeight.
nit: comment variable name and variable name don't match.

https://codereview.chromium.org/2806703002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_constraints_util_video_content_unittest.cc:318:
const int kMaxHeight = 400;
I think you shouldn't remove these test cases to make sure that max and default
behaves the way we expect it to behave, as opposed to pruning away the test due
to the knowledge of the algorithm behaving this way.

Add a comment saying we expect the max because the max should overwrite the
default.

Do the same for other tests you've removed even though the expected result will
be default overridden by max.

[Guido Urdaneta, 2017-04-07 15:05:19]

https://codereview.chromium.org/2806703002/diff/20001/content/renderer/media/...
File
content/renderer/media/media_stream_constraints_util_video_content_unittest.cc
(right):

https://codereview.chromium.org/2806703002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_constraints_util_video_content_unittest.cc:303:
// kSmallHeight is less that the default, so expect kSmallHeight.
On 2017/04/07 13:16:29, hbos_chromium wrote:
> nit: comment variable name and variable name don't match.

Done.

https://codereview.chromium.org/2806703002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_constraints_util_video_content_unittest.cc:318:
const int kMaxHeight = 400;
On 2017/04/07 13:16:29, hbos_chromium wrote:
> I think you shouldn't remove these test cases to make sure that max and
default
> behaves the way we expect it to behave, as opposed to pruning away the test
due
> to the knowledge of the algorithm behaving this way.
> 
> Add a comment saying we expect the max because the max should overwrite the
> default.
> 
> Do the same for other tests you've removed even though the expected result
will
> be default overridden by max.

Done.

[Guido Urdaneta, 2017-04-07 15:05:27]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2017-04-07 15:05:28]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org
Link to the patchset: https://codereview.chromium.org/2806703002/#ps40001
(title: "re-add tests")

[commit-bot: I haz the power, 2017-04-07 15:05:44]

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-04-07 16:00:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491577527710200,
"parent_rev": "311b4ebadca3e47753627f385ee4ce3482331404", "commit_rev":
"258527ddc93825a5feac1bd3de5f5efb8d975856"}

[commit-bot: I haz the power, 2017-04-07 16:01:38]

Description was changed from

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

1. Update default from 640x480 to 2880x1800. The goal is to use the native
screen resolution as default, but this is an improvement over the current
situation.
2. If a maximum resolution constraint is given, use that as default instead
of the hardcoded default. This makes the algorithm more compatible with
existing applications without breaking compatibility with the spec.

BUG=257097,709345
==========

to

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

1. Update default from 640x480 to 2880x1800. The goal is to use the native
screen resolution as default, but this is an improvement over the current
situation.
2. If a maximum resolution constraint is given, use that as default instead
of the hardcoded default. This makes the algorithm more compatible with
existing applications without breaking compatibility with the spec.

BUG=257097,709345

Review-Url: https://codereview.chromium.org/2806703002
Cr-Commit-Position: refs/heads/master@{#462876}
Committed:
https://chromium.googlesource.com/chromium/src/+/258527ddc93825a5feac1bd3de5f...
==========

[commit-bot: I haz the power, 2017-04-07 16:01:39]

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