Allow CopyOutputRequests on all render passes of a surface

Allow CopyOutputRequests on all render passes of a surface

The browser compositor in content/ currently expects to be able to do copy output requests of arbitrary layers. The surface aggregator should allow ever render pass to contain copy requests, and move them to the aggregated frame.

BUG=397730
Committed: https://crrev.com/5e50dfa786102701fd831bd155f5dc1ee224458f
Cr-Commit-Position: refs/heads/master@{#299831}

[jbauman, 2014-09-30 22:31:18]

jbauman@chromium.org changed reviewers:
+ jamesr@chromium.org

[jbauman, 2014-09-30 22:31:19]

This is ugly, but seems to be the only way to get content/ to work without
creating a bunch of extra compositors and surfaces for screenshotting parts of
the widget hierarchy.

[jamesr, 2014-10-03 00:09:57]

Can't you just make an extra surface in content/ for this case?  Injecting an
additional surface is super easy.  I'd rather not special case cc/surfaces/ just
for this odd case.

[jbauman, 2014-10-03 00:21:39]

On 2014/10/03 00:09:57, jamesr wrote:
> Can't you just make an extra surface in content/ for this case?  Injecting an
> additional surface is super easy.  I'd rather not special case cc/surfaces/
just
> for this odd case.

It's possible, but for cases like DesktopMediaListAsh::EnumerateWindowsForRoot
it'd have to create a new ui::Compositor, new Surface, then rerasterize all the
contents, upload them, etc, for a single frame to grab the contents, which
really sucks.

I've modified this patch so it now just generalizes the original
CopyOutputRequest per-surface logic. Now CopyOutputRequests can hang off of any
render pass, not just the root render pass for a surface. RequestCopyOfOutput
only can handle CopyOutputRequests for the root renderpass at the moment, but
other copy requests can be specified in SubmitFrame, and they'll just work.

[jamesr, 2014-10-03 00:24:01]

On 2014/10/03 00:21:39, jbauman wrote:
> On 2014/10/03 00:09:57, jamesr wrote:
> > Can't you just make an extra surface in content/ for this case?  Injecting
an
> > additional surface is super easy.  I'd rather not special case cc/surfaces/
> just
> > for this odd case.
> 
> It's possible, but for cases like DesktopMediaListAsh::EnumerateWindowsForRoot
> it'd have to create a new ui::Compositor, new Surface, then rerasterize all
the
> contents, upload them, etc, for a single frame to grab the contents, which
> really sucks.

Why?  You shouldn't need any of that stuff to make a new surface.  You don't
even need a GL context to exist to submit a surface that references another
surface.  All you need is to know the id you want to reference and to set the
transform correctly.

[jamesr, 2014-10-03 00:29:38]

On 2014/10/03 00:21:39, jbauman wrote:

> I've modified this patch so it now just generalizes the original
> CopyOutputRequest per-surface logic. Now CopyOutputRequests can hang off of
any
> render pass, not just the root render pass for a surface. RequestCopyOfOutput
> only can handle CopyOutputRequests for the root renderpass at the moment, but
> other copy requests can be specified in SubmitFrame, and they'll just work.

This sounds pretty reasonable, thanks for helping me understand.  This seems
like a nice generalization.

I'll review this patch again Real Soon Now (TM).

[jbauman, 2014-10-14 02:17:22]

ping

[jamesr, 2014-10-15 03:21:39]

sorry, slipped off my radar

https://codereview.chromium.org/614073005/diff/40001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/614073005/diff/40001/cc/surfaces/surface.h#ne...
cc/surfaces/surface.h:45: ScopedPtrVector<ScopedPtrVector<CopyOutputRequest>>*
copy_requests);
why's this a pointer to a vector of pointers of vectors of pointers?

if this is intended to be sparse, instead of passing piles of NULLs can we pass
a list of tuples of CopyOutputRequest, pass id, quad ids?  there should be 0 or
1 entry in this in the average case

[jbauman, 2014-10-15 23:40:08]

On 2014/10/15 03:21:39, jamesr wrote:
> sorry, slipped off my radar
> 
> https://codereview.chromium.org/614073005/diff/40001/cc/surfaces/surface.h
> File cc/surfaces/surface.h (right):
> 
>
https://codereview.chromium.org/614073005/diff/40001/cc/surfaces/surface.h#ne...
> cc/surfaces/surface.h:45: ScopedPtrVector<ScopedPtrVector<CopyOutputRequest>>*
> copy_requests);
> why's this a pointer to a vector of pointers of vectors of pointers?
> 
> if this is intended to be sparse, instead of passing piles of NULLs can we
pass
> a list of tuples of CopyOutputRequest, pass id, quad ids?  there should be 0
or
> 1 entry in this in the average case

Ok, switched it to a multimap mapping from the pass id to the
CopyOutputRequest*.

[jamesr, 2014-10-15 23:51:24]

https://codereview.chromium.org/614073005/diff/60001/cc/surfaces/surface_aggr...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/614073005/diff/60001/cc/surfaces/surface_aggr...
cc/surfaces/surface_aggregator.cc:219: MoveMatchingRequests(source.id,
&copy_requests, &copy_pass->copy_requests);
the RenderPassIds in copy_requests will be in the surface's ID namespace, right?
 Don't we have to map them into the aggregated namespace (i.e. call RemapPassId)
in order for them to refer to aggregated pass IDs?

[jbauman, 2014-10-16 00:02:06]

On 2014/10/15 23:51:24, jamesr wrote:
>
https://codereview.chromium.org/614073005/diff/60001/cc/surfaces/surface_aggr...
> File cc/surfaces/surface_aggregator.cc (right):
> 
>
https://codereview.chromium.org/614073005/diff/60001/cc/surfaces/surface_aggr...
> cc/surfaces/surface_aggregator.cc:219: MoveMatchingRequests(source.id,
> &copy_requests, &copy_pass->copy_requests);
> the RenderPassIds in copy_requests will be in the surface's ID namespace,
right?
>  Don't we have to map them into the aggregated namespace (i.e. call
RemapPassId)
> in order for them to refer to aggregated pass IDs?

The input to MoveMatchingRequests are both in the surface's ID namespace, and
the output doesn't include a RenderPassId at all, so no remapping is necessary.

[jamesr, 2014-10-16 00:04:42]

Right, of course.  They're copied to the actual pass.

lgtm

[jbauman, 2014-10-16 00:19:30]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-10-16 00:21:45]

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

[commit-bot: I haz the power, 2014-10-16 03:21:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-16 03:22:24]

Patchset 4 (id:??) landed as
https://crrev.com/5e50dfa786102701fd831bd155f5dc1ee224458f
Cr-Commit-Position: refs/heads/master@{#299831}