1173286 - Emulate a simple version of the page transition api

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The navigation transition API work is under discussions. It will takes a bit of time to get a full spec and the start of an implementation.

In the meantime it would be useful to be able to navigate into a multi-documents app without the need for a content wrapper.

This little set of patches emulate that by making a screenshot (as fast as as possible by reading the content of the OGL compositor) of the current panel, before navigating to the next/previous panel.

A little glue in the system app animates all that.

I will need help to make this set of patches correct.
Also I have taken in account the argumentthat this work is only temporary and will be throw away once we have the real API.
The way I have taken it into account is by trying to improve some of the APIs we have in order to make them useful for other purposes. So the only part that will should be removed once the real API is ready is the part living in the system app.

Part 1 - Expose an API to pause/resume the rendering of the targetted app
  This could be useful for other purposes, such as edge gestures, or revealing the utility tray by reducing the amount of work on the child process, and giving a bit more CPU to the current task.

Part 2 - Get a screenshot directly from the compositor.
  Currently our mozbrowser screenshot API send a message to the child process, which itself tries to do a full paint before serializing the rendered content and forwarding it via IPC to the parent.

 It should be way faster to get the rendering from the compositor directly and avoid IPC calls (and also make sure to not wait on the child process that may be busy at that point).

Part 3 - Use MozAfterRemotePaint on the parent process instead of MozAfterPaint in the child process.

 As of today the waitForNextPaint method will attach a message to the child process waiting for it to repaint and will forward this call to the parent process once it is ready.

 It add an additional IPC calls but the biggest issue is that the signal in the child process does not always correspond to the real painting in the parent process. Using MozAfterRemotePaint should be much more reliable.


This is the set of features I would like to see fix with this patch queue. For the moment this is a little fuzzy in the set of patch and having some help to clean up all those things and make them real patches will be highly appreciated!

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot.compositor.patch

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: add.pause.resume.methods.on.mozbrowser.patch

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: encode.surface.to.membuffer.patch

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: expose.method.copy.to.target.compositor.ogl.patch

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: gaia.transition.poc.patch

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I'm trying to make the CanvasRenderingContext2D::DrawWindow use compositor.
In card-view case, content process will ignore DRAWWINDOW_USE_WIDGET_LAYERS flag and go through BasicLayerManager for drawing. I'm still reading the code flow for DRAWWINDOW_USE_WIDGET_LAYERS flag checking. With this, we can still re-use the DrawWindow() method without the new api.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #6)
> I'm trying to make the CanvasRenderingContext2D::DrawWindow use compositor.
> In card-view case, content process will ignore DRAWWINDOW_USE_WIDGET_LAYERS
> flag and go through BasicLayerManager for drawing. I'm still reading the
> code flow for DRAWWINDOW_USE_WIDGET_LAYERS flag checking. With this, we can
> still re-use the DrawWindow() method without the new api.

Does the CanvasRenderingContext2D::DrawWindow call will be called on the child process side ?

If so, I think the new API will be faster because it does not need to exchange IPC messages between the parent process and the child process, and also avoid to dispatch a message on the event loop of the child process (which may be busy and takes a while to answer).

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] snapshot using CompositorParent.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

CopyToTarget()[1] costs a lot of time in CompositorOGL::EndFrame(). Even though we use compositor for snapshot, the BasicLayerManager(draw snapshot in content side) still wins in my test. My test is pressing the home button to get card-view for "Message" app. I need to check my implementation again.

Time:
Compositor       BasicLayerManager
131.539 ms       81.753 ms

[1]
https://hg.mozilla.org/mozilla-central/annotate/aad95360a002/gfx/layers/opengl/CompositorOGL.cpp#l1365

Attachment 8617803 [details] [diff] also calls CopyToTarget(). I think it will also hit the same situation.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

For b2g, I'm trying to pass a GrallocBuffer to CompositorParent and set the render target to this buffer. Hope we can reduce the copy time.

Comment 11

[Chris Lord [:cwiiis]]

Just for reference, the latest revision of the page transition spec lives here: http://cwiiis.github.io/gaia-navigator/
Similarly, there's a JS shim implementation in the corresponding repository: https://github.com/Cwiiis/gaia-navigator/ (which really lives here: https://gitlab.com/Cwiiis/gaia-navigator) - the shim is mostly up-to-date with the spec, but has a few bugs/limitations/missing features.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Roc, my understanding is that the screenshot from compositor part of this patch may be useful for e10s Firefox as well. Notably from the slide to navigate into history for Mac.

Do you have an opinion / some suggestions on how to implement it ?

The needs:
 - Get a snapshot of the content process, and make it available to the system app (parent process) so it can be manipulated. So far the only manipulation is to scale it via css for display purposes.

So far the 2 approaches are:
 - take a snapshot of the compositor from the parent process by exposing a (theorical) fast path.
 - take a snapshot of the compositor from the child process by optimizing canvas.drawWindow.

My assumption is that doing main-thread-to-compositor-thread communication will be faster and more predictable than having to exchange ipc messages between the parent and the child process, hoping the child process is not busy and returning a serialized blob over ipc.

Jerry's number seems to prove the contrary, which is surprising to me !

Jerry may also explain things more precisely than me about why it is faster :)

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I have obviously forgotten to needinfo roc...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My proposal here https://wiki.mozilla.org/User:Roc/ScreenCaptureAPI was to take the screenshot during the next composite. That's because for many use-cases (e.g. FxOS app screenshots, Mac sliding effects) minimum overhead is more important than low latency, and I'm pretty sure just reading back the results of rendering that we probably already had to do is less work than drawing the content all over again with the CPU. In particular, it should leave the CPU free for other tasks. But I believe Jerry that it could be lower latency to draw everything again with the CPU.

If we can't have an implementation that's optimal for both latency and overhead, then perhaps we need a different API for each case (or a single API with a flag).

nical and Jeff are the authorities on the compositor. They may be able to tell us more about how to minimize overhead of readback.

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> If we can't have an implementation that's optimal for both latency and
> overhead, then perhaps we need a different API for each case (or a single
> API with a flag).
> 
> nical and Jeff are the authorities on the compositor. They may be able to
> tell us more about how to minimize overhead of readback.

What I do in attachment 8627632 [details] [diff] [review] is that I send the content process' root layer to b2g and composite the layer tree again at CompositorParent. It's hard to just read the final result for a content at Compositor. Maybe it has alpha blending at some layer. Doing the composition at compositor is faster, but CopyToTarget() in ComposierOGL::CopyToTarget() is slow when we copy the framebuffer into our target surface. In B2G platform, maybe we can use GraphicBuffer to reduce the copy time. I'm trying to do that.

Comment 16

[Nicolas Silva [:nical]]

ContainerLayer has code to render its subtree in an intermediate surface. I was hoping that we could have a simple ipdl message that says "snapshot this subtree" pointing to a Layer, and have the compositor just render it to a texture. then we would wrap the rendered texture in a special RemoteTextureClient/Host pair which only has access to the underlying texture on the host side, and only do an asynchronous read-back to get the pixels to the client if the client side requests it. This way we could avoid the read-back in cases like the app manager in b2g, where the content thread only needs to say "snapshot this sub-tree and render it there" without needing to read the pixels. Then if at some point we want to store a thumbnail on disk or something, we ask for a read-back and get it asynchronously. On b2g we could render in a gralloc buffer, but for other platforms we should strive to avoid doing read-back when we don't need it. I also don't think we should re-create the layer tree the way we do it with drawWindow (as far as I know).

The plumbing to ask the compositor to render to a subtree is not very hard to do. I don't know about the glue to expose that to JS (mapping an API that captrues part of the DOM to something that captures part of the layer tree, and then how to expose the screenshot layer to the DOM...), but the layer side of things would not be insane within our architecture.

Comment 17

[Nicolas Silva [:nical]]

About your proposal, roc, I would propose to split the API in two like:

Promise createScreenshot() -- returns a promise of a ScreenshotImageThingy object (no read-back, but can be placed in the DOM and seen on screen)

Promise ScreenshotImageThingy.getImageBitmap() -- returns a promise of a ImageBitmap (does the readback)


And maybe implement everything in createScreenshot as a first implementation but leaving the door open for the API to be used without read-backs when not needed.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nicolas Silva [:nical] from comment #16)
> Then if at some point we want to store a
> thumbnail on disk or something, we ask for a read-back and get it
> asynchronously. On b2g we could render in a gralloc buffer, but for other
> platforms we should strive to avoid doing read-back when we don't need it.

Yeah. The best path here is to make ImageBitmap support layers::Image --- which is happening here: https://bugzilla.mozilla.org/show_bug.cgi?id=1044102
Then our screenshot API can return an ImageBItmap backed by an Image which is holding GPU memory. Then if you just need to render the image somewhere in an <img>, that should work without readback, and we can create other non-readback paths as necessary (e.g. to draw Image-backed ImageBitmaps to WebGL or GPU-accelerated 2D canvas without readback).

