953303 - HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.

Status: RESOLVED FIXED

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

Description

[Sushil]

Attachment: HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.

During partial HWC Composition, if HWC_HINT_CLEAR_FB has been set by hwcomposer on an opaque HWC_OVERLAY layer, then HwcComposer2D should clear the visible rectangle of this layer on Frame Buffer with transparent pixels.

Here is the definition of HWC_HINT_CLEAR_FB from HWC header:
/*
 * HWC sets HWC_HINT_CLEAR_FB to tell SurfaceFlinger that it should clear the
 * framebuffer with transparent pixels where this layer would be.
 * SurfaceFlinger will only honor this flag when the layer has no blending
 *
 */
HWC_HINT_CLEAR_FB       = 0x00000002

Comment 1

[Sushil]

For some reason, I am unable to assign any bug to myself.

Comment 2

[Sushil]

I will re-base on Gecko tip due to bug # 959719.

Comment 3

[Sushil]

Attachment: HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.

Please review. I am uploading patch re-based on Gecko tip with a small conflict resolved in HwcComposer2D.cpp (due to patch in Bug # 959719).

Comment 4

[Nick Cameron [:nrc]]

Comment on attachment 8360128 [details] [diff] [review]
HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +321,5 @@
> +      if (layerToRender->GetClearFB()) {
> +          // Clear layer's visible rect on FrameBuffer with transparent pixels
> +          gfx::Rect aRect(clipRect.x, clipRect.y, clipRect.width, clipRect.height);
> +          compositor->clearFBRect(&aRect);
> +          layerToRender->SetClearFB(false);

nit: 2 space indent

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +774,5 @@
> +    return;
> +  }
> +
> +  mGLContext->fScissor(aRect->x, aRect->y, aRect->width, aRect->height);
> +  mGLContext->PushScissorRect();

Use a ScopedScissorRect

::: gfx/layers/opengl/CompositorOGL.h
@@ +252,5 @@
>     */
>    bool mFrameInProgress;
>  
> +  /*
> +   * Clear aRect on FrameBuffer with Open GL.

nit: remove "with Open GL"

::: widget/gonk/HwcComposer2D.cpp
@@ +507,5 @@
> +                        (mList->hwLayers[k].blending == HWC_BLENDING_NONE)) {
> +                        // Clear visible rect on FB with transparent pixels.
> +                        // Never clear the 1st layer since we're guaranteed
> +                        // that FB is already cleared.
> +                        mHwcLayerMap[k]->SetClearFB(true);

It seems like you can set this on any layer, but it only has an effect on container layers. On non-container layers, it silently does nothing. That seems bad. Is an invariant that we only have HWC_HINT_CLEAR_FB on container layers? If not, then you need to clear in those cases. IF so, then I suggest that the default {S,G}etClearFB implementation asserts, and we only do the sensible thing for container layers.

Comment 5

[Sushil]

(In reply to Nick Cameron [:nrc] from comment #4)
> Comment on attachment 8360128 [details] [diff] [review]
> HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.
> 
> Review of attachment 8360128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +774,5 @@
> > +    return;
> > +  }
> > +
> > +  mGLContext->fScissor(aRect->x, aRect->y, aRect->width, aRect->height);
> > +  mGLContext->PushScissorRect();
> 
> Use a ScopedScissorRect

I am not much familiar with Gfx code. Can you please tell me how to do "ScopedScissorRect" ? Should I call like this:
mGLContext->fEnable(LOCAL_GL_SCISSOR_TEST);
mGLContext->fScissor(aRect->x, aRect->y, aRect->width, aRect->height);
mGLContext->PushScissorRect();

Comment 6

[Sushil]

(In reply to Nick Cameron [:nrc] from comment #4)
> Comment on attachment 8360128 [details] [diff] [review]
> HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.
> 
> Review of attachment 8360128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +507,5 @@
> > +                        (mList->hwLayers[k].blending == HWC_BLENDING_NONE)) {
> > +                        // Clear visible rect on FB with transparent pixels.
> > +                        // Never clear the 1st layer since we're guaranteed
> > +                        // that FB is already cleared.
> > +                        mHwcLayerMap[k]->SetClearFB(true);
> 
> It seems like you can set this on any layer, but it only has an effect on
> container layers. On non-container layers, it silently does nothing. That
> seems bad. Is an invariant that we only have HWC_HINT_CLEAR_FB on container
> layers? If not, then you need to clear in those cases. IF so, then I suggest
> that the default {S,G}etClearFB implementation asserts, and we only do the
> sensible thing for container layers.

Nick,

It is actually opposite. Only visible non-container layers (leafs like Thebes, Image, Color, etc.) are included in HWC layer list. So mHwcLayerMap[k] always points to the corresponding non-container LayerComposite layer. Hence, SetClearFB() will never be called on a Container layer from HwcComposer2D, so this check will always be false if 'layerToRender' is a container layer, hence this will be No-OP for container layers:

In gfx/layers/composite/ContainerLayerComposite.cpp:
"if (layerToRender->GetClearFB()) {"

Moreover, "if (layerToRender->HasLayerBeenComposited()) {" check in same function will always be false for a container layer hence it will never enter into that block.

I had added this code in "ContainerLayerComposite.cpp" because this is a common entry point for each layer type like Thebes/Image/Color/Canvas otherwise I would have to dup same code in corresponding render function of each layer type.

Comment 7

[Nick Cameron [:nrc]]

(In reply to Sushil from comment #5)
> (In reply to Nick Cameron [:nrc] from comment #4)
> > Comment on attachment 8360128 [details] [diff] [review]
> > HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.
> > 
> > Review of attachment 8360128 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/opengl/CompositorOGL.cpp
> > @@ +774,5 @@
> > > +    return;
> > > +  }
> > > +
> > > +  mGLContext->fScissor(aRect->x, aRect->y, aRect->width, aRect->height);
> > > +  mGLContext->PushScissorRect();
> > 
> > Use a ScopedScissorRect
> 
> I am not much familiar with Gfx code. Can you please tell me how to do
> "ScopedScissorRect" ? Should I call like this:
> mGLContext->fEnable(LOCAL_GL_SCISSOR_TEST);
> mGLContext->fScissor(aRect->x, aRect->y, aRect->width, aRect->height);
> mGLContext->PushScissorRect();

ScopedScissorRect autoScissorRect(mGLContext, aRect->x, aRect->y, aRect->width, aRect->height);

should replace the fScissor and PushScissorRect lines. You shouldn't need the fEnable line

Comment 8

[Nick Cameron [:nrc]]

(In reply to Sushil from comment #6)
> (In reply to Nick Cameron [:nrc] from comment #4)
> > Comment on attachment 8360128 [details] [diff] [review]
> > HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.
> > 
> > Review of attachment 8360128 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/gonk/HwcComposer2D.cpp
> > @@ +507,5 @@
> > > +                        (mList->hwLayers[k].blending == HWC_BLENDING_NONE)) {
> > > +                        // Clear visible rect on FB with transparent pixels.
> > > +                        // Never clear the 1st layer since we're guaranteed
> > > +                        // that FB is already cleared.
> > > +                        mHwcLayerMap[k]->SetClearFB(true);
> > 
> > It seems like you can set this on any layer, but it only has an effect on
> > container layers. On non-container layers, it silently does nothing. That
> > seems bad. Is an invariant that we only have HWC_HINT_CLEAR_FB on container
> > layers? If not, then you need to clear in those cases. IF so, then I suggest
> > that the default {S,G}etClearFB implementation asserts, and we only do the
> > sensible thing for container layers.
> 
> Nick,
> 
> It is actually opposite. Only visible non-container layers (leafs like
> Thebes, Image, Color, etc.) are included in HWC layer list. So
> mHwcLayerMap[k] always points to the corresponding non-container
> LayerComposite layer. Hence, SetClearFB() will never be called on a
> Container layer from HwcComposer2D, so this check will always be false if
> 'layerToRender' is a container layer, hence this will be No-OP for container
> layers:
> 

Ok, makes sense, thanks for the clarification.

> In gfx/layers/composite/ContainerLayerComposite.cpp:
> "if (layerToRender->GetClearFB()) {"
> 
> Moreover, "if (layerToRender->HasLayerBeenComposited()) {" check in same
> function will always be false for a container layer hence it will never
> enter into that block.
> 
> I had added this code in "ContainerLayerComposite.cpp" because this is a
> common entry point for each layer type like Thebes/Image/Color/Canvas
> otherwise I would have to dup same code in corresponding render function of
> each layer type.

Comment 9

[Nick Cameron [:nrc]]

Comment on attachment 8360128 [details] [diff] [review]
HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.

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

r=me with the nits and scissor rect thing addressed

Comment 10

[Sushil]

Nick,
It seems "ScopedScissorRect" has been recently added and it is not defined in v1.3 (see Bug # 956401).

Diego, should we keep this bug on-hold until we move to v1.4 ?

Comment 11

[Nick Cameron [:nrc]]

(In reply to Sushil from comment #10)
> Nick,
> It seems "ScopedScissorRect" has been recently added and it is not defined
> in v1.3 (see Bug # 956401).
> 
> Diego, should we keep this bug on-hold until we move to v1.4 ?

You can still take this for 1.3, just use the push scissor rect code you were using (you need to also pop the scissor rect though).

Comment 12

[Sushil]

Ok. I will make this change along with nits.

Comment 13

[Sushil]

Yes, adding PopScissorRect() works fine too:

mGLContext->PushScissorRect();
mGLContext->fClearColor(0.0, 0.0, 0.0, 0.0);
mGLContext->fClear(LOCAL_GL_COLOR_BUFFER_BIT | LOCAL_GL_DEPTH_BUFFER_BIT);
mGLContext->PopScissorRect();

Comment 14

[Sushil]

Attachment: HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer. r=ncameron

Addressed review comments. Uploading HG friendly version of reviewed patch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/b2g-inbound/rev/60ede59eddea

https://tbpl.mozilla.org/php/getParsedLog.php?id=33184213&tree=B2g-Inbound

Comment 17

[Sushil]

It seems I need to upload separate patches for v1.3 and v1.4 because:
- PushScissorRect() and PopScissorRect() are not defined in v1.4
- ScopedScissorRect is not defined in v1.3

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This failed to compile on all platforms. Please ensure that it does before requesting checkin in the future.

Comment 19

[Sushil]

Attachment: HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer on b2g v1.3 . r=ncameron

Changed the attachment name & description to indicate it should land in v1.3 only.

Comment 20

[Sushil]

Attachment: HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer on b2g master (v1.4). r=ncameron

Uploaded patch for b2g master (v1.4). Diego, can you please run it on try server before we request for check-in ?

Comment 21

[Andreas Gal :gal]

Sushil, if you don't have a level 1 account yet please file a bug and I will vouch for you and you will get try-server access. If anyone else on your team needs access, please cc me on the bug.

Comment 22

[Sushil]

(In reply to Andreas Gal :gal from comment #21)
> Sushil, if you don't have a level 1 account yet please file a bug and I will
> vouch for you and you will get try-server access. If anyone else on your
> team needs access, please cc me on the bug.

Sure. Thanks.

Comment 23

[Sushil]

Ryan,

I have uploaded separate patches for b2g master and v1.3 as per my Comment 17.

1. For b2g master (v1.4): Attachment #8362036 [details] [diff]
- "HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer on b2g master (v1.4). r=ncameron"

2. For b2g v1.3: Attachment #8361878 [details] [diff]
- "HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer on b2g v1.3 . r=ncameron"

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d6cee50cfd2

Comment 26

[Michael Vines [:m1] [:evilmachines]]

(v1.3+, needed to for proper camera preview function w/ hw comp for some devices)

Comment 27

[Sushil]

Yes, it is needed in all use cases where an OVERLAY layer is sandwiched between opaque GPU layers on a device with H/W overlay.

Ryan, for b2g v1.3, you can pick the patch mentioned in Comment 23.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1951b06166e3