Plumb overlay processing into DirectRenderer.

Plumb overlay processing into DirectRenderer.

This change tests for overlays inside DirectRenderer::DrawFrame and if
new overlay passes were produced, it skips rendering them and forwards the
information to FinishDrawingFrame to allow subclasses like GLRenderer and
SoftwareRenderer to schedule overlays in a manner specific to their
implementation.

GLRenderer schedules overlays via ContextSupport.

Adds more tests to ensure overlay quads are not drawn and if no overlays
are present, no quads are skipped.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260267

[alexst (slow to review), 2014-03-21 14:36:18]

Initial bits of plumbing plus tests to make sure existing stuff doesn't break.

[enne (OOO), 2014-03-21 18:15:44]

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc...
cc/output/direct_renderer.cc:227: frame.render_passes_in_draw_order =
render_passes_in_draw_order;
This variable is a little awkward.  It says render_passes_in_draw_order, but it
only gets set when the OverlayProcessor succeeds, and it's only used as a
boolean in tests.

Could you always set it, or have your test verify that DrawRenderPass doesn't
occur for overlays that you expect to happen?

[enne (OOO), 2014-03-21 18:16:08]

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc...
cc/output/direct_renderer.cc:227: frame.render_passes_in_draw_order =
render_passes_in_draw_order;
(Er, sorry, I see that it's not just used as a boolean, but it's still only used
in tests.)

[alexst (slow to review), 2014-03-21 18:37:41]

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc...
cc/output/direct_renderer.cc:227: frame.render_passes_in_draw_order =
render_passes_in_draw_order;
On 2014/03/21 18:16:08, enne wrote:
> (Er, sorry, I see that it's not just used as a boolean, but it's still only
used
> in tests.)

It's not useful unless overlays are present, so it could be utilized to quickly
skip extra work in GLRenderer. I could rename it overlay_passes_in_draw_order,
since it's what they are used as. 

It already tests that the contents of that pass are not drawn via DrawQuad. I
could make DrawRenderPass virtual and test it that way as well if you think the
above is insufficient.

As for it only being used in tests, I decided to break my patch here fully
tested because the next one will be pretty long and tricky. It'll be adding
plumbing to the GPU process and buffer lifetimes and I didn't want to mix the
two to avoid mega-patches.

[piman, 2014-03-21 21:45:12]

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc...
cc/output/direct_renderer.cc:227: frame.render_passes_in_draw_order =
render_passes_in_draw_order;
On 2014/03/21 18:37:41, alexst wrote:
> On 2014/03/21 18:16:08, enne wrote:
> > (Er, sorry, I see that it's not just used as a boolean, but it's still only
> used
> > in tests.)
> 
> It's not useful unless overlays are present, so it could be utilized to
quickly
> skip extra work in GLRenderer. I could rename it overlay_passes_in_draw_order,
> since it's what they are used as. 
> 
> It already tests that the contents of that pass are not drawn via DrawQuad. I
> could make DrawRenderPass virtual and test it that way as well if you think
the
> above is insufficient.
> 
> As for it only being used in tests, I decided to break my patch here fully
> tested because the next one will be pretty long and tricky. It'll be adding
> plumbing to the GPU process and buffer lifetimes and I didn't want to mix the
> two to avoid mega-patches. 

Do you have a CL that shows how it will be used? Your "overall" CL doesn't seem
to have it... What more are you thinking of skipping beyond what you have below?

[alexst (slow to review), 2014-03-24 12:46:45]

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/1/cc/output/direct_renderer.cc...
cc/output/direct_renderer.cc:227: frame.render_passes_in_draw_order =
render_passes_in_draw_order;
On 2014/03/21 21:45:12, piman wrote:
> On 2014/03/21 18:37:41, alexst wrote:
> > On 2014/03/21 18:16:08, enne wrote:
> > > (Er, sorry, I see that it's not just used as a boolean, but it's still
only
> > used
> > > in tests.)
> > 
> > It's not useful unless overlays are present, so it could be utilized to
> quickly
> > skip extra work in GLRenderer. I could rename it
overlay_passes_in_draw_order,
> > since it's what they are used as. 
> > 
> > It already tests that the contents of that pass are not drawn via DrawQuad.
I
> > could make DrawRenderPass virtual and test it that way as well if you think
> the
> > above is insufficient.
> > 
> > As for it only being used in tests, I decided to break my patch here fully
> > tested because the next one will be pretty long and tricky. It'll be adding
> > plumbing to the GPU process and buffer lifetimes and I didn't want to mix
the
> > two to avoid mega-patches. 
> 
> Do you have a CL that shows how it will be used? Your "overall" CL doesn't
seem
> to have it... What more are you thinking of skipping beyond what you have
below?

I'll add more code so this makes more sense. I was going to get a read lock on
the texture resources in GLRenderer:: FinishDrawingFrame, hold on to them until
the next swap and make a call on context support to schedule the planes. The
plumbing was in a different place, which I didn't like in the overall patch, so
I'm doing it here instead. Patch coming up.