> The plumbing to ask the compositor to render to a subtree is not very hard
> to do. I don't know about the glue to expose that to JS (mapping an API that
> captrues part of the DOM to something that captures part of the layer tree,
> and then how to expose the screenshot layer to the DOM...), but the layer
> side of things would not be insane within our architecture.

Yes, although we need to do work in layout to ensure the correct layer is created for the document even if it normally wouldn't be because it's not visible. Also, we need work in the compositor to ensure that layer is composited to a surface of the correct size even if it normally wouldn't be because it's not visible.

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] snapshot using CompositorParent. v2

This version use android graphicBuffer to reduce memory copy. CompositorParent receives a graphicBuffer and uses it as the render target.
In my test with Contact app(200 contaces), the total time of snapshot is much similer. The BasicLayerManager is still win. I will keep working to improve it.
This wip does't handle the different format of CompositorOGL and cairo surface, so we can see the y-flip and RB-swap problem.

Time:
Compositor    BasicLayerManager
8X.XX ms       8X.XX ms

Comment 20

[Nicolas Silva [:nical]]

The reason Vivien's patch is slow is because he hacked it up using CopyToTarget which causes a readback, rather than rendering the screenshot directly into a gralloc buffer. The compositor approach is strictly better if done right, because the only work required is to do a composition of already rendered content on the gpu, whereas re-rendering everything on the content side with the cpu is a lot more work (and happens on a thread which already has many bottlenecks, especially since the cpu and the main thread are usually very busy around page loads).
Implementing this on the compositor side also lets us have an API that can expose screenshots to the content process without providing access to the pixels (while also being able to provide access to said pixels if we choose to), which is a good security feature and would make it easier for us to expose the api to non-priviledge web content eventually.
In my opinion it would be best to invest in improving the compositor-based screenshot implementation, otherwise I have the feeling we'll eventually end up re-doing this work on the compositor side when we reach the limits of the BasicLayermanager approach.

Also, we eventually want to get rid of the maintenance burden of BasicLayerManager. It's a lot of old code that we don't use much anymore and has better alternatives in most cases. It's not clear that we'll be able to remvoe it soon, but I would rather avoid attaching more dependencies to it if it can be avoided.

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] snapshot using CompositorParent. v3

remove useless code path.

People can use this patch to test snapshot using CompositorParent(long press the home-button to see the card-veiw). I haven't handled the y-flip and r-b color channel swap stuff. That will be at the next patch.

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] snapshot using CompositorParent. v4

Handle y-flip and rb color channel swap for cairo backend.

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I got a weird size when I capture the card-view for browser app.

The (x,y,w,h) for DrawWindow is:
CanvasRenderingContext2D::DrawWindow(0.000000,0.000000,980.000000,1524.000000)
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/dom/canvas/CanvasRenderingContext2D.cpp#l4637

But the compositor receives a big size as:
CompositorParent::RecvMakeSnapshot(0,0,2940,4572)
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/gfx/layers/ipc/CompositorParent.cpp#l801

The size is so big and b2g can't create this big gralloc buffer. Then we fal lback to the slower path.

Comment 24

[Chris Lord [:cwiiis]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #23)
> I got a weird size when I capture the card-view for browser app.
> 
> The (x,y,w,h) for DrawWindow is:
> CanvasRenderingContext2D::DrawWindow(0.000000,0.000000,980.000000,1524.
> 000000)
> https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/dom/canvas/
> CanvasRenderingContext2D.cpp#l4637
> 
> But the compositor receives a big size as:
> CompositorParent::RecvMakeSnapshot(0,0,2940,4572)
> https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/gfx/layers/ipc/
> CompositorParent.cpp#l801
> 
> The size is so big and b2g can't create this big gralloc buffer. Then we fal
> lback to the slower path.

This size is exactly 3x the size listed above on each axis... Could it be either not taking dpi into account, or possibly the displayport size instead of the viewport size?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The parameters to DrawWindow should be in CSS pixels. I think your test code is passing in a size in device pixels.

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: Draw window with DRAW_CARET flag. v1

With this flag, we might be able to use WIDGET_LAYER to get snapshot.

Comment 27

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: Create a pref "layer.content.widget-layer" for content WIDGET_LAYER usage. v1

Comment 28

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Create a gl render target from TextureHost. v1

Comment 29

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: Update MakeSnapshot() ipc call. v1

Comment 30

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: Use compositor for snapshot. v1

Comment 31

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Handle y-flip and rb color channel swap for compositor snapshot. v1

Comment 32

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: Use compositor for snapshot. v2

update for EndEmptyTransaction() crash.

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Handle y-flip and rb color channel swap for compositor snapshot. v2

fix the wrong gl scissor test

Comment 34

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: Use compositor for snapshot. v2

fix the crash when we create cairo surface at parent side.

Comment 35

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Handle y-flip and rb color channel swap for compositor snapshot. v3

Fix clip region for non-y-flip case.

Comment 36

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8635293 [details] [diff] [review]
P5: Use compositor for snapshot. v2

Review of attachment 8635293 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/ClientLayerManager.cpp
@@ +483,5 @@
>        // The compositor doesn't draw to a different sized surface
>        // when there's a rotation. Instead we rotate the result
>        // when drawing into dt
>        IntRect outerBounds;
>        mWidget->GetBounds(outerBounds);

For browser app, I got a big bound from mWidget->GetBounds()[1].
e.g. bound(x,y,w,h)=(0,0,2940,4572)

But we still only have the layer size (w,h)=(1080*1920). It already contains the whole web page.

What's the unit of GetBounds()? Since the bound size is too big, we can't create this big GrallocBuffer at b2g. Then we go though the slower path.
I'm also trying to use the root's layer bound instead of the widget bound, but the root's bound (x,y,w,h)=(0,0,0,0) here.
e.g. call ClientLayerManager::GetRoot()->GetLayerBounds()

I would like to get the suitable size to create the drawTarget, not a big size. I don't know where is the factor from (2940,4572) to (1080,1920)

Right now, most of app's card-view and combinational-key snapshot could go though compositor-snapshot path.

Especially for combinational-key snapshot, it's much faster than the original one(if we don't need to compile the shader code for r-b channel swap).

[1]
https://dxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#1309

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #36)
> ::: gfx/layers/client/ClientLayerManager.cpp
> @@ +483,5 @@
> >        // The compositor doesn't draw to a different sized surface
> >        // when there's a rotation. Instead we rotate the result
> >        // when drawing into dt
> >        IntRect outerBounds;
> >        mWidget->GetBounds(outerBounds);
> 
> For browser app, I got a big bound from mWidget->GetBounds()[1].
> e.g. bound(x,y,w,h)=(0,0,2940,4572)
> 
> But we still only have the layer size (w,h)=(1080*1920). It already contains
> the whole web page.

I don't know why the widget bounds have that big value. They should be in device pixels. Is it possible that the <browser> element extends off the screen?

> What's the unit of GetBounds()? Since the bound size is too big, we can't
> create this big GrallocBuffer at b2g. Then we go though the slower path.
> I'm also trying to use the root's layer bound instead of the widget bound,
> but the root's bound (x,y,w,h)=(0,0,0,0) here.
> e.g. call ClientLayerManager::GetRoot()->GetLayerBounds()

Don't use LayerBounds for anything.

> Right now, most of app's card-view and combinational-key snapshot could go
> though compositor-snapshot path.

What snapshots can't go through the compositor-snapshot path, and why?

> Especially for combinational-key snapshot, it's much faster than the
> original one(if we don't need to compile the shader code for r-b channel
> swap).

I'm not sure what you mean here, can you clarify?

Comment 38

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #36)
> > ::: gfx/layers/client/ClientLayerManager.cpp
> > @@ +483,5 @@
> > >        // The compositor doesn't draw to a different sized surface
> > >        // when there's a rotation. Instead we rotate the result
> > >        // when drawing into dt
> > >        IntRect outerBounds;
> > >        mWidget->GetBounds(outerBounds);
> > 
> > For browser app, I got a big bound from mWidget->GetBounds()[1].
> > e.g. bound(x,y,w,h)=(0,0,2940,4572)
> > 
> > But we still only have the layer size (w,h)=(1080*1920). It already contains
> > the whole web page.
> 
> I don't know why the widget bounds have that big value. They should be in
> device pixels. Is it possible that the <browser> element extends off the
> screen?
> 

It's my fault. The outerBounds is (0,222,1080,1680), and the big area is due to the mShadowTarget->GetClipExtents()[1]. I will check the target's transform matrix later.

