De-dupe copy requests for tab capture in DelegatedFrameHost.

De-dupe copy requests for tab capture in DelegatedFrameHost.

Tab capture requests copies/readbacks in response to every delegated
frame swap from the renderer.  These are executed in a later step, when
the browser is composited.  Unfortunately, the browser may not be
composited for every render frame.  This means multiple copies/readbacks
may become enqueued, each of which is expensive, and will turn out to be
completely redundant as the results will be identical.  This change
detects this scenario, and de-dupes by "aborting" the older frame
subscriber callback.

BUG=464475
Committed: https://crrev.com/ddee0845cc298cbba6f9d8d6ff7fb9e06073f0e8
Cr-Commit-Position: refs/heads/master@{#319998}

[miu, 2015-03-07 03:46:39]

Patchset #1 (id:1) has been deleted

[miu, 2015-03-07 05:16:31]

Patchset #1 (id:20001) has been deleted

[miu, 2015-03-09 20:39:19]

miu@chromium.org changed reviewers:
+ ccameron@chromium.org, danakj@chromium.org, hubbe@chromium.org

[miu, 2015-03-09 20:39:20]

ccameron: Need OWNERS "rubber stamp" for
render_widget_host_view_aura_unittest.cc.  Feel free to take a look at the
delegated_frame_host* changes as well.

danakj: PTAL at delegated_frame_host* changes, as discussed in recent e-mail
thread.

hubbe: PTAL at the content/browser/media/capture/* changes, to adjust timestamps
for de-duped captures.

[danakj, 2015-03-09 22:20:15]

https://codereview.chromium.org/986823002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/986823002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2115: //
Simulate a composite by running the loop and then notifying
Can you maybe add a test here that shows de-duping works?

https://codereview.chromium.org/986823002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2119:
view_->GetDelegatedFrameHost()->OnCompositingStarted(nullptr,
Maybe you should OnCompositingEnded() here too, you know to match them.

[miu, 2015-03-10 02:50:43]

danakj: After chatting with my teammate here, things were simplified a lot by
doing a little queuing of the frame subscriber delivery callbacks in DFH (as
opposed having the weird "abort detection" logic downstream in
VideoCaptureOracle).  PTAL.  Also, I addressed your comments:

https://codereview.chromium.org/986823002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/986823002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2115: //
Simulate a composite by running the loop and then notifying
On 2015/03/09 22:20:15, danakj wrote:
> Can you maybe add a test here that shows de-duping works?

Done.

https://codereview.chromium.org/986823002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2119:
view_->GetDelegatedFrameHost()->OnCompositingStarted(nullptr,
On 2015/03/09 22:20:15, danakj wrote:
> Maybe you should OnCompositingEnded() here too, you know to match them.

Done.

[hubbe, 2015-03-10 17:06:53]

lgtm

https://codereview.chromium.org/986823002/diff/100001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/986823002/diff/100001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:275: if
(!frame_subscriber_callbacks_.empty() &&
The empty() call should not be required, as it should not be possible for the
queue to be empty if frame_subscriber_copy_request_pending_ is true. Perhaps
make it a DCHECK instead?

[danakj, 2015-03-10 17:17:18]

https://codereview.chromium.org/986823002/diff/100001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/986823002/diff/100001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:300: NOTREACHED();
Don't do "NOTREACHED(); return;" please. Either DCHECK() and don't handle the
case (it can't happen right?) or don't dcheck and handle it because it can
happen. I think here it can't happen, so just DCHECK(!empty()).

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2136: for
(int i = 0; i < 50; ++i) {
isnt i < 3 enough? the rest are redundant?

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2177:
std::vector<linked_ptr<cc::CopyOutputRequest>> requests;
Can you use ScopedVector instead of linked_ptr please?

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2270:
EXPECT_EQ(2, callback_count_);
I think I prefer to keep these EXPECTs after the SendTextureResult. They also
help show that the SendTextureResult isn't doing something? Is there some reason
they moved above?

[miu, 2015-03-10 20:54:08]

https://codereview.chromium.org/986823002/diff/100001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/986823002/diff/100001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:275: if
(!frame_subscriber_callbacks_.empty() &&
On 2015/03/10 17:06:53, hubbe wrote:
> The empty() call should not be required, as it should not be possible for the
> queue to be empty if frame_subscriber_copy_request_pending_ is true. Perhaps
> make it a DCHECK instead?

Done.

https://codereview.chromium.org/986823002/diff/100001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:300: NOTREACHED();
On 2015/03/10 17:17:17, danakj wrote:
> Don't do "NOTREACHED(); return;" please. Either DCHECK() and don't handle the
> case (it can't happen right?) or don't dcheck and handle it because it can
> happen. I think here it can't happen, so just DCHECK(!empty()).

Done.

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2136: for
(int i = 0; i < 50; ++i) {
On 2015/03/10 17:17:17, danakj wrote:
> isnt i < 3 enough? the rest are redundant?

Done.

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2177:
std::vector<linked_ptr<cc::CopyOutputRequest>> requests;
On 2015/03/10 17:17:17, danakj wrote:
> Can you use ScopedVector instead of linked_ptr please?

Done.

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2270:
EXPECT_EQ(2, callback_count_);
On 2015/03/10 17:17:17, danakj wrote:
> I think I prefer to keep these EXPECTs after the SendTextureResult. They also
> help show that the SendTextureResult isn't doing something? Is there some
reason
> they moved above?

The reason I moved them is that the TearDownEnvironment() call will cause DFH to
be destroyed, and I wanted to test that the callbacks are run (with failure
status) from ~DFH().  So, I've added them after the SendTextureResult() call to
make sure that call doesn't somehow cause callbacks to be run twice.

[danakj, 2015-03-10 20:59:21]

LGTM

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/986823002/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2270:
EXPECT_EQ(2, callback_count_);
On 2015/03/10 20:54:07, miu wrote:
> On 2015/03/10 17:17:17, danakj wrote:
> > I think I prefer to keep these EXPECTs after the SendTextureResult. They
also
> > help show that the SendTextureResult isn't doing something? Is there some
> reason
> > they moved above?
> 
> The reason I moved them is that the TearDownEnvironment() call will cause DFH
to
> be destroyed, and I wanted to test that the callbacks are run (with failure
> status) from ~DFH().  So, I've added them after the SendTextureResult() call
to
> make sure that call doesn't somehow cause callbacks to be run twice.

Cool thanks! SG

https://codereview.chromium.org/986823002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/986823002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2175: for
(int i = 0; i < 10; ++i) {
(btw 3 would be enough here too right?)

[ccameron, 2015-03-10 21:00:51]

OWNER lgtm for content/renderer_host

[miu, 2015-03-10 21:05:21]

Thanks all for the review!  :)

https://codereview.chromium.org/986823002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/986823002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2175: for
(int i = 0; i < 10; ++i) {
On 2015/03/10 20:59:21, danakj wrote:
> (btw 3 would be enough here too right?)

Oh yeah.  Duh.  Done; and in the "normal case" loop as well.

[miu, 2015-03-10 21:05:27]

The CQ bit was checked by miu@chromium.org

[miu, 2015-03-10 21:05:27]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org,
danakj@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/986823002/#ps160001
(title: "3 is enough!  :)")

[commit-bot: I haz the power, 2015-03-10 21:06:16]

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

[commit-bot: I haz the power, 2015-03-10 23:30:56]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-03-10 23:31:50]

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

[miu, 2015-03-23 19:25:17]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.chromium.org/1010803013/ by miu@chromium.org.

The reason for reverting is: Weird frame flickering effect in capture on some
systems.  https://crbug.com/469345.