804852 - Add support for fast-path 2d composition to Layers OGL (initially)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

GL is a giant powerful tool, and needed to implement the full set of layers features.  But in the most common case, we have a layer tree with just translations (maybe scales).  GL isn't needed to compose those layer trees.

There are some 2d APIs that allow composing trees like that.  One is DRI2, another is hwc (on android).  These APIs can be backed with dedicated non-GPU hardware that's far more power efficient than GPUs.

We have patches to add hwc support to our layers OGL backend.  The goal here is get that support in and pref'd off initially.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Need to set this blocking-basecamp+.  Ping me in private for rationale.

Comment 2

[Nick Cameron [:nrc]]

I suspect this is going to conflict big-time with the layers refactoring :-( Is it possible to do this as a separate layers backend? I guess that would lead to massive code duplication.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The high-level plan here is to
 - create a Composer2D interface, scoped to layers/opengl atm.  IOW, it's a sort of layers OGL "friend" API.
 - the setup of LayerManagerOGL and Composer2D looks like the following
   * LayerManagerOGL is created with a GL context that wraps the framebuffer target
   * a corresponding Composer2D is created for the same framebuffer target
   * this is highly platform-specific, so handled within widget/gonk
   * the LayerManagerOGL and Composer2D are glued together using nsIWidget
 - the common-case interaction is
   * when LayerManagerOGL starts to Render(), it asks Composer2D to TryRender()
   * the Composer2D walks the layer tree and sees if it meets the criteria for fast-pathing
   * if so, it renders and returns true.  otherwise return false

The integration with LayerManagerOGL is done such that readback from the framebuffer continues to work, which means we can reftest layer trees rendered by Composer2D.

That's really about it.  The layer-tree information that we need to expose for the android "hwc" includes layers' buffers, which is yet more reason for this to be a "friend" API initially.

Any concerns or thoughts about this?  The patches are pretty simple, maybe even easier to understand than the description here.  Coming up in a bit.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Nick Cameron [:nrc] from comment #2)
> I suspect this is going to conflict big-time with the layers refactoring :-(

I don't think so; the touch points here are actually quite small.  Composer2D is well isolated.

> Is it possible to do this as a separate layers backend? I guess that would
> lead to massive code duplication.

It's actually not possible at all.  The problem is, Composer2D needs to fall back on GL in lots of situations
 - 3d transforms
 - non-axis-aligned transforms
 - subtrees that require intermediate surfaces
 - layers that are backed by memory that the composer HW can't read
 - ... a few other cases

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 0: Add a dynamic cast to ColorLayer*

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add a Composer2D interface to enable implementations to more efficiently compose layer trees

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Let widgets expose Composer2Ds, if they have them

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Expose a layers ogl "friend" API that Composer2D will consume

Probably the grossest part of this work.  We have to expose some of the internal impl details of the GL layers.  This tries to make this as clean as possible, but it's not, particularly :/.

This doesn't add GetRenderState() for ImageLayer, which we'll want, but we can do that in a followup.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Hook Composer2D into the LayerManagerOGL rendering pipeline

nrc, if there's a way to make this less invasive wrt The Great Layers Refactoring, let me know.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 5: Implement all the goop to let widget/gonk use a Composer2D (HwcComposer2D)

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: WIP HwcComposer2D implementation

This is what this glue code is all working towards.  Our friends are developing this, but aren't able to participate in public bugs anymore :(.

Comment 12

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

Comment on attachment 674532 [details] [diff] [review]
part 1: Add a Composer2D interface to enable implementations to more efficiently compose layer trees

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

::: gfx/layers/opengl/Composer2D.h
@@ +43,5 @@
> +   * Return true if |aRoot| met the implementation's criteria for fast
> +   * composition and the render was successful.  Return false to fall
> +   * back on the GPU.
> +   */
> +  virtual bool TryRender(Layer* aRoot, const gfxMatrix& aGLWorldTransform) = 0;

aWorldTransform. Also, document what aWorldTransform means.

Also, document that we're drawing the entire widget from which the Composer2D was obtained.

Comment 13

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

Comment on attachment 674533 [details] [diff] [review]
part 2: Let widgets expose Composer2Ds, if they have them

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

rev nsIWidget IID.

Is this pull approach really the way to go? I wonder if it might be better to have a method on LayerManagerOGL to set the current Composer2D? That would reduce the exposure of this API.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 674532 [details] [diff] [review]
> part 1: Add a Composer2D interface to enable implementations to more
> efficiently compose layer trees
> 
> Review of attachment 674532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/Composer2D.h
> @@ +43,5 @@
> > +   * Return true if |aRoot| met the implementation's criteria for fast
> > +   * composition and the render was successful.  Return false to fall
> > +   * back on the GPU.
> > +   */
> > +  virtual bool TryRender(Layer* aRoot, const gfxMatrix& aGLWorldTransform) = 0;
> 
> aWorldTransform. Also, document what aWorldTransform means.
> 

I wanted to emphasize that this transform is for "GL space", but ok.

> Also, document that we're drawing the entire widget from which the
> Composer2D was obtained.

Sure.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 674533 [details] [diff] [review]
> part 2: Let widgets expose Composer2Ds, if they have them
> 
> Review of attachment 674533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rev nsIWidget IID.
> 
> Is this pull approach really the way to go? I wonder if it might be better
> to have a method on LayerManagerOGL to set the current Composer2D? That
> would reduce the exposure of this API.

I don't have a strong preference.  I did it this way because it was a lot grosser to "push" this in from widget/gonk, because the layer manager can be initialized on several different paths.  There's only one we really care about though.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 674535 [details] [diff] [review]
part 3: Expose a layers ogl "friend" API that Composer2D will consume

nrc, this part is also relevant to the interests of The Great Refactoring.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Comment on attachment 674532 [details] [diff] [review]
> part 1: Add a Composer2D interface to enable implementations to more
> efficiently compose layer trees
> 
> Also, document that we're drawing the entire widget from which the
> Composer2D was obtained.

I should note that that's not a hard requirement though --- we could compose part of the layer tree (backgrounds etc.) with Composer2D, and then switch over to the GPU for a set of higher z-order layers.  But that's obviously complicated, and is unlikely to win power in the same way, so hasn't been seriously pursued yet.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Add a Composer2D interface to enable implementations to more efficiently compose layer trees, v2

Update, carrying sr=roc.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 674533 [details] [diff] [review]
> part 2: Let widgets expose Composer2Ds, if they have them
> 
> Review of attachment 674533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this pull approach really the way to go? I wonder if it might be better
> to have a method on LayerManagerOGL to set the current Composer2D? That
> would reduce the exposure of this API.

Ah, while changing to a "push" model, I remembered why we have to do this now.  The problem is that the LayerManagerOGL for a given nsIWidget* is set up on the compositor thread, while the main thread is blocked on the compositor.  That's the only reason we get away with the wild thread unsafety.

That setup happens within common code in widget/base, so there's not really a clean way for widget/[platform] to hook in.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 674533 [details] [diff] [review]
part 2: Let widgets expose Composer2Ds, if they have them

sr? re-request.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 674533 [details] [diff] [review]
part 2: Let widgets expose Composer2Ds, if they have them

Nm, needed an IID rev here too.  Coming right up.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Let widgets expose Composer2Ds, if they have them, v2

Comment 23

[Michael Wu [:mwu]]

Comment on attachment 674537 [details] [diff] [review]
part 5: Implement all the goop to let widget/gonk use a Composer2D (HwcComposer2D)

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

::: widget/gonk/HWComposer.h
@@ +41,4 @@
>  private:
> +#else
> +protected:
> +#endif

Just make this protected and get rid of the ifdefs - our version barely resembles the Android version as is.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 674536 [details] [diff] [review]
part 4: Hook Composer2D into the LayerManagerOGL rendering pipeline

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

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +679,5 @@
> +        MakeCurrent();
> +        CopyToTarget(mTarget);
> +        mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0);
> +      }
> +      MOZ_ASSERT(!needGLRender);