[alexst (slow to review), 2014-03-24 23:25:40]

PTAL.

I plumbed this a bit further to give you better context.

https://codereview.chromium.org/208213003/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2179:
in_use_overlay_resources_.swap(pending_overlay_resources_);
This particular section, I am not 100% certain about and would like some
feedback on. I think this is sufficient and the sync point that these buffers
will be returned with should prevent the renderer from writing while they are
scanned out, but I want to be sure there is no race condition with SwapBuffers
above.

[piman, 2014-03-25 00:08:08]

https://codereview.chromium.org/208213003/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2179:
in_use_overlay_resources_.swap(pending_overlay_resources_);
On 2014/03/24 23:25:40, alexst wrote:
> This particular section, I am not 100% certain about and would like some
> feedback on. I think this is sufficient and the sync point that these buffers
> will be returned with should prevent the renderer from writing while they are
> scanned out, but I want to be sure there is no race condition with SwapBuffers
> above.

The ScopedReadLockGL is not enough to prevent the resource from being returned.
The assumption is that the ScopedReadLockGL is short term, just for the duration
of the draw calls, but would not span frames. There are some DCHECKs that I
think would trigger, when we want to return the resources to the child. See e.g.
ResourceProvider::DeleteAndReturnUnusedResourcesToChild or
ResourceProvider::DeleteResource, where we
DCHECK(!resource.lock_for_read_count);

I think we have 2 ways of dealing with this:
1- add logic to make ScopedReadLockGL prevent the returns. I.e. remove all these
DCHECKs and instead handle gracefully.
2- use the logic that is meant to deal with that for delegated renderers, i.e.
through the resource's "exported_count". The idea is to treat the overlays as a
"parent" compositor. We would "export" the resource when giving it to an overlay
plane, and "return" it on the next swap (well, the one *after* we commit the
overlays). The count would make sure that if we reuse a resource across frames
we wouldn't return it to the child until it's fully not used.

Most of the logic already exists for (2), it's just a matter of adding a path
for overlays (unless we can reuse the
PrepareSendToParent/ReceiveReturnsFromParent logic?). I'm not sure how much is
needed for (1), it may be small.

https://codereview.chromium.org/208213003/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3068: continue;
I wonder if it could make sense to construct the overlay list in DirectRenderer,
instead of passing the full set of render passes, construct the list of overlay
passes as we skip them when drawing.

https://codereview.chromium.org/208213003/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3085: gfx::RectF uv_rect =
BoundingRect(quad.uv_top_left, quad.uv_bottom_right);
Mmh, a lot of these we computed previously when trying to figure out what can go
as an overlay.
Is there a way we could reuse that?
E.g. having ProcessForOverlays output a list of OverlayCandidates, that we'd
pass back through here?

https://codereview.chromium.org/208213003/diff/40001/gpu/command_buffer/clien...
File gpu/command_buffer/client/context_support.h (right):

https://codereview.chromium.org/208213003/diff/40001/gpu/command_buffer/clien...
gpu/command_buffer/client/context_support.h:39: unsigned plane_transform,
nit: it'd make sense to have the enum down to gpu/ so that we can use it here.

[alexst (slow to review), 2014-03-26 01:23:51]

Thanks for the feedback re:exporting resources.

This should follow roughly the same code path that child/parent renderers do as
far as resources. I also added some tests to check that overlay textures are
held past the SwapBuffers call.

The resource management code is pretty complicated, so hopefully I didn't
violate any assumptions.

I plumbed the overlay candidates all the way through and removed changes to
RenderPass since they are no longer needed.

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3069: static void CollectResources(const
ReturnedResourceArray& returned) {}

Since these resources never crossed any process boundaries during
PrepareSendToParent/ReceiveReturnsFromParent,  and child and parent are
technically the same here, if I understand it correctly, they'll get sent back
to the actual child renderers on the next sweep? Or is there more than meets the
eye?

[piman, 2014-03-26 03:43:32]

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:260: }
If you keep the "child" logic (see below), you'd need to destroy the child here,
to make sure we send back the resources from the last frame.
Maybe we need to "turn off" the overlays too?

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2185: }
nit: TransferableResource::ReturnResources

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2187: if (unused_overlay_resources.size())
nit: if (!unused_overlay_resources.empty())

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3069: static void CollectResources(const
ReturnedResourceArray& returned) {}
On 2014/03/26 01:23:51, alexst wrote:
> 
> Since these resources never crossed any process boundaries during
> PrepareSendToParent/ReceiveReturnsFromParent,  and child and parent are
> technically the same here, if I understand it correctly, they'll get sent back
> to the actual child renderers on the next sweep? Or is there more than meets
the
> eye?

