915729 - Add mixed HWC and GPU composition

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Diego Wilson [:diego]]

This is a follow up to bug 91139. We currently support composition of full gecko frames either via HwcComposer2D or OpenGL.

For devices that support multiple hardware layers (overlays) we can also composing some layers with OpenGL and some layers into HWC overlays within the same frame cycle.

Comment 1

[Diego Wilson [:diego]]

Attachment: Add-support-for-partial-MDP-Composition.patch

Comment 2

[Diego Wilson [:diego]]

There are three parts to this bug:

1. Add (or find) a way to mark layers composed in an HWC overlay as "no need to render in OpenGL". One proposal is in attachment 803793 [details] [diff] [review].
2. Split HwcComposer2D::TryRender() into to steps: Prepare() and Render(). Prepare() will identify and mark the layers to be composed in an Hwc Overlay. Render() will compose both the overlay layers and the gl layers rendered to the framebuffer surface.
3. Make GLContextProviderEGL [1] call HwcComposer2D::Render() instead of SwapBuffers().

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#600

Comment 3

[Sushil]

(In reply to Diego Wilson [:diego] from comment #2)
> There are three parts to this bug:
> 
> 2. Split HwcComposer2D::TryRender() into to steps: Prepare() and Render().
> Prepare() will identify and mark the layers to be composed in an Hwc
> Overlay. Render() will compose both the overlay layers and the gl layers
> rendered to the framebuffer surface.
> 3. Make GLContextProviderEGL [1] call HwcComposer2D::Render() instead of
> SwapBuffers().
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.
> cpp#600

It sounds good. It can handle both full and partial HWC Composition paths. In Render():
1. Call GetGonkDisplay()->SwapBuffers().
2. Then, for "JB only", do stuff similar to GonkDisplayJB::Post() but do not re-populate layer[0]. Set framebuffer layer handle and acquireFenceFd, do not call prepare and take care of release fences after set call.

Also, need to remove "return Post()" from GonkDisplayJB::SwapBuffers().

Comment 4

[Sushil]

Attachment: Add support for partial HWC Composition.

Re-based my previous patch on tip. Here is the overview:

If all the layers in frame cannot be composed using HWC, try for partial HWC Composition. Skip GPU rendering for the layers marked with OVERLAY during partial HWC composition. These layers will be composed by OVERLAY. All layers marked with GPU get composed on FrameBuffer layer. Eventually, OVERLAY layers and FB layer are drawn in HWC set.

Comment 5

[Peter Chang[:pchang]]

Comment on attachment 810267 [details] [diff] [review]
Add support for partial HWC Composition.

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +198,4 @@
>        continue;
>      }
>  
> +    if (!layerToRender->GetLayer()->GetSkipGPU()) {

I prefer to rename the skipGpuRender flag indicate "this layer was rendered" and  add it inside LayerComposite class.

Because Layer class is more generic and is not only used for composition.

::: widget/gonk/HwcComposer2D.h
@@ +24,4 @@
>  
>  #include <hardware/hwcomposer.h>
>  
> +#define MAX_HWC_LAYERS 32

Why do you define the maximum hwc layers as 32?

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 810267 [details] [diff] [review]
Add support for partial HWC Composition.

Nick is probably a better reviewer for layers code than I am.

Comment 7

[Nick Cameron [:nrc]]

Comment on attachment 810267 [details] [diff] [review]
Add support for partial HWC Composition.

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

Looks good. I'd like to see a new version before r+ing.

::: gfx/layers/Layers.h
@@ +1252,4 @@
>  
>    virtual LayerRenderState GetRenderState() { return LayerRenderState(); }
>  
> +  void SetSkipGPU(bool value);

Lets add all this new API to LayerComposite, rather than Layer since it does not apply to other kinds of Layers.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +198,4 @@
>        continue;
>      }
>  
> +    if (!layerToRender->GetLayer()->GetSkipGPU()) {

Put the positive branch first.

Also, what pchang said about renaming.

::: widget/gonk/HwcComposer2D.cpp
@@ +445,4 @@
>      hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = {NULL};
>      const hwc_rect_t r = {0, 0, mScreenRect.width, mScreenRect.height};
>      int idx = mList->numHwLayers;
> +    bool fullHwcComp = true;

add a comment saying what a partial vs full composite is. Also change name to 'fullHwcComposite' - always prefer a longer name if it makes things clearer.

@@ +470,3 @@
>  
>      for (int j = 0; j < idx; j++) {
>          if (mList->hwLayers[j].compositionType == HWC_FRAMEBUFFER) {

I'm not sure what is going on here - if the composition type is HWC_FRAMEBUFFER, then we _can't_ composite? This seems backwards to me, but it is probably that the meaning is not clear from the flag name. Could you express this differently? If not, please add a comment.

@@ +479,5 @@
> +    if (!fullHwcComp) {
> +        // For partial HWC Comp, skip GPU rendering of OVERLAY layers
> +        for (int k=0; k < idx; k++) {
> +            if (mList->hwLayers[k].compositionType == HWC_OVERLAY)
> +                mLeafLayerMap[k]->SetSkipGPU(true);

{} please

Comment 8

[Sushil]

(In reply to peter chang[:pchang] from comment #5)
> Comment on attachment 810267 [details] [diff] [review]
> Add support for partial HWC Composition.
> 
> Review of attachment 810267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.h
> @@ +24,4 @@
> >  
> >  #include <hardware/hwcomposer.h>
> >  
> > +#define MAX_HWC_LAYERS 32
> 
> Why do you define the maximum hwc layers as 32?

Because HAL supports up to 32 HWC layers. If more than 32, we fallback to GPU.

Comment 9

[Sushil]

(In reply to Nick Cameron [:nrc] from comment #7)
> Comment on attachment 810267 [details] [diff] [review]
> Add support for partial HWC Composition.
> 
> Review of attachment 810267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I'd like to see a new version before r+ing.
> 
> >      for (int j = 0; j < idx; j++) {
> >          if (mList->hwLayers[j].compositionType == HWC_FRAMEBUFFER) {
> 
> I'm not sure what is going on here - if the composition type is
> HWC_FRAMEBUFFER, then we _can't_ composite? This seems backwards to me, but
> it is probably that the meaning is not clear from the flag name. Could you
> express this differently? If not, please add a comment.
> 
During hwc prepare, HAL marks HWC layers as HWC_FRAMEBUFFER (for GPU) or HWC_OVERLAY (for OVERLAY). So after hwc prepare, if there is any layer marked for GPU, it means the frame is not going for full HWC Composition. So all layers marked for GPU, get rendered on FBSurface. Hence, in Render() call, FB layer gets composed (along with OVERLAY layers if it is partial HWC). I will add a comment in code as well. Please see Bug # 919676 for Render() call.

Comment 10

[Sushil]

Thanks. I will work on review comments and update the patch.

Comment 11

[Sotaro Ikeda [:sotaro]]

set "1.3?" flag. This is important to get good performance on video playback by using Web Browser app.

Comment 12

[Diego Wilson [:diego]]

(In reply to Sushil from comment #8)
> Because HAL supports up to 32 HWC layers. If more than 32, we fallback to
> GPU.

Is there a way to obtain this limit dynamically? Or maybe we can just rely on libhwcomposer to fail gracefully if more than the limit is used?

Comment 13

[Sushil]

We can rely on HAL to fail if HWC layers are more than maximum supported layers. But the main motive of adding/checking it in HwcComposer2D is that it also defines the size of these members:
- mLeafLayerMap[MAX_HWC_LAYERS]
- mPrevRelFdand[MAX_HWC_LAYERS + 1]

Comment 14

[Sushil]

Typo in previous comment: mPrevRelFd[MAX_HWC_LAYERS + 1]

Comment 15

[Diego Wilson [:diego]]

That should really be a vector like we use for visible rects [1]. It's probably beyond the scope of this review.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.h#38

Comment 16

[Diego Wilson [:diego]]

Comment on attachment 810267 [details] [diff] [review]
Add support for partial HWC Composition.

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

Looks like this patch needs a rebase.

::: widget/gonk/HwcComposer2D.h
@@ +81,4 @@
>      //to render the current frame
>      std::list<RectVector>   mVisibleRegions;
>      int                     mPrevRelFd[MAX_HWC_LAYERS + 1];
> +    layers::Layer*          mLeafLayerMap[MAX_HWC_LAYERS];

Maybe you can try converting both of the above to vectors? See RectVector for an example.

Comment 17

[Sushil]

(In reply to Diego Wilson [:diego] from comment #16)
> Comment on attachment 810267 [details] [diff] [review]
> Add support for partial HWC Composition.
> 
> Review of attachment 810267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like this patch needs a rebase.
> 

Yes, I will upload the re-based patch today.

Comment 18

[Sushil]

Attachment: Add support for partial HWC Composition.

Following updates in latest patch. Please review.
1. Addressed review comments and added the new API in LayerComposite class (instead of Layer).
2. Removed MAX_HWC_LAYERS limit from HwcComposer2D. HAL takes care of it.
3. "mPrevRelFds" and "mHwcLayerMap" are lists now (no more arrays).

Comment 19

[Nick Cameron [:nrc]]

Hi, I'm at the Mozilla summit right now and then travelling. I should get to these reviews on Tuesday.

Comment 20

[Peter Chang[:pchang]]

Comment on attachment 813793 [details] [diff] [review]
Add support for partial HWC Composition.

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +198,5 @@
>        continue;
>      }
>  
> +    if (layerToRender->GetHwcRenderUsage()) {
> +      layerToRender->SetHwcRenderUsage(false);

Please add comment here that we skip composition this time and reset the composition flag for next composition phase.

::: gfx/layers/composite/LayerManagerComposite.h
@@ +404,4 @@
>    bool mUseShadowClipRect;
>    bool mShadowTransformSetByAnimation;
>    bool mDestroyed;
> +  bool mUseHwcRender;

This class is related to composition, I prefer not to put Hwc logic inside this class.
How about rename to mLayerComposited?
Then you can query/set with IsLayerComposited/SetLayerComposited(bool).

Comment 21

[Nick Cameron [:nrc]]

Comment on attachment 813793 [details] [diff] [review]
Add support for partial HWC Composition.

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

::: widget/gonk/HwcComposer2D.cpp
@@ +434,4 @@
>          hwcLayer.transform = colorLayer->GetColor().Packed();
>      }
>  
> +    mHwcLayerMap.push_back(static_cast<LayerComposite*>(aLayer->ImplData()));

Can we change HwcComposer to operate only on LayerComposite rather than Layer? Don't do this if it is a lot of work, but if it is only changing a few method sigs, then it is worth it.

@@ +466,3 @@
>      for (int j = 0; j < idx; j++) {
>          if (mList->hwLayers[j].compositionType == HWC_FRAMEBUFFER) {
> +            // After prepare, if any HWC_FRAMEBUFFER layer, it means

I think this comment is missing a verb, it does not make sense to me.

::: widget/gonk/HwcComposer2D.h
@@ +81,4 @@
>      //Holds all the dynamically allocated RectVectors needed
>      //to render the current frame
>      std::list<RectVector>   mVisibleRegions;
> +    std::list<int>          mPrevRelFds;

May as well make the name more explanatory whilst we're changing the type.

@@ +81,5 @@
>      //Holds all the dynamically allocated RectVectors needed
>      //to render the current frame
>      std::list<RectVector>   mVisibleRegions;
> +    std::list<int>          mPrevRelFds;
> +    std::list<layers::LayerComposite*> mHwcLayerMap;

Is there a reason to use STL rather than Mozilla data structures? Probably better to use nsTArray here if you can.

Comment 22

[Sushil]

(In reply to Nick Cameron [:nrc] from comment #21)
> Comment on attachment 813793 [details] [diff] [review]
> Add support for partial HWC Composition.
> 
> Review of attachment 813793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +434,4 @@
> > +    mHwcLayerMap.push_back(static_cast<LayerComposite*>(aLayer->ImplData()));
> 
> Can we change HwcComposer to operate only on LayerComposite rather than
> Layer? Don't do this if it is a lot of work, but if it is only changing a
> few method sigs, then it is worth it.

We can discuss this in a new bug. We might want to check if we need to have it on Firefox OS 1.1, 1.2 and 1.3 plus make sure that we do not break any existing supported device.

Comment 23

[Sushil]

Attachment: Add support for partial HWC Composition.

Comment 24

[Sushil]

Attachment: Add support for partial HWC Composition.

Address review comments. Diego, please test on ICS device.

Comment 25

[Nick Cameron [:nrc]]

Comment on attachment 815195 [details] [diff] [review]
Add support for partial HWC Composition.

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

r=me with the nits addressed

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +198,5 @@
>        continue;
>      }
>  
> +    if (layerToRender->IsLayerComposited()) {
> +      // HWC will compose this layer so skip GPU composition this

s/HWC/Composer2D

::: gfx/layers/composite/LayerManagerComposite.h
@@ +391,5 @@
>    const nsIntRect* GetShadowClipRect() { return mUseShadowClipRect ? &mShadowClipRect : nullptr; }
>    const nsIntRegion& GetShadowVisibleRegion() { return mShadowVisibleRegion; }
>    const gfx3DMatrix& GetShadowTransform() { return mShadowTransform; }
>    bool GetShadowTransformSetByAnimation() { return mShadowTransformSetByAnimation; }
> +  bool IsLayerComposited() { return mLayerComposited; }

s/IsLayerComposited/HasLayerBeenComposited

::: widget/gonk/HwcComposer2D.cpp
@@ +434,4 @@
>          hwcLayer.transform = colorLayer->GetColor().Packed();
>      }
>  
> +    mHwcLayerMap.AppendElement(static_cast<LayerComposite*>(aLayer->ImplData()));

probably worth asserting that mHwcLayerMap is empty at the start of PrepareLayerList

@@ +466,3 @@
>      for (int j = 0; j < idx; j++) {
>          if (mList->hwLayers[j].compositionType == HWC_FRAMEBUFFER) {
> +            // After prepare, if there is a HWC_FRAMEBUFFER layer, it

s/a/an

@@ +624,4 @@
>      MOZ_ASSERT(Initialized());
>      if (mList) {
>          mList->numHwLayers = 0;
> +        mHwcLayerMap.Clear();

please assert mHwcLayerMap is empty as a postcondition for TryRender - it looks like it always is, but I would like to assert that

Comment 26

[Sushil]

(In reply to Nick Cameron [:nrc] from comment #25)
> Comment on attachment 815195 [details] [diff] [review]
> Add support for partial HWC Composition.
> 
> Review of attachment 815195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the nits addressed
> 
> @@ +434,4 @@
> >          hwcLayer.transform = colorLayer->GetColor().Packed();
> >      }
> >  
> > +    mHwcLayerMap.AppendElement(static_cast<LayerComposite*>(aLayer->ImplData()));
> 
> probably worth asserting that mHwcLayerMap is empty at the start of
> PrepareLayerList

PrepareLayerList() is recursive so I will add an ASSERT in TryRender, mentioned in next comment.

> @@ +624,4 @@
> >      MOZ_ASSERT(Initialized());
> >      if (mList) {
> >          mList->numHwLayers = 0;
> > +        mHwcLayerMap.Clear();
> 
> please assert mHwcLayerMap is empty as a postcondition for TryRender - it
> looks like it always is, but I would like to assert that

I will pull Clear() out of this 'mList" check and will add "mHwcLayerMap is empty" assert after doing Clear().

Comment 27

[Sushil]

@nrc: Sorry, did you mean to add ASSERT at end of TryRender (postcondition), in that case I will have to add it at 3 places: PrepareLayerList fails, TryHwComposition fails and TryHwComposition pass. Please comment.

Comment 28

[Sushil]

I would like to get info for my last comments.

Comment 29

[Nick Cameron [:nrc]]

(In reply to Sushil from comment #28)
> I would like to get info for my last comments.

Sorry, I'm in Europe right now, so timezones are inconvenient. In general, use needinfo if you need info :-)

Comment 30

[Nick Cameron [:nrc]]

So, if you are going to move the clear out of the if(mList) block, then you don't need to assert right after it - that is pointless. Perhaps you should just assert that mHwcLayerMap is empty before the first call to PrepareLayerList? I think it is enough to know that, rather than check all the early returns. If you are feeling fancy you could use an RAII class to either call clear or assert the emptiness, but that is probably excessive.

Comment 31

[Sushil]

Sounds good. I will add ASSERT before PrepareLayerList call in TryRender, no need to move clear.

Comment 32

[Diego Wilson [:diego]]

Sushil, the patch still needs a rebase

Comment 33

[Sushil]

Yes, I will re-base and upload HG friendly patch.

Comment 34

[Sushil]

Attachment: Bug 915729 - Add mixed HWC and GPU Composition. r=ncameron

Addressed last review comments. Uploading HG friendly patch.

Comment 35

[Diego Wilson [:diego]]

Comment on attachment 817467 [details] [diff] [review]
Bug 915729 - Add mixed HWC and GPU Composition. r=ncameron

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

Verified on ICS target

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bdce774ee886

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bdce774ee886

Comment 38

[Jason Smith [:jsmith]]

Clearing nom - this is already landed, don't need to worry about triaging this