[1]
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/gfx/layers/client/ClientLayerManager.cpp#l488

> > What's the unit of GetBounds()? Since the bound size is too big, we can't
> > create this big GrallocBuffer at b2g. Then we go though the slower path.
> > I'm also trying to use the root's layer bound instead of the widget bound,
> > but the root's bound (x,y,w,h)=(0,0,0,0) here.
> > e.g. call ClientLayerManager::GetRoot()->GetLayerBounds()
> 
> Don't use LayerBounds for anything.
> 
> > Right now, most of app's card-view and combinational-key snapshot could go
> > though compositor-snapshot path.
> 
> What snapshots can't go through the compositor-snapshot path, and why?

I mean we can go through the faster compositor-snapshot path.
If we have a big rect as (0,0,2940,4572), we can't create this big gralloc buffer at b2g. Instead, we create a shmem buffer for this big area. With gralloc buffer, we can create a CompositingRenderTarget from it and compose all layers into the target directly. With shmem, the compositor need to do the composition to the original draw target, and then use glReadPixels() to copy the data into shmem. This path will be slower than our original BasicLayerManager path.

> 
> > Especially for combinational-key snapshot, it's much faster than the
> > original one(if we don't need to compile the shader code for r-b channel
> > swap).
> 
> I'm not sure what you mean here, can you clarify?

If we want to use compositor-snapshot path, we can handle the r-b swap and y-flip at compositor. Thus, we don't need to convert the result to cairo format at content side(cairo needs r-b swap, and the OGL's coordinate is upside down. Doing the conversion at cpu is pretty slow. I write a new OGL shader code to convert that. Since this is a special shader, we don't use that before as other normal shader. Our shader-cache system will have cache miss at the first time, and then we spend about 20ms for the shader code compiling. So the first time which we use snapshot is slower. After that, it faster.

Comment 39

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I'm considering using PresShellResolution() for the final rect size(). With that, I think I can get the right layer size from [1]. Patch will be uploaded tomorrow. 

[1]
https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/gfx/layers/client/ClientLayerManager.cpp#l488

Comment 40

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: Use compositor for snapshot. v3

fix changing renderTarget error.

Comment 41

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Handle y-flip and rb color channel swap for compositor snapshot. v4

update rb-swap and y-flip renderTarget comment.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7: Handle pan/zoom factor. v1

Handle pan/zoom factor for browser app.

Comment 43

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8633942 [details] [diff] [review]
P1: Draw window with DRAW_CARET flag. v1

If we have caret in page, we need to update the layer data at compositor if we don't have DRAWWINDOW_DRAW_CARET flag. With this, we might still see the caret in snapshot.

Comment 44

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8633943 [details] [diff] [review]
P2: Create a pref "layer.content.widget-layer" for content WIDGET_LAYER usage. v1

Review of attachment 8633943 [details] [diff] [review]:
-----------------------------------------------------------------

This patch would like to preserve the PAINT_WIDGET_LAYERS flag in content process, and then we can use compositor for snapshot.

