New resolution change policies for desktop and tab capture.

New resolution change policies for desktop and tab capture.

This change replaces the "fixed" versus "variable" resolution change
policies with a 3-tier set: fixed, variable with fixed aspect ratio, and
variable at any aspect ratio.

This allows users of the desktop and tab capture APIs to specify minimum
and maximum resolutions, and from those, a resolution change policy is
determined.  The policy is used by the desktop and tab capture
implementation to activate or disable sender-side letterboxing of
screen-capture content.

BUG=473336
NOPRESUBMIT=true
NOTRY=true

Committed: https://crrev.com/f79202747a089a00f653e5827409c779395bda70
Cr-Commit-Position: refs/heads/master@{#329056}

[miu, 2015-05-07 00:52:33]

Patchset #1 (id:1) has been deleted

[miu, 2015-05-07 00:54:21]

miu@chromium.org changed reviewers:
+ dalecurtis@chromium.org, mcasas@chromium.org

[miu, 2015-05-07 00:54:22]

dalecurtis: OWNERS approval please.

mcasas: Please review.

Thanks in advance for your time!  :)

https://codereview.chromium.org/1124263004/diff/20001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1124263004/diff/20001/media/base/video_captur...
media/base/video_capture_types.h:76: bool operator==(const VideoCaptureFormat&
other) const;
Note: I added an operator==() method here and below to support the mock matchers
in the unit tests.

[DaleCurtis, 2015-05-07 01:13:39]

https://codereview.chromium.org/1124263004/diff/20001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1124263004/diff/20001/media/base/video_captur...
media/base/video_capture_types.h:76: bool operator==(const VideoCaptureFormat&
other) const;
On 2015/05/07 00:54:22, miu wrote:
> Note: I added an operator==() method here and below to support the mock
matchers
> in the unit tests.

If it's just for tests you can use MATCHER_P()

https://code.google.com/p/chromium/codesearch#chromium/src/media/renderers/vi...

Up to you, though I think it should be inlined if you keep operator==.

[miu, 2015-05-07 02:49:19]

https://codereview.chromium.org/1124263004/diff/20001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1124263004/diff/20001/media/base/video_captur...
media/base/video_capture_types.h:76: bool operator==(const VideoCaptureFormat&
other) const;
On 2015/05/07 01:13:38, DaleCurtis wrote:
> On 2015/05/07 00:54:22, miu wrote:
> > Note: I added an operator==() method here and below to support the mock
> matchers
> > in the unit tests.
> 
> If it's just for tests you can use MATCHER_P()
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/renderers/vi...
> 
> Up to you, though I think it should be inlined if you keep operator==.

Done.  Good point here.  I went with the inline operator==() to keep its
definition close to where the members are declared.  Otherwise, using the
MATCHER_P() solution, I fear the test might not be updated in-tandem with
video_capture_types.* changes in the future.

[mcasas, 2015-05-07 22:10:00]

Few comments and a question about usage of
RESOLUTION_POLICY_FIXED_ASPECT_RATIO.

https://codereview.chromium.org/1124263004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1124263004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:182: << " (" <<
(params.resolution_change_policy ==
Nit: this parenthesis isn't matched.

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:42: x <= 16384;  
// Upper-bound: Common maximum frame size in encoders, etc.
Shouldn't we use GetArea(), for minimum size, and [1] for maximum?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/limits....

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:48: (frame_rate <=
120);               // Upper-bound: 120 frames per second.
[1]?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/limits....

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:146:
DimensionValueIsValid(height) &&
Ni: shouldn't l.146-147 be justified @8th column (i.e. four blanks right of the
} )?

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:388: }
Justiy two columns leftwards.

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:110: // An implementation
must start to capture frames using the requested
s/start to capture/start capturing/? (Disclaimer: English is not my native
language as you know  :) )

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:141: // fulfilled.
... and |fulfilled_contraints| is the actual result?

https://codereview.chromium.org/1124263004/diff/40001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1124263004/diff/40001/media/base/video_captur...
media/base/video_capture_types.h:49: // It is implementation-specific for the
capture device to scale, letter-box
nit: I'd refactor this comment out, somehow saying that it applies to
both _FIXED_ polcies, otherwise is repeated verbatim.