It's not possible to hit this, right?

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That's correct.  The goal of this assertion is, no matter what happens in the code above, we never take the GL Render() path.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Wu [:mwu] from comment #23)
> Comment on attachment 674537 [details] [diff] [review]
> part 5: Implement all the goop to let widget/gonk use a Composer2D
> (HwcComposer2D)
> 
> Review of attachment 674537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HWComposer.h
> @@ +41,4 @@
> >  private:
> > +#else
> > +protected:
> > +#endif
> 
> Just make this protected and get rid of the ifdefs - our version barely
> resembles the Android version as is.

Done.

Comment 27

[Nick Cameron [:nrc]]

Comment on attachment 674536 [details] [diff] [review]
part 4: Hook Composer2D into the LayerManagerOGL rendering pipeline

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

I think this won't affect the refactoring at all, we will just move all of this stuff into the Compositor

Comment 28

[Nick Cameron [:nrc]]

Comment on attachment 674535 [details] [diff] [review]
part 3: Expose a layers ogl "friend" API that Composer2D will consume

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

This will be a bit of a pain because that info will be scattered around. But I don't think there is anything to do now which will make it less painful later, and it is a pretty small bit of code, so it won't be too bad at all.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Testcase 1: old homescreen

To run this,
 (1) $ cd b2g-toplevel/gaia
 (2) $ git apply old-homescreen-template.patch
 (3) $ make reset-gaia

