Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 8 years ago

Trying to do a snapshot from an e10s mochitest (e.g. by calling SpeicalPowers.snapshotWindowWithOptions) and including the DRAWWINDOW_DRAW_VIEW | DRAWWINDOW_USE_WIDGET_LAYERS | DRAWWINDOW_DRAW_CARET flags doesn't actually work using the widget layers. Instead it bails out and uses a BasicLayerManager in the content process. While this is ok for many things, it doesn't capture APZ transforms which I would like to do.

I have patches that make this work, will put them up in a bit.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 1 • 8 years ago

The IPDL code unconditionally calls forget() on any Shmem instances that are
sent over the IPC channel. This means that if the child process has a
SurfaceDescriptor containing a Shmem (such as a shmem-type SurfaceDescriptorBuffer)
then the shmem object in it will be zeroed out after sending it over IPC. In
order to still have access to the underlying SharedMemory, we need to make a
copy of the shmem or SurfaceDescriptor before doing the IPC call. Note that this
is true for safe and unsafe shmems.

Review commit: https://reviewboard.mozilla.org/r/53958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53958/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 2 • 8 years ago

This patch moves the snapshot ipc code from PCompositorBridge to PLayerTransaction,
so that the parent side receiving the request can grab the correct
PCompositorBridgeParent instance. Otherwise in the case of cross-process
communication the request arrives in the CrossProcessCompositorBridgeParent,
from which is it is not easy to get a hold of the corresponding CompositorBridgeParent.

That, combined with removing various XRE_IsParentProcess checks, seems to be
sufficient to make snapshotting using widget layers work in e10s child processes.

Review commit: https://reviewboard.mozilla.org/r/53960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53960/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 3 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/53962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53962/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 4 • 8 years ago

The try push I triggered from MozReview burned because stuff changed on m-c so that I needed to #include CompositorBridgeParent.h into LayerTransactionParent.cpp. This try push includes that change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b60f8209148

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 5 • 8 years ago

Well. I guess that broke reftests pretty bad.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 6 • 8 years ago

Comment on attachment 8754413 [details]
MozReview Request: Bug 1274284 - Make a copy of the SurfaceDescriptor before sending it to IPC. r?nical

Dropping review while I figure out what happened.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 7 • 8 years ago

Turns out that (a) compositor IDs are different from layers ids, and (b) a layers id of 0 is special. Fixed up locally, try push at

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c15014527bce

	Nicolas Silva [:nical]
	Comment 8 • 8 years ago

Comment on attachment 8754413 [details]
MozReview Request: Bug 1274284 - Make a copy of the SurfaceDescriptor before sending it to IPC. r?nical

https://reviewboard.mozilla.org/r/53958/#review50756

	Nicolas Silva [:nical]
	Comment 9 • 8 years ago

Comment on attachment 8754414 [details]
MozReview Request: Bug 1274284 - Make snapshotting using widget layers work in the child process. r?nical,mattwoodrow

https://reviewboard.mozilla.org/r/53960/#review50758

	Matt Woodrow (:mattwoodrow)
	Comment 10 • 8 years ago

What do you need this for?

The main problem with this is that it'll capture any content from the parent process that overlays the content process area.

This shouldn't matter for reftests, but there's definitely some addons using this function that might be affected.

Ideally we'd set enable an intermediate surface for the RefLayer, composite, and then read back just the intermediate.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 11 • 8 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> What do you need this for?

What I want to do is get a snapshot of the content process area, including APZ transforms, from a mochitest. Comment 0 has a bit more context.

> The main problem with this is that it'll capture any content from the parent
> process that overlays the content process area.

True, I hadn't thought of that. What happens in the non-e10s (but still OMTC) case? I thought we still used a ClientLayerManager in that case, and so that would have had the same problem. I didn't test it specifically though.

> This shouldn't matter for reftests, but there's definitely some addons using
> this function that might be affected.
> 
> Ideally we'd set enable an intermediate surface for the RefLayer, composite,
> and then read back just the intermediate.

That sounds reasonable. Is it as easy as you make it sound? :)

I see ForceIntermediateSurface() functions on BasicContainerLayer and ClientContainerLayer - would I have to add a similar one to ContainerLayerComposite, use it, and then copy the contents of the surface back into the snapshot surface?

	Matt Woodrow (:mattwoodrow)
	Comment 12 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
 
> True, I hadn't thought of that. What happens in the non-e10s (but still
> OMTC) case? I thought we still used a ClientLayerManager in that case, and
> so that would have had the same problem. I didn't test it specifically
> though.

You only get a compositor snapshot if you drawWindow for the root (chrome) window, so it'll include all the chrome content. If you try snapshot just the child document, then it'll be BasicLayers.


> That sounds reasonable. Is it as easy as you make it sound? :)

Probably not, unfortunately.

> 
> I see ForceIntermediateSurface() functions on BasicContainerLayer and
> ClientContainerLayer - would I have to add a similar one to
> ContainerLayerComposite, use it, and then copy the contents of the surface
> back into the snapshot surface?

Basically, yeah. You may also need to guarantee that we retain the intermediate surface so that it's still around when you want to do the readback.

The hard part is probably ensuring that we have code to copy from an intermediate surface into the snapshot surface. We probably don't have code for all the different types currently.

We have CompositorOGL::CopyToTarget and equivalents in other compositor backends. We'd probably need to refactor that to allow readback of an intermediate surface buffer.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 13 • 8 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Basically, yeah. You may also need to guarantee that we retain the
> intermediate surface so that it's still around when you want to do the
> readback.
> 
> The hard part is probably ensuring that we have code to copy from an
> intermediate surface into the snapshot surface. We probably don't have code
> for all the different types currently.
> 
> We have CompositorOGL::CopyToTarget and equivalents in other compositor
> backends. We'd probably need to refactor that to allow readback of an
> intermediate surface buffer.

Hm, I don't know if I care enough to spend the time to make this work. I've already spent two days more on this than I intended to. I can probably rewrite my test to take a parent-process snapshot instead. (Although that was broken too when I tried, so I might have a different set of patches to fix that up, but at least it will be in code that I have paged in right now).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 14 • 8 years ago

Yeah I got my test working with parent-process snapshotting. Unassigning since I won't be spending more time on this (until the next time I need it, anyway). Feel free to steal.

The footgun that part 1 fixes might be good to land regardless, perhaps as a separate bug.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 15 • 8 years ago

Comment on attachment 8754413 [details]
MozReview Request: Bug 1274284 - Make a copy of the SurfaceDescriptor before sending it to IPC. r?nical

I landed the part 1 patch in bug 1280998.