Automatic resolution throttling for screen capture pipeline.

Automatic resolution throttling for screen capture pipeline.

This makes VideoCaptureOracle responsible for proposing frame capture
sizes.  The oracle is given utilization feedback signals for each frame,
and uses these to decide when to increase or decrease the capture size.
Its goal is to automatically adapt to the current, ever-changing user
environment and maintain capture at the frame rate of the content by lowering
the capture resolution when necessary.

NOTE: This new functionality is not enabled by default.

BUG=156767
Committed: https://crrev.com/b372398531cba1b6a917a996e50f8ddf2e033d55
Cr-Commit-Position: refs/heads/master@{#337731}

[miu, 2015-06-23 05:01:46]

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

[miu, 2015-06-23 05:01:46]

hubbe: Finally here!  PTAL.  Unit tests for all this coming soon.

[hubbe, 2015-06-23 20:04:31]

https://codereview.chromium.org/1199593005/diff/1/media/capture/thread_safe_c...
File media/capture/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1199593005/diff/1/media/capture/thread_safe_c...
media/capture/thread_safe_capture_oracle.cc:73: const double
attenuated_utilization = client_->GetBufferPoolUtilization() /
Multiplication is potentially faster than division.

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
File media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:80: // to |analyze_time|.
analyze_time is roughly *now*, isn't it?
(Perhaps call the variable "now" ?)

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:197: if (std::isfinite(pool_utilization)
&& pool_utilization >= 0.0) {
This seems like it could hide bugs.
Can we DCHECK() these instead?

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:286: if
(!std::isfinite(resource_utilization) || resource_utilization <= 0.0)
Bug hiding!
However, these values come from the not-so-trusted renderer process, right? So,
perhaps some paranoia is warrented.

Either way, we should at least log something.

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:343:
pessimistic_pool_utilization_.Reset(1.5, ignore_before_time);
1.5?
constant?

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:454: estimated_capable_area_.reset_time())
{
odd indentation

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
File media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.h:45: // Sets the source content size.  This
may not have an immediate affect on the
affect -> effect

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.h:77: // if the consumer is over/underloaded.
 |resource_utilization| is a value in
over/underloaded -> overloaded/underutilized ?

[miu, 2015-06-25 20:41:00]

Patchset #2 (id:20001) has been deleted

[miu, 2015-06-25 20:49:00]

Hubbe: PTAL.  Replaced pessimistic accumulators with a simpler "capable of
increase for 30 seconds" heuristic.

https://codereview.chromium.org/1199593005/diff/1/media/capture/thread_safe_c...
File media/capture/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1199593005/diff/1/media/capture/thread_safe_c...
media/capture/thread_safe_capture_oracle.cc:73: const double
attenuated_utilization = client_->GetBufferPoolUtilization() /
On 2015/06/23 20:04:30, hubbe wrote:
> Multiplication is potentially faster than division.

Eliminated one of the divisions.  If C++11 constexpr was supported on all
toolchains, I could eliminate more...

Bikeshedding discussion: Technically, float division is usually implemented in
hardware as "inverse the denominator, then multiply" because inverting a float
is as simple as negating the exponent.

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
File media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:80: // to |analyze_time|.
On 2015/06/23 20:04:31, hubbe wrote:
> analyze_time is roughly *now*, isn't it?
> (Perhaps call the variable "now" ?)

Done.  Makes sense here, to aid in readability.  However, I don't want to change
it in the AnalyzeAndAdjust() methods, since it really isn't TimeTicks::Now().

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:197: if (std::isfinite(pool_utilization)
&& pool_utilization >= 0.0) {
On 2015/06/23 20:04:31, hubbe wrote:
> This seems like it could hide bugs.
> Can we DCHECK() these instead?

Done.

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:286: if
(!std::isfinite(resource_utilization) || resource_utilization <= 0.0)
On 2015/06/23 20:04:31, hubbe wrote:
> Bug hiding!
> However, these values come from the not-so-trusted renderer process, right?
So,
> perhaps some paranoia is warrented.
> 
> Either way, we should at least log something.

Done.

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:343:
pessimistic_pool_utilization_.Reset(1.5, ignore_before_time);
On 2015/06/23 20:04:31, hubbe wrote:
> 1.5?
> constant?

Not applicable, since I got rid of these extra accumulators.

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.cc:454: estimated_capable_area_.reset_time())
{
On 2015/06/23 20:04:31, hubbe wrote:
> odd indentation

Done.

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
File media/capture/video_capture_oracle.h (right):

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.h:45: // Sets the source content size.  This
may not have an immediate affect on the
On 2015/06/23 20:04:31, hubbe wrote:
> affect -> effect

Done.  On a Dvorak keyboard, that's a typo.  ;-)

https://codereview.chromium.org/1199593005/diff/1/media/capture/video_capture...
media/capture/video_capture_oracle.h:77: // if the consumer is over/underloaded.
 |resource_utilization| is a value in
On 2015/06/23 20:04:31, hubbe wrote:
> over/underloaded -> overloaded/underutilized ?

Done.

[hubbe, 2015-06-25 21:57:43]

lgtm

https://codereview.chromium.org/1199593005/diff/40001/media/capture/video_cap...
File media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/1199593005/diff/40001/media/capture/video_cap...
media/capture/video_capture_oracle.cc:55: const int
kDebouncingPeriodForAnimatedContentMicros = 3000000;
All of these constants are a bit hard to read (too many zeroes)
Perhaps use millis? (Or seconds?)

[miu, 2015-07-01 22:11:47]

hubbe: PTAL.  Added unit tests and the "bool to enable" glue.

https://codereview.chromium.org/1199593005/diff/40001/media/capture/video_cap...
File media/capture/video_capture_oracle.cc (right):

https://codereview.chromium.org/1199593005/diff/40001/media/capture/video_cap...
media/capture/video_capture_oracle.cc:55: const int
kDebouncingPeriodForAnimatedContentMicros = 3000000;
On 2015/06/25 21:57:43, hubbe wrote:
> All of these constants are a bit hard to read (too many zeroes)
> Perhaps use millis? (Or seconds?)
> 

Done.  Sort of.  It all comes back down to all those extra division operations. 
;-)  If you feel strongly about this, I can change it again, but how about my
solution of just adding a little comment to the right of each for clarification?

[hubbe, 2015-07-08 00:13:04]

lgtm

https://codereview.chromium.org/1199593005/diff/80002/media/capture/video_cap...
File media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/1199593005/diff/80002/media/capture/video_cap...
media/capture/video_capture_oracle_unittest.cc:379: for (base::TimeTicks end_t =
t + base::TimeDelta::FromSeconds(10);
I think it would be better to move the end_t assignment to the previous line and
make the first part of the for statement empty (and on one line)
Currently it looks too much like you're initializing a loop variable here.
(same in a few places below)

[miu, 2015-07-08 00:56:08]

Thanks for the review!  :)

https://codereview.chromium.org/1199593005/diff/80002/media/capture/video_cap...
File media/capture/video_capture_oracle_unittest.cc (right):

https://codereview.chromium.org/1199593005/diff/80002/media/capture/video_cap...
media/capture/video_capture_oracle_unittest.cc:379: for (base::TimeTicks end_t =
t + base::TimeDelta::FromSeconds(10);
On 2015/07/08 00:13:03, hubbe wrote:
> I think it would be better to move the end_t assignment to the previous line
and
> make the first part of the for statement empty (and on one line)
> Currently it looks too much like you're initializing a loop variable here.
> (same in a few places below)

Done.

[miu, 2015-07-08 00:56:12]

The CQ bit was checked by miu@chromium.org

[miu, 2015-07-08 00:56:13]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org
Link to the patchset: https://codereview.chromium.org/1199593005/#ps110001
(title: "Move all end_t assignments out of for-loop init.")

[commit-bot: I haz the power, 2015-07-08 00:58:08]

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

[commit-bot: I haz the power, 2015-07-08 02:07:34]

Committed patchset #6 (id:110001)

[commit-bot: I haz the power, 2015-07-08 02:08:26]

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