When b2g comes back up, you'll see a "Template" app on the third page of icons.  Open this and the benchmark will start.

It simulates transforming a couple of prerendered divs.  The testcase is not so great in that it doesn't simulate user input; for that you need a harness like Eideticker, but it doesn't work on any hvga devices that I know of.  (Needs HDMI out.)

This testcase works with user and eng builds.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note, after you run the steps in comment 29, the "Crystalskull" app will also re-appear on the third page of icons.  That's a fairly interesting testcase of potential increased perf from taking load off of the GPU.

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Testcase 1: old homescreen, v2

Fixed a small bug.  Same instructions for applying and running.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Testcase 2: New homescreen

Like the old one but canvas-ized.  Same instructions to apply, install, and run.

Comment 33

[Diego Wilson [:diego]]

Comment on attachment 674535 [details] [diff] [review]
part 3: Expose a layers ogl "friend" API that Composer2D will consume

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

ImageLayer.h and ImageLayer.cpp will also need this to have complete layer support

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +467,5 @@
> +
> +  bool YFlipped() const
> +  { return mFlags & LAYER_RENDER_STATE_Y_FLIPPED; }
> +
> +  bool BufferRotated() const

Rotation can already be discovered through the layer's transformation matrix. This flag could be useful if combined with a BufferScaled() so the composer can figure out if it can skip using the transformation matrix altogether.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Diego Wilson from comment #33)
> Comment on attachment 674535 [details] [diff] [review]
> part 3: Expose a layers ogl "friend" API that Composer2D will consume
> 
> ::: gfx/layers/opengl/LayerManagerOGL.h
> @@ +467,5 @@
> > +
> > +  bool YFlipped() const
> > +  { return mFlags & LAYER_RENDER_STATE_Y_FLIPPED; }
> > +
> > +  bool BufferRotated() const
> 
> Rotation can already be discovered through the layer's transformation
> matrix. This flag could be useful if combined with a BufferScaled() so the
> composer can figure out if it can skip using the transformation matrix
> altogether.

This is a different kind of buffer rotation :/.  Sorry, it's somewhat confusing.

Comment 35

[Diego Wilson [:diego]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #34)
> This is a different kind of buffer rotation :/.  Sorry, it's somewhat
> confusing.
And that is also different from full screen rotation? Does that mean there's at least three kinds of rotation?

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, unfortunately :(.  There's
 - screen rotation, implemented by a world transform
 - per-layer transforms, which includes rotation transforms
 - layer buffer rotation, which is an optimization to avoid memcpy()s

Comment 37

[Diego Wilson [:diego]]

Comment on attachment 674535 [details] [diff] [review]
part 3: Expose a layers ogl "friend" API that Composer2D will consume

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

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +452,5 @@
>  };
>  
> +enum LayerRenderStateFlags {
> +  LAYER_RENDER_STATE_Y_FLIPPED = 1 << 0,
> +  LAYER_RENDER_STATE_BUFFER_ROTATION = 1 << 1

I think we need a LAYER_RENDER_STATE_CAN_SKIP.

For example: there is a ShadowImageLayer use case where RenderLayer() is a no-op if the image container ID is zero

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement composer friend API for ImageLayerOGL

On gonk, this will unfortunately only work for hardware-decoded video and sw-decoded video we were able to repack into the gralloc yv12 format.

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement Composer2D for hwc

This is the initial implementation of Composer2D that we've developed for the hwc interface.  We'll be looking to land this pref'd off since it still has some bugs and required but NYI features.

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Try push https://tbpl.mozilla.org/?tree=Try&rev=5adb3f27b9b4

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> Try push https://tbpl.mozilla.org/?tree=Try&rev=5adb3f27b9b4

These patches barfed because a vendor library added an |alpha| field to hwc_layer that's not in upstream.  The alpha field can only be used with RGB layers, and upcoming support for color layers allows directly setting an alpha value for the color.  So we decided to nuke the |alpha| extension in the vendor lib instead of trying to select the right struct layout at runtime.

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Round 2: https://tbpl.mozilla.org/?tree=Try&rev=0a72d6bcdc0c

Comment 43

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/mozilla-central/rev/c5fd901c5f95
https://hg.mozilla.org/mozilla-central/rev/cf8750abee06

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f142230d177
https://hg.mozilla.org/releases/mozilla-aurora/rev/59e8632b391e