Tab/Desktop Capture: Use requests instead of timer-based refreshing.

Tab/Desktop Capture: Use requests instead of timer-based refreshing.

Remove the long-standing, hacky timer-based frame capture mechanisms
from the tab (and Desktop-on-Aura) capture implementations.  Instead,
downstream consumers will now make refresh frame requests when needed,
and at the desired frequency for their specific use cases.

This change depends on https://codereview.chromium.org/1849003002/.

Also: 1) Deleted support implementation for the timer mechanisms from
SmoothEventSampler, 2) updated WCVCD unit tests to only expect frames
when explicitly requested, and 3) the NotificationCenter mechanism was
removed from the WCVCD unit tests because it was removed from the impl
long ago (but the unit tests were erroneously passing due to the timer
refreshes).

BUG=486274
Committed: https://crrev.com/e3d466694d3a14958b478408577158c4c6804fe4
Cr-Commit-Position: refs/heads/master@{#385668}

[miu, 2016-04-05 22:33:59]

miu@chromium.org changed reviewers:
+ isheriff@chromium.org, mcasas@chromium.org, xjz@chromium.org

[miu, 2016-04-05 22:34:00]

irfan/xjz: PTAL.

mcasas: Need OWNERS approval for 3-LOC bug fix in
	content/browser/renderer_host/media/video_capture_buffer_pool.cc.

[xjz, 2016-04-06 17:02:28]

lgtm

https://codereview.chromium.org/1864813002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:213: //
Called on refresh or mouse activity events.
nit: s/refresh/active refresh

https://codereview.chromium.org/1864813002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:223:
scoped_ptr<FrameSubscriber> refresh_subscriber_;
Is this only for active refresh events? Maybe add a comment.

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.cc:139: switch (event) {
nit: maybe just use if (event == kCompositorUpdate) { } else { }?

[Irfan, 2016-04-06 19:53:06]

Is the streaming side the downstream consumer for cast ? Do you have a
corresponding CL for that ?

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
File media/capture/content/screen_capture_device_core.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
media/capture/content/screen_capture_device_core.cc:68: void
ScreenCaptureDeviceCore::RequestRefreshFrame() {
Any value in providing feedback when the request is dropped ?

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
File media/capture/content/screen_capture_device_core.h (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
media/capture/content/screen_capture_device_core.h:58: // capture, and then do
so if the oracle agrees.
What happens when there is a request from consumer and the oracle for some
reason decides to not capture ?

Should this have been a force capture at oracle ? Should there be a response to
consumer when request cannot be honored ?

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/t...
File media/capture/content/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:157: if
(!ObserveEventAndDecideCapture(VideoCaptureOracle::kPassiveRefreshRequest,
I do not have the full picture here, but how does recording utilization and
capture events fit with a passive refresh since in that case, we  are not
capturing ?

Also, the naming observeEventAndDecideCapture seems to be a bit confusing for
the case of passive refresh where there is no capture ?

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.cc:19: const int
kUpperBoundDurationEstimateMicros = 1000000;  // 1 second
kUpperBoundFrameDuration... ?

[miu, 2016-04-06 22:33:54]

xjz/irfan: Responded to comments, addressed some.  PTAL.

BTW--Note that this change is dependent on this one (currently in the CQ):
https://codereview.chromium.org/1849003002

https://codereview.chromium.org/1864813002/diff/20001/content/browser/media/c...
File content/browser/media/capture/web_contents_video_capture_device.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:213: //
Called on refresh or mouse activity events.
On 2016/04/06 17:02:27, xjz wrote:
> nit: s/refresh/active refresh

Done.

https://codereview.chromium.org/1864813002/diff/20001/content/browser/media/c...
content/browser/media/capture/web_contents_video_capture_device.cc:223:
scoped_ptr<FrameSubscriber> refresh_subscriber_;
On 2016/04/06 17:02:27, xjz wrote:
> Is this only for active refresh events? Maybe add a comment.

This code is so far away from the logic that handles the passive refresh events,
that, IMHO, it's not necessary to document it here.

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
File media/capture/content/screen_capture_device_core.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
media/capture/content/screen_capture_device_core.cc:68: void
ScreenCaptureDeviceCore::RequestRefreshFrame() {
On 2016/04/06 19:53:05, Irfan wrote:
> Any value in providing feedback when the request is dropped ?

No.  The request originates in the renderer process and is meant to be a
one-way-communication and best-effort scheme.  There's documentation in the IPCs
and VideoCaptureDevice interface to explain further.

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
File media/capture/content/screen_capture_device_core.h (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/s...
media/capture/content/screen_capture_device_core.h:58: // capture, and then do
so if the oracle agrees.
On 2016/04/06 19:53:05, Irfan wrote:
> What happens when there is a request from consumer and the oracle for some
> reason decides to not capture ?

Nothing happens.  The oracle will decide not to capture whenever the refresh
request is unnecessary (because there was recently a frame delivered and/or
there is high expectation there will soon be another delivered soon anyway).

> Should this have been a force capture at oracle ? Should there be a response
to
> consumer when request cannot be honored ?

No: This is best-effort, as explained in my other comment.

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/t...
File media/capture/content/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:157: if
(!ObserveEventAndDecideCapture(VideoCaptureOracle::kPassiveRefreshRequest,
On 2016/04/06 19:53:05, Irfan wrote:
> I do not have the full picture here, but how does recording utilization and
> capture events fit with a passive refresh since in that case, we  are not
> capturing ?

From the oracle's point-of-view, the passive frames will still count as normal
captured frames.  When the refresh frames are delivered to the renderer, the
renderer will process them normally (since it cannot distinguish which are
normal vs refresh frames); and the utilization signals (for encoding and
transmission) will feed-back via the normal code paths.  This is intentional,
since we want to let Zero Config continue to automatically adjust (e.g.,
increase resolution for static screens).

> Also, the naming observeEventAndDecideCapture seems to be a bit confusing for
> the case of passive refresh where there is no capture ?

Well, there kind of *is* capture, it's just "capturing" an old buffer from the
video buffer pool rather than reading the frame from the GPU.  Do you have a
naming suggestion for this?

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/v...
File media/capture/content/video_capture_oracle.cc (right):

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.cc:19: const int
kUpperBoundDurationEstimateMicros = 1000000;  // 1 second
On 2016/04/06 19:53:05, Irfan wrote:
> kUpperBoundFrameDuration... ?

IMHO, adding "Frame" to this long name doesn't really make it that much clearer.
 It's already easily understood this is about frame durations, and it'd cause
some weird line-wrapping where this is used in the code, if the name were even
longer.

https://codereview.chromium.org/1864813002/diff/20001/media/capture/content/v...
media/capture/content/video_capture_oracle.cc:139: switch (event) {
On 2016/04/06 17:02:28, xjz wrote:
> nit: maybe just use if (event == kCompositorUpdate) { } else { }?

I would, except for two things:

1. This forces compile-time checking that all possible enum values are addressed
in this code.  (i.e., you get an error if someone adds a new item in the enum,
but doesn't decide how to handle it here)

2. I'm not sure whether I'm going to keep the refresh requests as being handled
the same way as the mouse cursor updates in the long-term.  I figured we'd start
with this behavior and see if things need to be improved.

-------

Actually, it just occurred to me that we should not allow refreshes (or mouse
updates) to happen if content was very-recently animating, since that could mess
up smoothness.  So, I made a logic change here to account for that.  PTAL.

[xjz, 2016-04-06 22:49:18]

lgtm

[Irfan, 2016-04-06 23:00:22]

lgtm

https://codereview.chromium.org/1864813002/diff/40001/media/capture/content/t...
File media/capture/content/thread_safe_capture_oracle.cc (right):

https://codereview.chromium.org/1864813002/diff/40001/media/capture/content/t...
media/capture/content/thread_safe_capture_oracle.cc:162: 
probably add a comment explaining why the capture itself is a no-op

[mcasas, 2016-04-06 23:09:01]

content/browser/renderer_host/media/ RS LGTM

[miu, 2016-04-06 23:59:04]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2016-04-07 00:00:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864813002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864813002/40001

[commit-bot: I haz the power, 2016-04-07 01:07:36]

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

[commit-bot: I haz the power, 2016-04-07 01:07:37]

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

[miu, 2016-04-07 01:21:18]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2016-04-07 01:22:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864813002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864813002/40001

[commit-bot: I haz the power, 2016-04-07 02:31:01]

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

[commit-bot: I haz the power, 2016-04-07 02:31:02]

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

[miu, 2016-04-07 04:42:52]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2016-04-07 04:43:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864813002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864813002/40001

[commit-bot: I haz the power, 2016-04-07 05:10:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-07 05:11:52]

Description was changed from

==========
Tab/Desktop Capture: Use requests instead of timer-based refreshing.

Remove the long-standing, hacky timer-based frame capture mechanisms
from the tab (and Desktop-on-Aura) capture implementations.  Instead,
downstream consumers will now make refresh frame requests when needed,
and at the desired frequency for their specific use cases.

This change depends on https://codereview.chromium.org/1849003002/.

Also: 1) Deleted support implementation for the timer mechanisms from
SmoothEventSampler, 2) updated WCVCD unit tests to only expect frames
when explicitly requested, and 3) the NotificationCenter mechanism was
removed from the WCVCD unit tests because it was removed from the impl
long ago (but the unit tests were erroneously passing due to the timer
refreshes).

BUG=486274
==========

to

==========
Tab/Desktop Capture: Use requests instead of timer-based refreshing.

Remove the long-standing, hacky timer-based frame capture mechanisms
from the tab (and Desktop-on-Aura) capture implementations.  Instead,
downstream consumers will now make refresh frame requests when needed,
and at the desired frequency for their specific use cases.

This change depends on https://codereview.chromium.org/1849003002/.

Also: 1) Deleted support implementation for the timer mechanisms from
SmoothEventSampler, 2) updated WCVCD unit tests to only expect frames
when explicitly requested, and 3) the NotificationCenter mechanism was
removed from the WCVCD unit tests because it was removed from the impl
long ago (but the unit tests were erroneously passing due to the timer
refreshes).

BUG=486274

Committed: https://crrev.com/e3d466694d3a14958b478408577158c4c6804fe4
Cr-Commit-Position: refs/heads/master@{#385668}
==========

[commit-bot: I haz the power, 2016-04-07 05:11:54]

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

[jam, 2016-04-07 21:46:35]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1865283003/ by jam@chromium.org.

The reason for reverting is: This has caused > 50 tryjobs to flake so far

BUG=601308.

[jam, 2016-04-07 21:48:09]

On 2016/04/07 21:46:35, jam wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/1865283003/ by mailto:jam@chromium.org.
> 
> The reason for reverting is: This has caused > 50 tryjobs to flake so far
> 
> BUG=601308.

I cancelled the revert since I found a fix landed.