Closed Bug 953303 Opened 6 years ago Closed 6 years ago

HwcComposer2D should honor HWC_HINT_CLEAR_FB set by hwcomposer.

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: sushilchauhan, Assigned: sushilchauhan)

Details

(Whiteboard: [CR 588749])

Attachments

(2 files, 3 obsolete files)

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
Attachment #8351588 - Flags: review?(dwilson)
For some reason, I am unable to assign any bug to myself.
Assignee: nobody → sushilchauhan
Whiteboard: [CR 588749]
I will re-base on Gecko tip due to bug # 959719.
Attachment #8351588 - Flags: review?(dwilson) → review?(ncameron)
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).
Attachment #8351588 - Attachment is obsolete: true
Attachment #8351588 - Flags: review?(ncameron)
Attachment #8360128 - Flags: review?(ncameron)
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.
Attachment #8360128 - Flags: review?(ncameron)
(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();
(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.
Flags: needinfo?(ncameron)
(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
Flags: needinfo?(ncameron)
(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 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
Attachment #8360128 - Flags: review+
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 ?
blocking-b2g: --- → 1.3?
Flags: needinfo?(ncameron)
Flags: needinfo?(dwilson)
(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).
Flags: needinfo?(ncameron)
Ok. I will make this change along with nits.
Flags: needinfo?(dwilson)
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();
Addressed review comments. Uploading HG friendly version of reviewed patch.
Attachment #8360128 - Attachment is obsolete: true
Attachment #8361841 - Flags: review+
Keywords: checkin-needed
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
This failed to compile on all platforms. Please ensure that it does before requesting checkin in the future.
Changed the attachment name & description to indicate it should land in v1.3 only.
Attachment #8361841 - Attachment is obsolete: true
Attachment #8361878 - Flags: review+
Uploaded patch for b2g master (v1.4). Diego, can you please run it on try server before we request for check-in ?
Attachment #8362036 - Flags: review+
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.
(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.
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"
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d6cee50cfd2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(v1.3+, needed to for proper camera preview function w/ hw comp for some devices)
blocking-b2g: 1.3? → 1.3+
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.
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.