aura: Allow delegated frames to be used by more than one impl layer.

aura: Allow delegated frames to be used by more than one impl layer.

Delegated frames can be used by multiple impl layers within a single
compositor if they are given to different DelegatedRendererLayers which
each send the frame to their impl layer.

Also, they can be given to different DelegatedRendererLayers that live
in different compositors.

So we need something that outlives any DelegatedRendererLayer and will
receive returned resources from the impl side of multiple compositors
and know when to release resoures back to the child compositor, and how
many resources to release.

This patch introduces two new classes:

DelegatedFrameProvider. This class holds a delegated frame and notifies
a single DelegatedRendererLayer (the latest one to attach to it) about
new frames so that it can grab them and display them. If a new
DelegatedRendererLayer steals a DelegatedFrameProvider it will receive
notifications in the future and the first layer will stop updating.

DelegatedFrameResourceCollection. This class holds resources and ref
counts on them to keep track of how many refs are being used by
compositor instances, and how many refs have been received from the
child compositor. Once a resource is not in use anymore by any
compositors, all references passed from the child compositor will
be made available via TakeUnusedResourcesForChildCompositor().

Tests:
LayerTreeHostDelegatedTestTwoImplLayers
LayerTreeHostDelegatedTestRemoveAndAddToTree
LayerTreeHostDelegatedTestRemoveAndChangeResources

R=piman,alexst@chromium.org
BUG=263069
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227807

[danakj, 2013-10-04 21:53:34]

piman: please review
alexst: please review content/renderer/browser_plugin

I'm going to add a couple more unit tests to satisfy myself.

[danakj, 2013-10-04 23:19:07]

Uploaded a version with ref-counted FrameProvider and ResourceCollection so more
than one DRL can show frames from a single FrameProvider and all continue
getting updates.

[alexst (slow to review), 2013-10-04 23:23:56]

https://codereview.chromium.org/26023004/diff/7001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/26023004/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:368:
background_layer_->AddChild(delegated_layer_);
I think if the frame_size changes, you will leak a delegated_layer_ here.
background_layer_ would still have the last layer you created as its child .
You'll need to remove delegated_layer_ from background_layer_ if it's not null.

[danakj, 2013-10-04 23:26:37]

https://codereview.chromium.org/26023004/diff/7001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/26023004/diff/7001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:368:
background_layer_->AddChild(delegated_layer_);
On 2013/10/04 23:23:56, alexst wrote:
> I think if the frame_size changes, you will leak a delegated_layer_ here.
> background_layer_ would still have the last layer you created as its child .
> You'll need to remove delegated_layer_ from background_layer_ if it's not
null.

Ah! Thanks. Done.

[alexst (slow to review), 2013-10-04 23:32:43]

https://codereview.chromium.org/26023004/diff/16001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/26023004/diff/16001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:350:
delegated_layer_.get());
One more comment, this is trouble here. delegated_layer_ was not yet created and
this will try to resize it.

[piman, 2013-10-04 23:42:24]

The patch looks really good.
Do you think we should add unit tests for the minute logic in DFRC/DFP, or do
you think it's covered well enough with the DRL tests?

https://codereview.chromium.org/26023004/diff/6001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/26023004/diff/6001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer_impl.cc:106: DCHECK_NE(0u,
frame_data->render_pass_list.size());
nit: DCHECK(!frame_data->render_pass_list.empty()) ?

https://codereview.chromium.org/26023004/diff/6001/cc/trees/layer_tree_host_u...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/26023004/diff/6001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest_delegated.cc:2041:
LayerTreeHostDelegatedTestTwoImplLayersTwoFrames);
Could we add one test that tries to put a layer in the tree, commit, remove it,
commit, put it again, and check that we didn't lose the frame/resources in the
process?

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1481: // any
resources from the old output surface with the new output surface id.
I wonder if we should have the DelegatedFrameResourceCollection manage the
output_surface_id somehow? When we receive the resources and create the DFRC, we
know exactly what surface id we're supposed to return them to. Then in
SendDelegatedFrameAck we could check the surface id the resource_collection_
corresponds to, and only return the resources through the SwapCompositorFrameAck
if it matches the surface id. If not, we can send the ReclaimCompositorResources
message (maybe? Or dropping resources is probably ok).

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1490: if
(!resource_collection_)
nit: needs {}

https://codereview.chromium.org/26023004/diff/6001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/26023004/diff/6001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:352:
DCHECK_NE(0u, frame_data->render_pass_list.size());
nit: DCHECK(!frame_data->render_pass_list.empty());

https://codereview.chromium.org/26023004/diff/21001/cc/layers/delegated_frame...
File cc/layers/delegated_frame_provider.cc (right):

https://codereview.chromium.org/26023004/diff/21001/cc/layers/delegated_frame...
cc/layers/delegated_frame_provider.cc:71:
observers_[i].layer->ProviderHasNewFrame();
This can't result (indirectly) in the observer removing itself (or adding
another one), right?

[danakj, 2013-10-07 21:57:38]

I could add some unit tests for these classes directly, though judging from the
number of failures I had to constantly work through in the LTTDelegatedTests, I
do feel like we have decent coverage.

https://codereview.chromium.org/26023004/diff/6001/cc/layers/delegated_render...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/26023004/diff/6001/cc/layers/delegated_render...
cc/layers/delegated_renderer_layer_impl.cc:106: DCHECK_NE(0u,
frame_data->render_pass_list.size());
On 2013/10/04 23:42:25, piman wrote:
> nit: DCHECK(!frame_data->render_pass_list.empty()) ?

Done.