::: gfx/layers/client/ClientLayerManager.cpp
@@ +221,5 @@
>  #endif
>  
>    // If we have a non-default target, we need to let our shadow manager draw
>    // to it. This will happen at the end of the transaction.
> +  if (aTarget && gfxPrefs::UseWidgetLayerAtContent()) {

Setup the "mShadowTarget", then we can use it for compositor snapshot at [1]

[1]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/client/ClientLayerManager.cpp#l476

::: layout/base/nsPresShell.cpp
@@ +4671,5 @@
>      nsView* view = rootFrame->GetView();
>      if (view && view->GetWidget() &&
>          nsLayoutUtils::GetDisplayRootFrame(rootFrame) == rootFrame) {
>        LayerManager* layerManager = view->GetWidget()->GetLayerManager();
> +      if (gfxPrefs::UseWidgetLayerAtContent() && layerManager && layerManager->AsClientLayerManager()) {

If it has "clientLayerManager", then we can use chrome's compositor for snapshot. If it only has basicLayerManager, skip the PAINT_WIDGET_LAYERS flag.

@@ -4676,5 @@
> -      // ClientLayerManagers in content processes don't support
> -      // taking snapshots.
> -      if (layerManager &&
> -          (!layerManager->AsClientLayerManager() ||
> -           XRE_IsParentProcess())) {

I'm not sure why we remove the PAINT_WIDGET_LAYERS flag at [1].
Does it fix reftest failed or it fix other problem?

And if we use basicLayerManager, do we always have PAINT_WIDGET_LAYERS? I don't think basicLayerManager has different handling between PAINT_WIDGET_LAYERS flag and normal flag.

[1]
https://bugzilla.mozilla.org/show_bug.cgi?id=1083635#c8

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8633944 [details] [diff] [review]
P3: Create a gl render target from TextureHost. v1

Review of attachment 8633944 [details] [diff] [review]:
-----------------------------------------------------------------

This patch will try to create a CompositingRenderTargetOGL from a "TextureHost". Then compositor can put all layer into this textureHost directly.

At B2G, if we pass a GrallocTextureClientOGL to parent process and use it as a renderTarget, content side could get the final result without glReadPixel() cost. We can see the glReadPixel() and copy operation at [1].

The r-b color channel and y-flip conversion are at another patch for content cairo backend.

[1]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/opengl/CompositorOGL.cpp#l1366

::: gfx/layers/Compositor.h
@@ -188,5 @@
> -    , mDiagnosticTypes(DiagnosticTypes::NO_DIAGNOSTIC)
> -    , mParent(aParent)
> -    , mScreenRotation(ROTATION_0)
> -  {
> -  }

Move ctor and dtor impl into cpp file. Since we have inline function here, some function of "RefPtr<CompositingRenderTarget> mCompositingRenderTarget" will be put here. Then we need to include the header.

@@ +218,5 @@
>     * If this method is not used, or we pass in nullptr, we target the compositor's
>     * usual swap chain and render to the screen.
>     */
> +  void SetTarget(gfx::DrawTarget* aTarget, const gfx::IntRect& aRect);
> +  void SetTarget(CompositingRenderTarget* aSurface, const gfx::IntRect& aRect);

Set the new target with CompositingRenderTarget.

@@ +220,5 @@
>     */
> +  void SetTarget(gfx::DrawTarget* aTarget, const gfx::IntRect& aRect);
> +  void SetTarget(CompositingRenderTarget* aSurface, const gfx::IntRect& aRect);
> +  bool HasTarget() const;
> +  void ClearTarget();

It's the same reason as ctor/dtor.

::: gfx/layers/composite/LayerManagerComposite.h
@@ +268,5 @@
>      // because that's what they mean.
>      // Also when we're not drawing to the screen, DidComposite will not be
>      // called to extract and send these notifications, so they might linger
>      // and contain stale ImageContainerParent pointers.
> +    if (!mCompositor->HasTarget()) {

If we have CompositingRenderTarget or gfx::DrawTarget, we don't need DidComposite() notification.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +41,5 @@
>  #include "ScopedGLHelpers.h"
>  #include "GLReadTexImageHelper.h"
>  #include "GLBlitTextureImageHelper.h"
>  #include "HeapCopyOfStackArray.h"
> +#include "EGLImageHelpers.h"

I will remove this header at next round. We don't need eglimage here.

@@ +543,5 @@
> +
> +  if (!aTextureSource.get()) {
> +    return nullptr;
> +  }
> +  GLTextureSource* glSource = aTextureSource->AsSourceOGL()->AsGLTextureSource();

Should we need to check the return value for AsSourceOGL() and AsGLTextureSource()?

Comment 46

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8633945 [details] [diff] [review]
P4: Update MakeSnapshot() ipc call. v1

Review of attachment 8633945 [details] [diff] [review]:
-----------------------------------------------------------------

Add a new parameter "PLayer" actor in SendMakeSnapshot(). Then, CompositorParent could use this layer as the layer root, and do the composition for this sub layer tree to get the snapshot result.

::: gfx/layers/ipc/CompositorChild.h
@@ +113,5 @@
>    bool SendNotifyHidden(const uint64_t& id);
>    bool SendNotifyVisible(const uint64_t& id);
>    bool SendNotifyChildCreated(const uint64_t& id);
>    bool SendAdoptChild(const uint64_t& id);
> +  bool SendMakeSnapshot(PLayerTransactionChild* layerTransactionChildActor,

Pass the PLayerTransactionChild from content process to chrome. Then we can set the right layer root to LayerManagerComposite.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +2332,5 @@
> +  { // scope lock
> +    MonitorAutoLock lock(*sIndirectLayerTreesLock);
> +    parent = sIndirectLayerTrees[id].mParent;
> +  }
> +  if (parent) {

Just like what we do as [1]. After we get the right CompositorParent, we can call the RecvMakeSnapshot() to do the remaining snapshot task.

[1]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/ipc/CompositorParent.cpp#l2155

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5: Use compositor for snapshot. v4

rebase.

Comment 48

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Handle y-flip and rb color channel swap for compositor snapshot. v5

rebase

Comment 49

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8638046 [details] [diff] [review]
P6: Handle y-flip and rb color channel swap for compositor snapshot. v5

Review of attachment 8638046 [details] [diff] [review]:
-----------------------------------------------------------------

Create a special renderTarget with r-b swap and y-flip as cairo format.

::: gfx/layers/opengl/CompositingRenderTargetOGL.h
@@ +64,5 @@
>  
>  public:
>    CompositingRenderTargetOGL(CompositorOGL* aCompositor, const gfx::IntPoint& aOrigin,
> +                             GLuint aTexure, GLuint aFBO,
> +                             bool aUseGLCoordinate = true, bool aRBSwap = false)

Add two additional parameter for rb-swap and y-flip.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1026,5 @@
>    }
>    IntRect intClipRect;
>    clipRect.ToIntRect(&intClipRect);
>  
> +  gl()->fScissor(intClipRect.x, (mUseGLCoordinate) ? FlipY(intClipRect.y + intClipRect.height) : intClipRect.y,

Since we don't transform the scissor rect with the layer matrix, we need adjust the rect for non-y-flip case.

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +449,5 @@
>    } else {
>      fs << "  COLOR_PRECISION float mask = 1.0;" << endl;
>      fs << "  color *= mask;" << endl;
>    }
> +  if (aConfig.mFeatures & ENABLE_DRAW_TARGET_RB_SWAP) {

rb-swap shader code

Comment 50

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8637807 [details] [diff] [review]
P7: Handle pan/zoom factor. v1

Review of attachment 8637807 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1046,5 @@
>      let maxPixelWidth = Math.round(maxWidth * devicePixelRatio);
>      let maxPixelHeight = Math.round(maxHeight * devicePixelRatio);
>  
> +    let contentPixelWidth = Math.round(content.innerWidth * devicePixelRatio * zoomFactor);
> +    let contentPixelHeight = Math.round(content.innerHeight * devicePixelRatio * zoomFactor);

"content.innerHeight * devicePixelRatio" doesn't equal to the actual devicePixelHeight at screen. We have pan/zoom for browser app. Adjust the size with the scale factor.

::: gfx/layers/client/ClientLayerManager.cpp
@@ +498,5 @@
>          bounds = RotateRect(bounds, outerBounds, mTargetRotation);
>        }
> +      Rect scaledRect = resolutionScaleMatrix.TransformBounds(Rect(bounds));
> +      scaledRect.RoundOut();
> +      scaledRect.ToIntRect(&bounds);

Adjust the final rect with the zoom factor too.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8633943 [details] [diff] [review]
P2: Create a pref "layer.content.widget-layer" for content WIDGET_LAYER usage. v1

Review of attachment 8633943 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/ClientLayerManager.cpp
@@ +221,5 @@
>  #endif
>  
>    // If we have a non-default target, we need to let our shadow manager draw
>    // to it. This will happen at the end of the transaction.
> +  if (aTarget && gfxPrefs::UseWidgetLayerAtContent()) {

This is disabling this path for parent processes where the pref is off (which is the default). That seems like a mistake.

::: gfx/thebes/gfxPrefs.h
@@ +348,5 @@
>    DECL_GFX_PREF(Once, "layers.uniformity-info",                UniformityInfo, bool, false);
>    DECL_GFX_PREF(Once, "layers.use-image-offscreen-surfaces",   UseImageOffscreenSurfaces, bool, false);
>  
> +  // Use widget-layer at content process
> +  DECL_GFX_PREF(Once, "layer.content.widget-layer",            UseWidgetLayerAtContent, bool, false);

A temporary pref for this is OK, but really we should have this supported across platforms.

::: layout/base/nsPresShell.cpp
@@ +4671,5 @@
>      nsView* view = rootFrame->GetView();
>      if (view && view->GetWidget() &&
>          nsLayoutUtils::GetDisplayRootFrame(rootFrame) == rootFrame) {
>        LayerManager* layerManager = view->GetWidget()->GetLayerManager();
> +      if (gfxPrefs::UseWidgetLayerAtContent() && layerManager && layerManager->AsClientLayerManager()) {

Note that this patch turns *off* PAINT_WIDGET_LAYERS for the main process. That seems like a mistake.

@@ -4676,5 @@
> -      // ClientLayerManagers in content processes don't support
> -      // taking snapshots.
> -      if (layerManager &&
> -          (!layerManager->AsClientLayerManager() ||
> -           XRE_IsParentProcess())) {

We added this patch because of the reason in the comment: currently on trunk, ClientLayerManagers don't support taking snapshots correctly in content processes --- because the client's ClientLayerManager isn't a toplevel layer manager --- so PAINT_WIDGET_LAYERS does not work. Hopefully your work will fix this.

Comment 52

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: Create a pref "layer.content.widget-layer" for content WIDGET_LAYER usage. v2

Parent process should always use WIDGET_LAYER flag.

Comment 53

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8638369 [details] [diff] [review]
P2: Create a pref "layer.content.widget-layer" for content WIDGET_LAYER usage. v2

Review of attachment 8638369 [details] [diff] [review]:
-----------------------------------------------------------------

If we are at parent process, just add the WIDGET_LAYER.
Otherwise, check the pref and layerManager type.

I would like to have compositor snapshot at b2g first, since we can't get better performance without gralloc buffer. If we use compositor snapshot for snapshot at other platform, we need to copy the pixel data from frameBuffer to content side's buffer. It's slower than the original basicLayerManager.

The pref is default off. If we pass all test, I will turn on it at another bug.

Comment 54

[Nicolas Silva [:nical]]

Comment on attachment 8633944 [details] [diff] [review]
P3: Create a gl render target from TextureHost. v1

Review of attachment 8633944 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +543,5 @@
> +
> +  if (!aTextureSource.get()) {
> +    return nullptr;
> +  }
> +  GLTextureSource* glSource = aTextureSource->AsSourceOGL()->AsGLTextureSource();

(answering your question about checking for the return value of AsSourceOGLand AsGLTextureSource)
If either of these two return null it means there are incompatible backends at play and it's a bug. It's fine by me to let it crash on the null deref so that we can notice and fix the bug if it ever happens.

Comment 55

[Nicolas Silva [:nical]]

Comment on attachment 8638046 [details] [diff] [review]
P6: Handle y-flip and rb color channel swap for compositor snapshot. v5

Review of attachment 8638046 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/CompositorOGL.h
@@ +474,5 @@
>    gfx::Rect mRenderBoundsOut;
>  
>    CompositorOGLVRObjects mVR;
> +
> +  bool mUseGLCoordinate;

nit: please add a comment explaining that gl coordinates refers to whether y needs to be flipped.

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +449,5 @@
>    } else {
>      fs << "  COLOR_PRECISION float mask = 1.0;" << endl;
>      fs << "  color *= mask;" << endl;
>    }
> +  if (aConfig.mFeatures & ENABLE_DRAW_TARGET_RB_SWAP) {

Isn't the existing ENABLE_TEXTURE_RB_SWAP enough to achieve what you need?

On a side note I am not super found of "DrawTarget" being in the name here, it sounds like a moz2d thing while this refers to the compositor render target. ENABLE_TARGET_RG_SWAP, SetTargetRBSwap, would be less confusing but first I'd like to understand why not using ENABLE_TEXTURE_RB_SWAP (I am not used to the shader generation code so the question might sound silly).

Comment 56

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8638046 [details] [diff] [review]
P6: Handle y-flip and rb color channel swap for compositor snapshot. v5

Review of attachment 8638046 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +449,5 @@
>    } else {
>      fs << "  COLOR_PRECISION float mask = 1.0;" << endl;
>      fs << "  color *= mask;" << endl;
>    }
> +  if (aConfig.mFeatures & ENABLE_DRAW_TARGET_RB_SWAP) {

The TEXTURE_RB_SWAP is at [1]. It's in the "vec4 sample(vec2 coord)" function block [2] used at [3].

So the rb_swap only swap the data from the texture. But we want the "framebuffer result" with rb-swap. Then we can use this framebuffer data for cairo backend. 

[1]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/opengl/OGLShaderProgram.cpp#l370
[2]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/opengl/OGLShaderProgram.cpp#l335
[3]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/opengl/OGLShaderProgram.cpp#l410

Comment 57

[Nicolas Silva [:nical]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #56)
> So the rb_swap only swap the data from the texture. But we want the
> "framebuffer result" with rb-swap. Then we can use this framebuffer data for
> cairo backend. 

If I understand correctly the only thing that doesn't depend on TEXTURE_RB_SWAP is filling a solid color rectangle (like color layers), right? I am trying to understand if we really need both TEXTURE_RB_SWAP and TARGET_RB_SWAP. I'd rather avoid adding too many configurations especially if they can cancel one another.

Comment 58

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8638044 [details] [diff] [review]
P5: Use compositor for snapshot. v4

Review of attachment 8638044 [details] [diff] [review]:
-----------------------------------------------------------------

This is the main patch for compositor-base snapshot.

We do:
1) create a textureClient and pass to chrome
2) at chrome, use the corresponding textureHost as OGL render target
3) compose the content's sub-layer-tree to the new OGL render target
4) copy the textureClient to canvas2d's cairo drawTarget.

After 4), the js side can get the final snapshot data.

::: gfx/layers/client/ClientLayerManager.cpp
@@ -478,5 @@
>    if (!mShadowTarget) {
>      return;
>    }
>    if (mWidget) {
> -    if (CompositorChild* remoteRenderer = GetRemoteRenderer()) {

It always returns nullptr if we are at content process. So we change to use GetCompositorChild().

@@ +497,5 @@
> +            TextureClient::CreateForDrawing(mForwarder,
> +                                            SurfaceFormat::R8G8B8A8,
> +                                            bounds.Size(),
> +                                            gfx::BackendType::NONE,
> +                                            TextureFlags::DEALLOCATE_CLIENT);

Try to create a textureClient and pass it to chrome as compositor render target.

@@ +505,5 @@
> +
> +        if (textureClient && textureClient->CanExposeDrawTarget() &&
> +            textureClient->ToSurfaceDescriptor(snapshotSurfaceDescriptor) &&
> +            remoteRenderer->SendMakeSnapshot(mForwarder->GetShadowManager(),
> +                                             GetRoot()->AsShadowableLayer()->GetShadow(),

Pass the current process's root layer to chrome. Then parent side could draw the sub-layer-tree as snapshot.

@@ +530,5 @@
> +          }
> +          dt->SetTransform(oldMatrix);
> +
> +          // Defer the release of textureClient. When we use GrallocTextureHostOGL,
> +          // it spends a lot of time for munmap() call in dtor.

In profiler, I can see we spend about 8ms for 1920*1080 grallocBuffer releasing. Defer the release to boost up the snapshot call.

::: gfx/layers/client/TextureClient.h
@@ +485,5 @@
> +    * Calling ToSurfaceDescriptor again after it has already returned true,
> +    * or never constructing a TextureHost with aDescriptor may result in a memory
> +    * leak (see CairoTextureClientD3D9 for example).
> +    */
> +   virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aDescriptor) = 0;

Since we need to pass the textureClieht to chrome, we need to export ToSurfaceDescriptor() as the public interface.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +688,5 @@
>    nsRefPtr<Composer2D> composer2D;
>    composer2D = mCompositor->GetWidget()->GetComposer2D();
>  
>    // We can't use composert2D if we have layer effects
> +  if (!HaveTarget() && !haveLayerEffects &&

We can't use hwc if we have target. Just use compositorOGL to do that.

@@ +795,5 @@
>  
>      mCompositor->EndFrame();
>    }
>  
> +  if (composer2D && !HaveTarget()) {

If we have target, we don't need to run hwc related code.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +802,5 @@
>  CompositorParent::RecvMakeSnapshot(PLayerTransactionParent* layerTransactoinActor,
>                                     PLayerParent* snapshotRoot,
>                                     const SurfaceDescriptor& aInSnapshot,
>                                     const gfx::IntRect& aRect)
>  {

This function almost comes from CompositorParent::CompositeToTarget()[1], but I remove a lot of checking condition and use the compositor directly without re-scheduling the composition task.

[1]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/ipc/CompositorParent.cpp#l1114

@@ +816,5 @@
> +  if (!mLayerManager || !currentRoot) {
> +    return true;
> +  }
> +
> +  mLayerManager->SetRoot(currentRoot);

We set the corresponding layer PLayerParent sending from content as the root layer. Then we can use compositor to compose this sub-layer tree.
Please check ClientLayerManager::MakeSnapshotIfRequired() for the content side layer.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +670,5 @@
>  #if MOZ_WIDGET_ANDROID
>    TexturePoolOGL::Fill(gl());
>  #endif
>  
> +  if (!mCompositingRenderTarget) {

If we have mCompositingRenderTarget, set it as the render target.
Otherwise, use the default render target.

@@ +875,5 @@
>      return iter->second;
>  
> +  PROFILER_LABEL("CompositorOGL", "CompileShaderProgram",
> +    js::ProfileEntry::Category::GRAPHICS);
> +

Since we have a new rb-swap shader, we might cost a lot of time for the first time usage. Add a profiler label here to check.

@@ +1400,5 @@
>    mContextStateTracker.PopOGLSection(gl(), "Frame");
>  
>    mFrameInProgress = false;
>  
> +  if (HasTarget()) {

HasTarget() will check if we have mTarget or mCompositingRenderTarget.

The mCompositingRenderTarget is the new textureHost base renderTarget.

@@ -1394,5 @@
>    mFrameInProgress = false;
>  
> -  if (mTarget) {
> -    PROFILER_LABEL("CompositorOGL", "handleCopy",
> -      js::ProfileEntry::Category::GRAPHICS);

Please ignore this profiler label, it my test code. Will be removed at next round patch.

@@ +1404,5 @@
> +  if (HasTarget()) {
> +    if (mTarget) {
> +      CopyToTarget(mTarget, mTargetBounds.TopLeft(), Matrix());
> +    } else {
> +      // Flush all GL command for mCompositingRenderTarget.

This is the new TextureHostOGL base renderTarget path. Call glFinish to make sure all gl commands are finished.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #53)
> I would like to have compositor snapshot at b2g first, since we can't get
> better performance without gralloc buffer. If we use compositor snapshot for
> snapshot at other platform, we need to copy the pixel data from frameBuffer
> to content side's buffer. It's slower than the original basicLayerManager.

I believe it's higher latency in some cases. But in other cases, e.g. involving 3D transforms, it'll be faster. And I suspect in most or all cases it'll be higher *throughput* assuming we avoid blocking on the readback. And for the usecases other than page transitions that I know about --- screenshots --- throughput matters more than latency. And on desktop, maybe the increased latency of readback still doesn't matter for the use cases.

Comment 60

[Nicolas Silva [:nical]]

Comment on attachment 8638046 [details] [diff] [review]
P6: Handle y-flip and rb color channel swap for compositor snapshot. v5

Review of attachment 8638046 [details] [diff] [review]:
-----------------------------------------------------------------

Deferring to Jeff about the rb swap thing.

Comment 61

[Nicolas Silva [:nical]]

Comment on attachment 8638044 [details] [diff] [review]
P5: Use compositor for snapshot. v4

Review of attachment 8638044 [details] [diff] [review]:
-----------------------------------------------------------------

It's fine to wire things up this way as a first step, but if we want to achieve the best performance we should do this without having a round-trip to the child process and without copying to a canvas layer (but I understand that we may want to do the canvas thing as a first step to avoid changing too many things at once). Ideally the parent process's main thread would send an synchronous message to the compositor which will create the TextureHost, render into it and send the resulting PTexture to the main thread asynchronously (this means adding some glue to create PTexture from the compositor rather than from the main thread but it's not too hard) and finally use the texture in an ImageLayer instead of a canvas to avoid copying it.

In the mean time, please don't use DEALLOCATE_CLIENT for this, it's way too expensive and we generally try to only use it as a last resort (see comments below).

::: gfx/layers/client/ClientLayerManager.cpp
@@ +497,5 @@
> +            TextureClient::CreateForDrawing(mForwarder,
> +                                            SurfaceFormat::R8G8B8A8,
> +                                            bounds.Size(),
> +                                            gfx::BackendType::NONE,
> +                                            TextureFlags::DEALLOCATE_CLIENT);

Why do you use the TextureFlags::DEALLOCATE_CLIENT flag ? It is very expensive (and the reason you resort to dispatching a task to release the texture a few lines below).

@@ +504,5 @@
> +        MOZ_ASSERT(textureClient && textureClient->CanExposeDrawTarget());
> +
> +        if (textureClient && textureClient->CanExposeDrawTarget() &&
> +            textureClient->ToSurfaceDescriptor(snapshotSurfaceDescriptor) &&
> +            remoteRenderer->SendMakeSnapshot(mForwarder->GetShadowManager(),

Instead of using ToSurfaceDescriptor manually, please either change SendMakeSnapshot to take a PTexture actor as parameter, or add a new ipdl message that takes the actor as parameter.

Comment 62

[Benoit Girard (:BenWa)]

Comment on attachment 8633945 [details] [diff] [review]
P4: Update MakeSnapshot() ipc call. v1

Review of attachment 8633945 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +2335,5 @@
> +  }
> +  if (parent) {
> +    parent->RecvMakeSnapshot(layerTransactoinActor, snapshotRoot, aInSnapshot, aRect);
> +
> +    return true;

this should return the result of RecvMakeSnapshot.

@@ +2336,5 @@
> +  if (parent) {
> +    parent->RecvMakeSnapshot(layerTransactoinActor, snapshotRoot, aInSnapshot, aRect);
> +
> +    return true;
> +  }

If we don't find the parent then we're silently ignoring this. If this is what we want then it should be documented in a comment. Otherwise there should be an assertion here.

::: gfx/layers/ipc/PCompositor.ipdl
@@ +116,5 @@
>    // results are undefined.
>    //
>    // NB: this message will result in animations, transforms, effects,
>    // and so forth being interpolated.  That's what we want to happen.
> +  sync MakeSnapshot(PLayerTransaction layerTransactoinActor, PLayer snapshotRoot, SurfaceDescriptor inSnapshot, IntRect dirtyRect);

Typo in 'layerTransactoinActor'.

- Why are we not sending a layer tree ID here like we normally do?
Are we still able to snapshot the root or only sub layer tree? We assert on the id != 0 which IIRC is the default root? If we really have no need for it then it's fine but we're breaking the API as it was originally designed.

Comment 63

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8633945 [details] [diff] [review]
P4: Update MakeSnapshot() ipc call. v1

Review of attachment 8633945 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/PCompositor.ipdl
@@ +116,5 @@
>    // results are undefined.
>    //
>    // NB: this message will result in animations, transforms, effects,
>    // and so forth being interpolated.  That's what we want to happen.
> +  sync MakeSnapshot(PLayerTransaction layerTransactoinActor, PLayer snapshotRoot, SurfaceDescriptor inSnapshot, IntRect dirtyRect);

a) Why are we not sending a layer tree ID here like we normally do?
=>Yes, we can send the layer tree id to parent. But at parent side, I will use the PLayerTransactionParent to alloc textureHost in CompositorParent::RecvMakeSnapshot(). I still need to get the PLayerTransactionParent from sIndirectLayerTrees using the layer tree id.
Please check [1].

[1]
https://bugzilla.mozilla.org/attachment.cgi?id=8638044&action=diff#a/gfx/layers/ipc/CompositorParent.cpp_sec3

b) id != 0
If we assert for "id!=0", we still can get the snapshot for the default root. All asserts for "id!=0" are for "CrossProcessCompositorParent". The id 0 is create from [2]. It's the in-process ipc pair. If we go through in-process path, we don't use CrossProcessCompositorParent.

[2]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/widget/nsBaseWidget.cpp#l1123

Comment 64

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Nicolas Silva [:nical] from comment #61)
> Comment on attachment 8638044 [details] [diff] [review]
> P5: Use compositor for snapshot. v4
> 
> Review of attachment 8638044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's fine to wire things up this way as a first step, but if we want to
> achieve the best performance we should do this without having a round-trip
> to the child process and without copying to a canvas layer (but I understand
> that we may want to do the canvas thing as a first step to avoid changing
> too many things at once). Ideally the parent process's main thread would
> send an synchronous message to the compositor which will create the
> TextureHost, render into it and send the resulting PTexture to the main
> thread asynchronously (this means adding some glue to create PTexture from
> the compositor rather than from the main thread but it's not too hard) and
> finally use the texture in an ImageLayer instead of a canvas to avoid
> copying it.

In my next patch, I will just send the PTextureClient to parent.
Since we only have the optimized path for b2g(having grallocBuffer). Content side changes to uses the new interface "CreateForParentDrawing()" for textureClient creation [1]. That function just calls CreateBufferTextureClient() with shmem for all platform except B2G. B2G will use GrallocTextureClientOGL.

At parent side, I will replace all gfx::DrawTarget related code with textureHost. Then, copy the framebuffer data into textureHost using textureHost::Update() for non-gralloc-texture-host case.

Thus, we can merge the faster path and the original path(using surfaceDescriptor)[2] for snapshot.

[1]
https://pastebin.mozilla.org/8840802
[2]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/ipc/CompositorParent.cpp#l804

Comment 65

[Jeff Muizelaar [:jrmuizel]]

Sorry for not getting to this review earlier, I'll try to get it done today. I've only skimmed the bug, but are there performance results from this approach vs our current approach?

Comment 66

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8638046 [details] [diff] [review]
P6: Handle y-flip and rb color channel swap for compositor snapshot. v5

Review of attachment 8638046 [details] [diff] [review]:
-----------------------------------------------------------------

I'm confused why the rb color swap is needed. Shouldn't using a PIXEL_FORMAT_BGRA_8888 gralloc buffer be sufficient?

Comment 67

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8638044 [details] [diff] [review]
P5: Use compositor for snapshot. v4

Review of attachment 8638044 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see the new version of this patch as well as performance results vs the current snapshotting approach. The current numbers in the bug don't show any advantage.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +347,3 @@
>    mTarget = nullptr;
> +  mCompositingRenderTarget = nullptr;
> +  mTextureTarget = nullptr;

This field is never read.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8637807 [details] [diff] [review]
P7: Handle pan/zoom factor. v1

(On desktop when we zoom, both screen.width and innerWidth change, apparently on b2g that doesn't happen.)

Comment 69

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Create a gl render target from TextureHost. v2

Pass the TextureHost to CompositorOGL::SetTarget() directly.
Check TextureHost direct drawing flag.

Comment 70

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: Update MakeSnapshot() ipc call. v2

Update MakeSnapshot interface. Add an interface for making snapshot with PTexture as comment 61.
Handle ipc message return value.

Comment 71

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7: Handle pan/zoom factor. v2. r=roc, smaug

rebase

Comment 72

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Handle y-flip and rb color channel swap for compositor snapshot. v6

rebase

Comment 73

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Create a gl render target from TextureHost. v3

Pass TextureHost to SetTarget() directly.
Check TextureHost direct drawing flag(make sure the TextureHost could be shared between process directly).

Comment 74

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4: Update MakeSnapshot() ipc call. v3

Please check comment 63 for "layer id" problem.

Handle ipc message return value(comment 62)
Pass PTexture instead of SurfaceDescriptor(comment 61)
=>Instead of using ToSurfaceDescriptor manually, please either change SendMakeSnapshot to take a PTexture actor as parameter, or add a new ipdl message that takes the actor as parameter.
Update MakeSnapshot interface. Add an interface for making snapshot into PTexture.

The MakeSnapshot message implementation is at patch P5-4.

Comment 75

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-1: Add TextureClient for parent side direct drawing. v1

Comment 76

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-2: Begin transaction with TextureHost. v1

Comment 77

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-3: Handle the different codepath if we set a DrawTarget or TextureHost. v1

Comment 78

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-4: Use compositor for snapshot. v1

Comment 79

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8641682 [details] [diff] [review]
P6: Handle y-flip and rb color channel swap for compositor snapshot. v6

If we use PIXEL_FORMAT_BGRA_8888 format , we still use RGBA8888 with TEXTURE_RB_SWAP flag[1]. We still can't get a rb-swap framebuffer content.
Please check comment 56.

[1]
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/opengl/GrallocTextureClient.cpp#l232

Comment 80

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: compositor_snapshot_with_compiling_rb_swap_shader.html

The android systrace for combinational key(press power and down key) snapsnot performance. Please use chrome browser to open it.

1. gralloc texture direct drawing
2. rb-swap shader compiling(it only appears at the first time we use this shader).

At nexus5-l:
CanvasRenderingContext2D::DrawWindow() => 82.161 ms

Comment 81

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: compositor_snapshot_without_compiling_rb_swap_shader.html

The android systrace for combinational key(press power and down key) snapsnot performance. Please use chrome browser to open it.

1. gralloc texture direct drawing
2. no rb-swap shader compiling(We take another snapshot here, and we don't need to compile the shader again. It's in the shader cache)

At nexus5-l:
CanvasRenderingContext2D::DrawWindow() => 66.908 ms

Comment 82

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: original_compositor_snapshot.html

The android systrace for combinational key(press power and down key) snapsnot performance. Please use chrome browser to open it.

This is the original b2g snapshot path.

We spend a lot of time at CopyToTarget() in CompositorOGL::EndFrame().
We can remove the CopyToTarget() with gralloc texture direct rendering, y-flip and rb-swap at compositor.

At nexus5-l:
CanvasRenderingContext2D::DrawWindow() => 129.475 ms

Comment 83

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Take the snapshot a homescreen:

The new path:
CanvasRenderingContext2D::DrawWindow() (no shader compiling)=> 66.908 ms
CanvasRenderingContext2D::DrawWindow() (with shader compiling)=> 82.161 ms

The original path:
CanvasRenderingContext2D::DrawWindow() => 129.475 ms

We remove the CopyToTarget() call with GrallocTexture.

--------
These patches could improve the CanvasRenderingContext2D::DrawWindow() at chrome process, but it's not good for content side.

We spend a lot of time doing the copy:
GrallocTexture => ShadowTarget => CanvasTarget

And I also see we spend more time at BuildDisplayList() and ProcessDisplayItems() comparing with the original basicLayerManager rendering for the CanvasRenderingContext2D::DrawWindow() at content.

Comment 84

[Nicolas Silva [:nical]]

Comment on attachment 8642344 [details] [diff] [review]
P3: Create a gl render target from TextureHost. v3

Review of attachment 8642344 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/TextureHost.h
@@ +500,5 @@
>    virtual bool HasInternalBuffer() const { return false; }
>  
> +  // Some TextureHosts could be a CompositingRenderTarget and be updated without
> +  // any additional memory copying.
> +  virtual bool DirectDrawing() { return false; }

You don't need to add this: DirectDrawing is equivalent to !HasInternalBuffer so I would prefer to just use !HasInternalBuffer

Comment 85

[Nicolas Silva [:nical]]

Comment on attachment 8642347 [details] [diff] [review]
P4: Update MakeSnapshot() ipc call. v3

Review of attachment 8642347 [details] [diff] [review]:
-----------------------------------------------------------------

You removed the implementation of RecvMakeSnapshot and I don't think it was your intention

::: gfx/layers/ipc/CompositorParent.cpp
@@ +803,5 @@
> +                                              PLayerParent* aSnapshotRoot,
> +                                              const SurfaceDescriptor& aInSnapshot,
> +                                              const gfx::IntRect& aRect)
> +{
> +  return true;

If you don't implement this for now, it's best to add an assertion so that we don't run into this code on desktop or android by accident.

@@ -803,3 @@
>  {
> -  RefPtr<DrawTarget> target = GetDrawTargetForDescriptor(aInSnapshot, gfx::BackendType::CAIRO);
> -  ForceComposeToTarget(target, &aRect);

Shouldn't this code still exist in RecvMakeSnapshotWithSurface?

Comment 86

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8642347 [details] [diff] [review]
P4: Update MakeSnapshot() ipc call. v3

Review of attachment 8642347 [details] [diff] [review]:
-----------------------------------------------------------------

Please check comment 74.

Comment 87

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #79)
> Comment on attachment 8641682 [details] [diff] [review]
> P6: Handle y-flip and rb color channel swap for compositor snapshot. v6
> 
> If we use PIXEL_FORMAT_BGRA_8888 format , we still use RGBA8888 with
> TEXTURE_RB_SWAP flag[1]. We still can't get a rb-swap framebuffer content.
> Please check comment 56.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/
> opengl/GrallocTextureClient.cpp#l232

I talked with Sotaro and it seems like we should be able to change this part of the code to use PIXEL_FORMAT_BGRA_8888. Can you investigate that approach instead?

Comment 88

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #87)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #79)
> > Comment on attachment 8641682 [details] [diff] [review]
> > P6: Handle y-flip and rb color channel swap for compositor snapshot. v6
> > 
> > If we use PIXEL_FORMAT_BGRA_8888 format , we still use RGBA8888 with
> > TEXTURE_RB_SWAP flag[1]. We still can't get a rb-swap framebuffer content.
> > Please check comment 56.
> > 
> > [1]
> > https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/gfx/layers/
> > opengl/GrallocTextureClient.cpp#l232
> 
> I talked with Sotaro and it seems like we should be able to change this part
> of the code to use PIXEL_FORMAT_BGRA_8888. Can you investigate that approach
> instead?

Since we would like use the texture as a render target and use it as a bgra buffer, system should support "bgra" framebuffer object. Thus, we don't need any new shader for rb-swap.

Unfortunately, GLES 2.0 supports fbo texture format GL_RGBA4, GL_RGB5_A1, GL_RGB565[1] and GL_RGBA888[2] as an extension. I don't see the document that we could use bgra as a render target.

I would like to describe again that the GrallocTexture is used for drawing. It's not for reading. Our TEXTURE_RB_SWAP is used for reading the rbga data from bgra buffer. And b2g don't have the capability to draw a bgra result yet. The P6 patch wants to do the rb swap in gpu.

[1]
https://www.khronos.org/opengles/sdk/docs/man/xhtml/glCheckFramebufferStatus.xml
[2]
https://www.khronos.org/registry/gles/extensions/ARM/ARM_rgba8.txt

Comment 89

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #88)
> Unfortunately, GLES 2.0 supports fbo texture format GL_RGBA4, GL_RGB5_A1,
> GL_RGB565[1] and GL_RGBA888[2] as an extension. I don't see the document
> that we could use bgra as a render target.

Yes, it looks that way. Sorry for the distraction. You're approach should be fine then.

Comment 90

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Create a gl render target from TextureHost. v4

Update for comment 84.

Pass TextureHost to SetTarget() directly.
Check TextureHost HasInternalBuffer() flag for directly drawing at compositor.

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I still think we need a better API than drawWindow here. We should return a Promise containing an ImageBitmap, since ImageBitmap will let us render the image result without having to read the data back from the GPU.

(In reply to Nicolas Silva [:nical] from comment #17)
> Promise createScreenshot() -- returns a promise of a ScreenshotImageThingy
> object (no read-back, but can be placed in the DOM and seen on screen)
> 
> Promise ScreenshotImageThingy.getImageBitmap() -- returns a promise of a
> ImageBitmap (does the readback)

ImageBitmap can be rendered without readback, since it contains a layers::Image. So we don't need to split the API like this.

We do need to return the resolution --- the ratio of the target window's CSS pixels to ImageBitmap pixels. So I suggest the following:

dictionary SnapshotResult {
  ImageBitmap image;
  float resolution;
}

partial interface Window {
  Promise<SnapshotResult> snapshotCurrentContents();
}

This method should not flush style or layout or do painting, or block at all --- just message the compositor to take a snapshot of the current painted contents and return to the caller. When the snapshot has been taken and returned to the content process, resolve the promise.

Comment 92

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8641682 [details] [diff] [review]
P6: Handle y-flip and rb color channel swap for compositor snapshot. v6

Review of attachment 8641682 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/CompositingRenderTargetOGL.h
@@ +175,5 @@
>    GLContext* mGL;
>    GLuint mTextureHandle;
>    GLuint mFBO;
> +  bool mUseGLCoordinate;
> +  bool mRBSwap;

Add a comment that says that these are used for rendering to gralloc buffers that can be used as BGRA images buffers by cairo.

Comment 93

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #91)
> I still think we need a better API than drawWindow here. We should return a
> Promise containing an ImageBitmap, since ImageBitmap will let us render the
> image result without having to read the data back from the GPU.
> 
> (In reply to Nicolas Silva [:nical] from comment #17)
> > Promise createScreenshot() -- returns a promise of a ScreenshotImageThingy
> > object (no read-back, but can be placed in the DOM and seen on screen)
> > 
> > Promise ScreenshotImageThingy.getImageBitmap() -- returns a promise of a
> > ImageBitmap (does the readback)
> 
> ImageBitmap can be rendered without readback, since it contains a
> layers::Image. So we don't need to split the API like this.
> 
> We do need to return the resolution --- the ratio of the target window's CSS
> pixels to ImageBitmap pixels. So I suggest the following:
> 
> dictionary SnapshotResult {
>   ImageBitmap image;
>   float resolution;
> }
> 
> partial interface Window {
>   Promise<SnapshotResult> snapshotCurrentContents();
> }
> 
> This method should not flush style or layout or do painting, or block at all
> --- just message the compositor to take a snapshot of the current painted
> contents and return to the caller. When the snapshot has been taken and
> returned to the content process, resolve the promise.

I will export another api base on these patch. These patches do the composition to textureClieht/Host directly. That will be used for our new api.

Comment 94

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8642348 [details] [diff] [review]
P5-1: Add TextureClient for parent side direct drawing. v1

Review of attachment 8642348 [details] [diff] [review]:
-----------------------------------------------------------------

I'd use 'Compostitor' instead of 'Parent', but other people might feel differently.

Comment 95

[Benoit Girard (:BenWa)]

Comment on attachment 8642347 [details] [diff] [review]
P4: Update MakeSnapshot() ipc call. v3

Review of attachment 8642347 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sold on the argument in https://bugzilla.mozilla.org/show_bug.cgi?id=1173286#c63. Passing the id means we're effectively holding a weak reference and that's what we do in general. I'm don't know enough about the code to be sure about the life times here and I'm not confident to say if the code is or isn't right during things like shutdown. Also it's not valuable to have me review a random piece of a large patch queue.

Nical should know these lifetimes better and has review more of the patch queue.

Comment 96

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #91)
> I still think we need a better API than drawWindow here. We should return a
> Promise containing an ImageBitmap, since ImageBitmap will let us render the
> image result without having to read the data back from the GPU.
> 
> (In reply to Nicolas Silva [:nical] from comment #17)
> > Promise createScreenshot() -- returns a promise of a ScreenshotImageThingy
> > object (no read-back, but can be placed in the DOM and seen on screen)
> > 
> > Promise ScreenshotImageThingy.getImageBitmap() -- returns a promise of a
> > ImageBitmap (does the readback)
> 
> ImageBitmap can be rendered without readback, since it contains a
> layers::Image. So we don't need to split the API like this.
> 
> We do need to return the resolution --- the ratio of the target window's CSS
> pixels to ImageBitmap pixels. So I suggest the following:
> 
> dictionary SnapshotResult {
>   ImageBitmap image;
>   float resolution;
> }
> 
> partial interface Window {
>   Promise<SnapshotResult> snapshotCurrentContents();
> }
> 
> This method should not flush style or layout or do painting, or block at all
> --- just message the compositor to take a snapshot of the current painted
> contents and return to the caller. When the snapshot has been taken and
> returned to the content process, resolve the promise.

I'm trying to export the new screenshot api in HTMLIFrameElement.webidl now.
It will use Promise and ImageBitmap.

Which level will we export this api? Window or iframe?
The proposal exports the api at iframe.
https://wiki.mozilla.org/User:Roc/ScreenCaptureAPI

Another question is:
Will the iframe's corresponding layer(I assume that it's a container layer) be merged to another layer(some inactive layer will be merged to another layer)?
If no, we always have a corresponding PLayer at compositor parent. And then we just compose this layer's sub-tree to our target buffer. If this PLayer is not in the compositor's viewport, we will get nothing.
If we want to get this region's data, we should do the painting for this area at content side and pass the layer to compositor with all culling method off. I think we should just use the original drawWindow() path.

Comment 97

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #96)
> I'm trying to export the new screenshot api in HTMLIFrameElement.webidl now.
> It will use Promise and ImageBitmap.
> 
> Which level will we export this api? Window or iframe?
> The proposal exports the api at iframe.
> https://wiki.mozilla.org/User:Roc/ScreenCaptureAPI

Yes, HTMLIFrameElement sounds better than Window actually. I was right the first time :-).

> Another question is:
> Will the iframe's corresponding layer(I assume that it's a container layer)
> be merged to another layer(some inactive layer will be merged to another
> layer)?
> If no, we always have a corresponding PLayer at compositor parent. And then
> we just compose this layer's sub-tree to our target buffer. If this PLayer
> is not in the compositor's viewport, we will get nothing.

When the <iframe> contains a remote window, we will have a RefLayer for it, unless the <iframe> is completely not visible in which case no layer may be created. We need to do more work to ensure that this API works when there's no layer for the <iframe>'ed document. But that work can be done in a separate bug and until then this API can just fail when there's no layer.

If the layer does exist but is partially outside the compositor's viewport, we won't get the whole element rect. So the message we send to the compositor needs to include the rectangle of content we expect to take a snapshot of. In the compositor, we need to make sure that we composite that part of the layer.

> If we want to get this region's data, we should do the painting for this
> area at content side and pass the layer to compositor with all culling
> method off. I think we should just use the original drawWindow() path.

I'm not sure what you mean by the last sentence.

When the <iframe> is cross-process, there shouldn't be any need to do more painting in the content process to get a snapshot. The content process will have painted enough content to fill the <iframe> regardless of how much of the <iframe> is visible.

Comment 98

[Nicolas Silva [:nical]]

Comment on attachment 8642351 [details] [diff] [review]
P5-4: Use compositor for snapshot. v1

Review of attachment 8642351 [details] [diff] [review]:
-----------------------------------------------------------------

How much more work would it be to just keep the TextureClient instead of copying it? Then just create an ImageLayer around it (in my head this sounds simple but I haven't looked into the details). I'd rather just do that than add this code and then remove it to do the ImageLayer thing which we'll end up doing anyway.

::: gfx/layers/client/ClientLayerManager.cpp
@@ +542,5 @@
> +              snapshotTextureClient->Unlock();
> +            }
> +
> +            // Defer the release of textureClient. When we use GrallocTextureHostOGL,
> +            // it spends a lot of time for munmap() call in dtor.

Even without DEALLOCATE_CLIENT? This is worth investigating because we use gralloc textures all over the place and we don't attempt to dispatch a release task in the other cases. Also, what's the point of doing this in this case, is it to have the snapshot exposed to js as soon as possible? I assume this only moves the cost of unmap to a different time on the same thread.

@@ +546,5 @@
> +            // it spends a lot of time for munmap() call in dtor.
> +            mForwarder->GetMessageLoop()
> +                ->PostTask(FROM_HERE, new TextureClientReleaseTask(snapshotTextureClient));
> +          }
> +        } else if (mForwarder->AllocSurfaceDescriptor(bounds.Size(),

It would be nice to remove this branch entirely (it's probably fine if we do this as a followup).
I would much prefer only having the TextureClient code path and implement the missing parts on the host side rather than having manipulating SurfaceDescriptors like this (which has no advantage over using TextureClient).
I assume this code is there for other platforms that don't have gralloc-like textures?

@@ +550,5 @@
> +        } else if (mForwarder->AllocSurfaceDescriptor(bounds.Size(),
> +                                                      gfxContentType::COLOR_ALPHA,
> +                                                      &snapshotSurface)) {
> +          // Shmem SurfaceDescriptor will be reset to nullptr during ipc. Make
> +          // a copy here for later using.

nit: for later use

Comment 99

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8642351 [details] [diff] [review]
P5-4: Use compositor for snapshot. v1

Review of attachment 8642351 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/ClientLayerManager.cpp
@@ +510,5 @@
> +        RefPtr<TextureClient> snapshotTextureClient;
> +
> +        // Try to use direct drawing first. If we can't create direct drawing
> +        // textureClient, fall back to surface target.
> +        if (gfxPrefs::SnpashotUseDirectDraw()) {

SnapshotUseDirectDraw instead of Snpashot

@@ +519,5 @@
> +                                                    gfx::BackendType::NONE,
> +                                                    TextureFlags::NO_FLAGS);
> +        }
> +
> +        if (snapshotTextureClient) {

It would be nice to split this function up into separate pieces if it's not too hard.

@@ +524,5 @@
> +          // Init texture actor
> +          DebugOnly<bool> actorCreationResult = snapshotTextureClient->InitIPDLActor(mForwarder);
> +          MOZ_ASSERT(actorCreationResult);
> +
> +          printf_stderr("bignose snapshot client layer root:%p",GetRoot());

Remove debugging output.

Comment 100

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-1: Add TextureClient for parent side direct drawing. v2. r=nical,jmuizelaar

update for comment 94

Comment 101

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8642351 [details] [diff] [review]
P5-4: Use compositor for snapshot. v1

Review of attachment 8642351 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/ClientLayerManager.cpp
@@ +510,5 @@
> +        RefPtr<TextureClient> snapshotTextureClient;
> +
> +        // Try to use direct drawing first. If we can't create direct drawing
> +        // textureClient, fall back to surface target.
> +        if (gfxPrefs::SnpashotUseDirectDraw()) {

Sorry, I don't catch the meaning of this comment. Rename the pref name?

@@ +542,5 @@
> +              snapshotTextureClient->Unlock();
> +            }
> +
> +            // Defer the release of textureClient. When we use GrallocTextureHostOGL,
> +            // it spends a lot of time for munmap() call in dtor.

I would like to go back to js as soon as possible. The unmap for 1920x1080 costs about 8ms at nexus5 device. Yes, it just do the unmap at different time on main the same thread.
If it doesn't make sense, I will just remove the texture directly.

@@ +546,5 @@
> +            // it spends a lot of time for munmap() call in dtor.
> +            mForwarder->GetMessageLoop()
> +                ->PostTask(FROM_HERE, new TextureClientReleaseTask(snapshotTextureClient));
> +          }
> +        } else if (mForwarder->AllocSurfaceDescriptor(bounds.Size(),

Yes, this is the original path, and it's used for non-gralloc case.

Comment 102

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: Create a gl render target from TextureHost. v5. r=nical, jmuizelaar

fix HasInternalBuffer() checking error.

Comment 103

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6: Handle y-flip and rb color channel swap for compositor snapshot. v7. r=jmuizelaar

update for comment 92

Comment 104

[BMO Automation]

Firefox OS is not being worked on