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

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(10 attachments, 4 obsolete attachments)

1.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.33 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.79 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.16 KB, patch
mwu
: review+
Details | Diff | Splinter Review
3.05 KB, patch
cjones
: review+
Details | Diff | Splinter Review
3.11 KB, patch
Details | Diff | Splinter Review
5.72 KB, patch
Details | Diff | Splinter Review
6.07 KB, patch
Details | Diff | Splinter Review
3.43 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
13.82 KB, patch
cjones
: review+
Details | Diff | Splinter Review
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.
Need to set this blocking-basecamp+.  Ping me in private for rationale.
blocking-basecamp: --- → +

Comment 2

6 years ago
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.
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.
(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
Created attachment 674531 [details] [diff] [review]
part 0: Add a dynamic cast to ColorLayer*
Assignee: nobody → jones.chris.g
Created attachment 674532 [details] [diff] [review]
part 1: Add a Composer2D interface to enable implementations to more efficiently compose layer trees
Attachment #674532 - Flags: superreview?(roc)
Created attachment 674533 [details] [diff] [review]
part 2: Let widgets expose Composer2Ds, if they have them
Attachment #674533 - Flags: superreview?(roc)
Created attachment 674535 [details] [diff] [review]
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.
Attachment #674535 - Flags: review?(matt.woodrow)
Created attachment 674536 [details] [diff] [review]
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.
Attachment #674536 - Flags: review?(matt.woodrow)
Attachment #674536 - Flags: feedback?(ncameron)
Created attachment 674537 [details] [diff] [review]
part 5: Implement all the goop to let widget/gonk use a Composer2D (HwcComposer2D)
Attachment #674537 - Flags: review?(mwu)
Created attachment 674538 [details] [diff] [review]
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 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.
Attachment #674532 - Flags: superreview?(roc) → superreview+
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.
(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.
(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 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.
Attachment #674535 - Flags: feedback?(ncameron)
(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.
Created attachment 674569 [details] [diff] [review]
part 1: Add a Composer2D interface to enable implementations to more efficiently compose layer trees, v2

Update, carrying sr=roc.
Attachment #674532 - Attachment is obsolete: true
Attachment #674569 - Flags: review+
(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 on attachment 674533 [details] [diff] [review]
part 2: Let widgets expose Composer2Ds, if they have them

sr? re-request.
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.
Attachment #674533 - Attachment is obsolete: true
Attachment #674533 - Flags: superreview?(roc)
Created attachment 674576 [details] [diff] [review]
part 2: Let widgets expose Composer2Ds, if they have them, v2
Attachment #674576 - Flags: superreview?(roc)
Attachment #674576 - Flags: superreview?(roc) → superreview+

Comment 23

6 years ago
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.
Attachment #674537 - Flags: review?(mwu) → review+
Attachment #674535 - Flags: review?(matt.woodrow) → review+
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?
Attachment #674536 - Flags: review?(matt.woodrow) → review+
That's correct.  The goal of this assertion is, no matter what happens in the code above, we never take the GL Render() path.
(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

6 years ago
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
Attachment #674536 - Flags: feedback?(ncameron) → feedback+

Comment 28

6 years ago
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.
Attachment #674535 - Flags: feedback?(ncameron) → feedback+

Updated

6 years ago
Priority: -- → P1
Created attachment 676875 [details] [diff] [review]
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.
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.
Created attachment 676881 [details] [diff] [review]
Testcase 1: old homescreen, v2

Fixed a small bug.  Same instructions for applying and running.
Created attachment 676882 [details] [diff] [review]
Testcase 2: New homescreen

Like the old one but canvas-ized.  Same instructions to apply, install, and run.
Attachment #676875 - Attachment is obsolete: true
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.
(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.
(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?
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 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
Created attachment 680028 [details] [diff] [review]
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.
Attachment #680028 - Flags: review?(matt.woodrow)
Attachment #680028 - Flags: review?(matt.woodrow) → review+
Created attachment 680282 [details] [diff] [review]
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.
Attachment #674538 - Attachment is obsolete: true
Attachment #680282 - Flags: review+
(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.

Updated

5 years ago
Blocks: 913593

Updated

3 years ago
Depends on: 1246146
You need to log in before you can comment on or make changes to this bug.