This will be called from DeclareUsedResourcesFromChild when a resource goes from
used to !used, with the count of how many times it was "imported". That's
because we only get a sync point when we're fully done with the resource.
However in this case, because the parent and child are the same, it's ok to not
have a sync point (what ToReturnedResource above does), and return the resource
multiple times with a count of 1 each time, as you're doing.
OTOH returning directly from here would potentially be "too early" since in
theory the producer could kick commands to modify the resource before the
SwapBuffers that takes them out happened. I suppose you could delay the
DeclareUsedResourcesFromChild until the SwapBuffers and ReceiveReturnsFromParent
directly from here. It may be slightly simpler code (no need to track
pending_overlay_resources_ and in_use_overlay_resources_, just need to "pass"
|resources| from ScheduleOverlays to SwapBuffers).

However, see below for alternatives.

https://codereview.chromium.org/208213003/diff/60001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:3113: pending_overlay_resources_);
So, this is unexpected. Looking at this code, it kinda makes sense, but I wonder
if there is a better way.

Basically, we're using the resource provider to export resources, and then
re-import them, essentially just to manage the "exported" count. This has some
side effects at the GL level (we will create and destroy textures, not a big
deal, just slightly wasteful, and send extra IPCs to insert sync points that
we'll never need), and requires the remapping below... Conceptually it feels
like a lot of overhead, even though it's probably not measurable (since we have
at most a handful of overlays).

Some ways we could make it simpler/cheaper:
- have ScheduleOverlayPlane take a mailbox directly instead of a texture id. The
service side can do the translation. That way we wouldn't need to manage the
Child side at all. We can use pending_overlay_resources_ to translate
resource->mailbox
- have an "export lock" thing, that we keep around together with the
ScopedReadLockGL (or have a superclass that does both the  ScopedReadLockGL and
the export refcount), so that we don't even need to do the PrepareSendToParent
and friends
- have the lock_for_read_count taken into account before returning/deleting
resources in ResourceProvider

https://codereview.chromium.org/208213003/diff/60001/gpu/command_buffer/clien...
File gpu/command_buffer/client/context_support.h (right):

https://codereview.chromium.org/208213003/diff/60001/gpu/command_buffer/clien...
gpu/command_buffer/client/context_support.h:37: // primary surface during the
next buffer swap.
You may want to mention that this is not stateful, i.e. needs to be reissued
before the next swap?

At least I gather that's what we're planning to have given the rest of the code.

[alexst (slow to review), 2014-03-26 20:48:49]

So I tried going down a simpler resource export to parent path and then had a
realization that it's probably not the optimal way to go.

Imagine scheduling an overlay on frame 1 and doing exported_count++ via some
mechanism, then on frame 2 overlay buffer is the same, but now there is
something above it, you can't use overlays any more, so you fall back to
compositing in GL and now you are trying to take a read lock on an "exported"
resource and hit a dcheck. It could probably be fixed, but feels like the wrong
way to go.

So I went back to the lock_for_read_count way of doing things and added a
resource provider test. It looks much simpler and cleaner to me.

https://codereview.chromium.org/208213003/diff/130001/gpu/command_buffer/clie...
File gpu/command_buffer/client/context_support.h (right):

https://codereview.chromium.org/208213003/diff/130001/gpu/command_buffer/clie...
gpu/command_buffer/client/context_support.h:41: unsigned plane_transform,
Is //src/gpu/command_buffer/common/constants.h a reasonable place to move the
enum into for sharing it here?

[piman, 2014-03-27 00:12:36]

https://codereview.chromium.org/208213003/diff/150001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/208213003/diff/150001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1458:
child_info.in_use_resources.insert(local_id);
I'm not sure if it's the best way to do this, in particular it means we have to
wait one more frame before returning the resources after the swap (since we have
to wait for the next DeclareUsedResourcesFromChild, on the next commit, and that
might even be skipped if the layer didn't change).

What I had in mind rather, is to follow the places where we test exported_count,
and take lock_for_read_count into account there too, where it makes sense.
Specifically, DeleteAndReturnUnusedResourcesToChild would mark for deletion
(instead of deleting immediately), and conversely, UnlockForRead would call
DeleteResourceInternal or DeleteAndReturnUnusedResourcesToChild if the resource
was marked_for_deletion and the lock_for_read_count dropped to 0.

Possibly doing the same for DeleteResource, if we ever want to handle the case
where the overlay candidate comes directly from the UI compositor.

[alexst (slow to review), 2014-03-27 14:58:17]

Okay, this is deferring return until read lock is released.

PTAL, and thank for your patience and detailed comments, resource provider does
a lot of fancy footwork, so I appreciate your help.

[piman, 2014-03-27 17:53:26]

Cool, LGTM!

[alexst (slow to review), 2014-03-27 23:39:52]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:41:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/208213003/190001

[alexst (slow to review), 2014-03-28 16:39:44]

The CQ bit was unchecked by alexst@chromium.org

[alexst (slow to review), 2014-03-28 17:00:33]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:02:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/208213003/190001

[commit-bot: I haz the power, 2014-03-28 20:49:11]

Change committed as 260267