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.

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?

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

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?

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 ?

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.

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

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

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.

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

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 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

(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.

Uploading patch with review comments addressed.

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.

Uploaded HG friendly version of reviewed patch.

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