https://codereview.chromium.org/1124263004/diff/40001/media/base/video_captur...
media/base/video_capture_types.h:56: RESOLUTION_POLICY_ANY_WITHIN_LIMIT,
IIUC, only Desktop and Tab capture really care about these
division, i.e. the split of ...DYNAMIC_WITHIN_LIMIT into 
...FIXED_ASPECT_RATIO and ...ANY_WITHIN_LIMIT, however, 
I only see ThreadSafeCaptureOracle::UpdateCaptureSize()
and DesktopCaptureDevice::Core::RefreshCaptureFormat()
reacting to RESOLUTION_POLICY_ANY_WITHIN_LIMIT
and not to RESOLUTION_POLICY_FIXED_ASPECT_RATIO,
is this correct?

[miu, 2015-05-08 06:43:34]

Thanks.  Addressed your comments:

https://codereview.chromium.org/1124263004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1124263004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:182: << " (" <<
(params.resolution_change_policy ==
On 2015/05/07 22:10:00, mcasas wrote:
> Nit: this parenthesis isn't matched.

Assuming you're talking about the parentheses in the string literals since this
code compiles.  It appears matched to me (see line 188).  Am I missing
something?

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:42: x <= 16384;  
// Upper-bound: Common maximum frame size in encoders, etc.
On 2015/05/07 22:10:00, mcasas wrote:
> Shouldn't we use GetArea(), for minimum size, and [1] for maximum?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/limits....

Done.  I thought about this again, and I think anything greater than zero should
be acceptable (since the coded size of an I420 frame would round up).  I used
media/base/limits for both the max constraint.  Good suggestion.

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:48: (frame_rate <=
120);               // Upper-bound: 120 frames per second.
On 2015/05/07 22:10:00, mcasas wrote:
> [1]?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/limits....

Done.  media/base/limits.h didn't have a minimum frame rate.  For practical
purposes, one frame per minute should be acceptable for screen casting.  (Yes,
we do want to allow less than 1 FPS.  There are reasonable use cases for this
that I can discuss offline, if you're interested.)

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:146:
DimensionValueIsValid(height) &&
On 2015/05/07 22:10:00, mcasas wrote:
> Ni: shouldn't l.146-147 be justified @8th column (i.e. four blanks right of
the
> } )?

I don't quite understand, and the auto-indent feature in my editor didn't reveal
any issues.  Can you show me an example of how you think this should be
indented?

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:388: }
On 2015/05/07 22:10:00, mcasas wrote:
> Justiy two columns leftwards.

Done.  Good catch.

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:141: // fulfilled.
On 2015/05/07 22:10:00, mcasas wrote:
> ... and |fulfilled_contraints| is the actual result?

Fixed comment, and added details about the return value.

https://codereview.chromium.org/1124263004/diff/40001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1124263004/diff/40001/media/base/video_captur...
media/base/video_capture_types.h:49: // It is implementation-specific for the
capture device to scale, letter-box
On 2015/05/07 22:10:00, mcasas wrote:
> nit: I'd refactor this comment out, somehow saying that it applies to
> both _FIXED_ polcies, otherwise is repeated verbatim.

Done.

https://codereview.chromium.org/1124263004/diff/40001/media/base/video_captur...
media/base/video_capture_types.h:56: RESOLUTION_POLICY_ANY_WITHIN_LIMIT,
On 2015/05/07 22:10:00, mcasas wrote:
> IIUC, only Desktop and Tab capture really care about these
> division, i.e. the split of ...DYNAMIC_WITHIN_LIMIT into 
> ...FIXED_ASPECT_RATIO and ...ANY_WITHIN_LIMIT, however, 
> I only see ThreadSafeCaptureOracle::UpdateCaptureSize()
> and DesktopCaptureDevice::Core::RefreshCaptureFormat()
> reacting to RESOLUTION_POLICY_ANY_WITHIN_LIMIT
> and not to RESOLUTION_POLICY_FIXED_ASPECT_RATIO,
> is this correct?

