981732 - HwcComposer2D: Unnecessary / hidden layer(s) are being composed.

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Sushil]

In video playback, I have observed we are composing a layer which is totally hidden under a full screen opaque layer. This will lead to unnecessary processing by HAL/drivers. Hence it will impact performance & power numbers. Here are observations:

Case 1: Layer[0] and Layer[1] are exact same full screen Color layers.
HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Color=ff14120e Blending=0x100 Flags=0x8
HWC layer[1]::ColorLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Color=ff14120e Blending=0x100 Flags=0x8
HWC layer[2]::ImageLayerComposite: dst=[0 265 480 535] src=[0.000000 0.000000 320.000000 180.000000] Tr=0 Blending=0x100 Flags=0x0

Case 2: Color layer (Layer[0]) is completely hidden under Thebes layer.
HWC layer[0]::ColorLayerComposite: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Color=ff14120e Blending=0x100 Flags=0x8
HWC layer[1]::ThebesLayer: dst=[0 0 480 800] src=[0.000000 0.000000 480.000000 800.000000] Tr=0 Blending=0x100 Flags=0x40
HWC layer[2]::ImageLayerComposite: dst=[0 265 480 535] src=[0.000000 0.000000 320.000000 180.000000] Tr=0 Blending=0x100 Flags=0x0

Possible solutions:
1. In both these 2 cases, ideally we should not have received Layer[0] in the Composition tree. It needs to be fixed in Rendering Phase (Painting).
OR
2. HwcComposer2D should not include Layer[0] for composition and drop it. But do we need to take care if the geometry of current frame has changed (as compared to last frame) ? I mean, drop Layer[0] from composition only if geometry of current frame is same as last frame. I believe it should not matter, because if a layer is hidden behind a full screen opaque layer, then it should not matter whether the geometry of current frame has changed or not.

Comment 1

[Andreas Gal :gal]

We have an optimization in place that should drop any layer behind a full screen opaque layer. That code must be broken. Can you create a test case for this?

Comment 2

[Sushil]

Test case: Video playback (device is in Portrait mode).

Comment 3

[Andreas Gal :gal]

I can't seem to find the code that is culling any layer behind a full screen opaque layer. Sushil, did we lose that as part of the kitkat porting?

Comment 4

[Sushil]

Andreas, we never had this code in HwcComposer2D. I am not aware of such a code in Layout or Rendering phase. Are you pointing to this: https://bugzilla.mozilla.org/show_bug.cgi?id=946952 ?

Comment 5

[Matt Woodrow (:mattwoodrow)]

I assume he means the code in ContainerLayerComposite::Render.

This looks like it will only work if the overlapping layers belong to the same ContainerLayer and have identity transforms.

I guess this layer tree doesn't fit that, so it's not having any effect.

Comment 6

[Sushil]

ContainerLayerComposite::RenderLayer() is not executed in HWC composition path. Hence, logic at [1] is not executed. I can add this logic in HwcComposer2D and check if it works.

[1]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp#314

Comment 7

[Sushil]

I tried to include the logic at [1] in HwcComposer2D::PrepareLayerList(), but it does not help. I think at HwcComposer2D level, it should not matter whether any layer(s) behind the full screen opaque layer belong to same ContainerLayer or not. We should be able to drop it safely from HWC layer list.
Andreas, which code did you refer to, in Comment 3 ?

[1]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp#322

Comment 8

[Andreas Gal :gal]

I think we used to have code to do this but I guess it was lost. You are right, we basically should walk the layer list in z order and stop and throw away the rest the moment we hit a full screen opaque layer.

Comment 9

[Sushil]

Looks good with an initial patch. I will do more tests, then upload patch.

Comment 10

[Sushil]

Attachment: HwcComposer2D should not compose any layer below full screen Opaque layer.

HwcComposer2D should not compose any layer below full-screen Opaque layer because it contributes to unnecessary processing in driver, which impacts performance and power consumption.

Comment 11

[Diego Wilson [:diego]]

Comment on attachment 8391350 [details] [diff] [review]
HwcComposer2D should not compose any layer below full screen Opaque layer.

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

Please r? again with addressed comments

::: widget/gonk/HwcComposer2D.cpp
@@ +300,5 @@
> +
> +    // Do not compose any layer below full-screen Opaque layer
> +    bool isOpaque = (opacity == 0xFF) && (aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE);
> +    if (current && isOpaque && (mScreenRect == nsIntRect(displayRect.left, displayRect.top,
> +        displayRect.right - displayRect.left, displayRect.bottom - displayRect.top))) {

What about using Contains() [1] instead? Also, can you please add a comment explaining that this can be generalized to non-fullscreen layers with a little more work?

[1] http://opengrok.qualcomm.com/source/xref/b2g_kk_3.5/gecko/gfx/2d/BaseRect.h#72

@@ +325,2 @@
>  #if ANDROID_VERSION >= 17
>      hwcLayer.compositionType = HWC_FRAMEBUFFER;

Please remove this unnecessary line change

Comment 12

[Sushil]

(In reply to Diego Wilson [:diego] from comment #11)
> Also, can you please add a comment
> explaining that this can be generalized to non-fullscreen layers with a
> little more work?
> 

I can add a comment. But that change is out of scope of this Bug and it has to consider other factors:
1. We can do it only when there is no scaling required on below layer(s).
2. Source crop also needs to be modified accordingly for below layer(s).
3. Layer transform (plus Y-Flip) of below layers also needs to be considered, etc.

Comment 13

[Sushil]

Attachment: HwcComposer2D should not compose any layer below full screen Opaque layer.

Uploading patch with review comments addressed.

Comment 14

[Diego Wilson [:diego]]

Comment on attachment 8391623 [details] [diff] [review]
HwcComposer2D should not compose any layer below full screen Opaque layer.

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

r=me with minor nit.

::: widget/gonk/HwcComposer2D.cpp
@@ +303,5 @@
> +    bool isOpaque = (opacity == 0xFF) && (aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE);
> +    if (current && isOpaque) {
> +        nsIntRect displayRect = nsIntRect(displayFrame.left, displayFrame.top,
> +            displayFrame.right - displayFrame.left, displayFrame.bottom - displayFrame.top);
> +        if (displayRect.Contains(mScreenRect)) {

nit: add explanation of what this does. Eg.
  //All previous layers are below the current layer
  //in the Z order. We can ignore them now.

Comment 15

[Sushil]

Attachment: HwcComposer2D should not compose any layer below full screen Opaque layer. r=dwilson

Uploaded HG friendly version of reviewed patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ae395a54630

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/edfa7c4abf1f

Comment 19

[Yeojin Chung [:YeojinC]]

No STR is present to create test case to address bug.