https://codereview.chromium.org/26023004/diff/6001/cc/trees/layer_tree_host_u...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/26023004/diff/6001/cc/trees/layer_tree_host_u...
cc/trees/layer_tree_host_unittest_delegated.cc:2041:
LayerTreeHostDelegatedTestTwoImplLayersTwoFrames);
On 2013/10/04 23:42:25, piman wrote:
> Could we add one test that tries to put a layer in the tree, commit, remove
it,
> commit, put it again, and check that we didn't lose the frame/resources in the
> process?

Done. The main thread needs to hold a reference, so I've made the
DelegatedFrameProvider hold a ref on the last resources given to it.

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1481: // any
resources from the old output surface with the new output surface id.
On 2013/10/04 23:42:25, piman wrote:
> I wonder if we should have the DelegatedFrameResourceCollection manage the
> output_surface_id somehow? When we receive the resources and create the DFRC,
we
> know exactly what surface id we're supposed to return them to. Then in
> SendDelegatedFrameAck we could check the surface id the resource_collection_
> corresponds to, and only return the resources through the
SwapCompositorFrameAck
> if it matches the surface id. If not, we can send the
ReclaimCompositorResources
> message (maybe? Or dropping resources is probably ok).

Hm, if I'm understanding your suggestion, we'll have replaced the
resource_collection_ just below here anyway. So we'd need to keep around the old
collection just to wait for the resources to come back, but the child won't want
them. That's why I think we should just force lose them here so we can tell the
child immediately and keep this simpler with only one resource collection alive.

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1490: if
(!resource_collection_)
On 2013/10/04 23:42:25, piman wrote:
> nit: needs {}

Done.

https://codereview.chromium.org/26023004/diff/6001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/26023004/diff/6001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:352:
DCHECK_NE(0u, frame_data->render_pass_list.size());
On 2013/10/04 23:42:25, piman wrote:
> nit: DCHECK(!frame_data->render_pass_list.empty());

Done.

https://codereview.chromium.org/26023004/diff/16001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/26023004/diff/16001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:350:
delegated_layer_.get());
On 2013/10/04 23:32:44, alexst wrote:
> One more comment, this is trouble here. delegated_layer_ was not yet created
and
> this will try to resize it.

Ah, sorry my bad. I've moved this down.

https://codereview.chromium.org/26023004/diff/21001/cc/layers/delegated_frame...
File cc/layers/delegated_frame_provider.cc (right):

https://codereview.chromium.org/26023004/diff/21001/cc/layers/delegated_frame...
cc/layers/delegated_frame_provider.cc:71:
observers_[i].layer->ProviderHasNewFrame();
On 2013/10/04 23:42:25, piman wrote:
> This can't result (indirectly) in the observer removing itself (or adding
> another one), right?

In theory yes. In practice they only are added/removed in the DRL
constructor/destructor. And this callback does not cause the DRL to destroy
itself.

[danakj, 2013-10-07 23:29:20]

Added some cc/layers/delegated_frame_provider_unittest.cc tests for the DFP
directly.

I'll fix Android build tomorrow.

[piman, 2013-10-08 03:38:08]

lgtm

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/26023004/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:1481: // any
resources from the old output surface with the new output surface id.
On 2013/10/07 21:57:39, danakj wrote:
> On 2013/10/04 23:42:25, piman wrote:
> > I wonder if we should have the DelegatedFrameResourceCollection manage the
> > output_surface_id somehow? When we receive the resources and create the
DFRC,
> we
> > know exactly what surface id we're supposed to return them to. Then in
> > SendDelegatedFrameAck we could check the surface id the resource_collection_
> > corresponds to, and only return the resources through the
> SwapCompositorFrameAck
> > if it matches the surface id. If not, we can send the
> ReclaimCompositorResources
> > message (maybe? Or dropping resources is probably ok).
> 
> Hm, if I'm understanding your suggestion, we'll have replaced the
> resource_collection_ just below here anyway. So we'd need to keep around the
old
> collection just to wait for the resources to come back, but the child won't
want
> them. That's why I think we should just force lose them here so we can tell
the
> child immediately and keep this simpler with only one resource collection
alive.

Ok. The thing I'm kinda worried is below in SendDelegatedFrameAck, I'm not sure
what guarantees the resources we take back come from the right
output_surface_id. But that's not specific to this patch, so we can iterate
later if it proves to be an issue.

[alexst (slow to review), 2013-10-08 12:43:36]

LGTM!

[danakj, 2013-10-08 22:05:36]

+sievers for  content/browser/renderer_host/render_widget_host_view_android

This code duplicates a lot of logic from RWHVAura, we should look at merging
somehow.

[no sievers, 2013-10-08 22:54:56]

lgtm with 1 comment.

https://codereview.chromium.org/26023004/diff/128001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26023004/diff/128001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:702:
AttachLayers();
+dtrainor

I think you want to check if |are_layers_attached_| before calling
AttachLayers(). I assume otherwise we might get weird Show/Hide races where we
end up attaching the wrong layers to the ContentViewCore.

[danakj, 2013-10-09 15:44:29]

https://codereview.chromium.org/26023004/diff/128001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/26023004/diff/128001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:702:
AttachLayers();
On 2013/10/08 22:54:56, sievers wrote:
> +dtrainor
> 
> I think you want to check if |are_layers_attached_| before calling
> AttachLayers(). I assume otherwise we might get weird Show/Hide races where we
> end up attaching the wrong layers to the ContentViewCore.

Oh sounds right! Will do, thanks.

[commit-bot: I haz the power, 2013-10-09 15:48:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/26023004/137001

[commit-bot: I haz the power, 2013-10-09 21:03:47]

Change committed as 227807