Yes, that is correct.  The current implementations will still do the right
thing.  For example, if FIXED_ASPECT_RATIO is specified, they will actually
provide the FIXED_RESOLUTION behavior.  That is acceptable since
FIXED_RESOLUTION frames are all the same aspect ratio.

The whole reason for this change is to support my next CL (probably up for
review tomorrow or Monday).  That's where all three resolution change policies
will be implemented for desktop and tab capture.  See linked bug for details of
what this is all about.

[miu, 2015-05-08 06:44:19]

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

[miu, 2015-05-08 06:44:19]

teravest: Need OWNERS stamp for content/renderer/pepper/*

[miu, 2015-05-08 06:45:25]

miu@chromium.org changed reviewers:
+ bbudge@chromium.org
- teravest@chromium.org

[miu, 2015-05-08 06:45:26]

-teravest

+bbudge: Need OWNERS stamp for content/renderer/pepper/*

[bbudge, 2015-05-08 17:12:48]

https://codereview.chromium.org/1124263004/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/1124263004/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:436: const
blink::WebMediaConstraints& constraints,
Should we be doing something with this new parameter?

[miu, 2015-05-08 18:07:18]

https://codereview.chromium.org/1124263004/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/1124263004/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:436: const
blink::WebMediaConstraints& constraints,
On 2015/05/08 17:12:48, bbudge wrote:
> Should we be doing something with this new parameter?

Nope.  It's just extra context used by other impls of this interface.

[mcasas, 2015-05-08 18:38:42]

Capture classes LGTM provided we use
RESOLUTION_POLICY_FIXED_ASPECT_RATIO
in imminent CLs.

https://codereview.chromium.org/1124263004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/1124263004/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:182: << " (" <<
(params.resolution_change_policy ==
On 2015/05/08 06:43:34, miu wrote:
> On 2015/05/07 22:10:00, mcasas wrote:
> > Nit: this parenthesis isn't matched.
> 
> Assuming you're talking about the parentheses in the string literals since
this
> code compiles.  It appears matched to me (see line 188).  Am I missing
> something?

Oops you're right, I missed l.188. Forget my comment.

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/1124263004/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:146:
DimensionValueIsValid(height) &&
On 2015/05/08 06:43:34, miu wrote:
> On 2015/05/07 22:10:00, mcasas wrote:
> > Ni: shouldn't l.146-147 be justified @8th column (i.e. four blanks right of
> the
> > } )?
> 
> I don't quite understand, and the auto-indent feature in my editor didn't
reveal
> any issues.  Can you show me an example of how you think this should be
> indented?

I thought it should be
} else if (DimensionValueIsValid(width) &&
    DimensionValueIsValid(height) &&

but if the clang-format says it's fine it probably is.

[bbudge, 2015-05-08 19:28:24]

content/renderer/pepper LGTM

[DaleCurtis, 2015-05-08 20:57:22]

lgtm

[miu, 2015-05-08 21:07:56]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-05-08 21:11:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124263004/60001

[commit-bot: I haz the power, 2015-05-09 01:23:07]

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

[commit-bot: I haz the power, 2015-05-09 01:23:09]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[miu, 2015-05-09 01:40:25]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-05-09 01:48:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124263004/60001

[commit-bot: I haz the power, 2015-05-09 06:03:57]

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

[commit-bot: I haz the power, 2015-05-09 06:04:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[miu, 2015-05-09 18:58:02]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-05-09 18:59:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124263004/60001

[miu, 2015-05-09 21:12:49]

The CQ bit was unchecked by miu@chromium.org

[miu, 2015-05-09 22:13:39]

FYI--Landing with NOPRESUBMIT=true and NOTRY=true since the CQ has been
experiencing problems (since Friday) that is causing the chromium_presubmit
trybot to never complete for this commit.  As of this writing, all other trybots
were green and I confirmed the change compiles on my desktop (w/ the current
trunk).

[miu, 2015-05-09 22:13:53]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2015-05-09 22:14:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124263004/60001

[commit-bot: I haz the power, 2015-05-09 22:17:27]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-05-09 22:18:29]

Patchset 3 (id:??) landed as
https://crrev.com/f79202747a089a00f653e5827409c779395bda70
Cr-Commit-Position: refs/heads/master@{#329056}