1027343 - Normal home screen pan has stutter effect.

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sushil]

STR:
1. As vertical scrollable Home Screen is set by default on v2.0, so switch to normal Home screen (non-scrolling, horizontal) from Settings.
2. Pan the home screen once.
3. Observe the stutters during home screen pan.

I noticed, it normally happens only when HWC is enabled. If I disable HWC, I do not observe stutters. I checked there is not much time taken in HWC prepare and set calls. But I noticed that at the beginning and end of a HomeScreen pan, few TryRender() calls are being made after huge time intervals.

Comment 1

[Sushil]

Sotaro, 

It seems to be a regression on v2.0 since I did not observe these stutters on v1.4 build. Can you please check, which recent change should have introduced it? It is on H/W overlay device.

Comment 2

[Sushil]

This issue seems to be related to HWC_GEOMETRY_CHANGED flag. I do not see the stutter effect, if I always set the HWC_GEOMETRY_CHANGED flag at: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#184. I will debug it, assigning to myself.

Comment 3

[Sushil]

I checked it. During home screen pan, the stutter effect is observed because for few frames, we inform HAL that the layer tree geometry has not changed but it has changed because destination rect and source crop of few layers have changed in frame. Here are 2 consecutive frames:

Frame 1:
E/HWComposer(  219): setHwcGeometry: aGeometryChanged = False
E/HWComposer(  219): HWC layer[0]::ThebesLayerComposite: dst=[0 0 720 1280] src=[0 0 720 1280] Tr=0 Blend=0x100 Flags=0x40
E/HWComposer(  219): HWC layer[1]::ThebesLayerComposite: dst=[0 48 720 1130] src=[0 0 720 1082] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[2]::ThebesLayerComposite: dst=[0 1278 2 1280] src=[0 0 2 2] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[3]::ThebesLayerComposite: dst=[0 48 77 339] src=[643 0 720 291] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[4]::ThebesLayerComposite: dst=[87 64 720 1007] src=[0 0 633 943] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[5]::ColorLayerComposite: dst=[79 48 319 54] src=[0 0 240 6] Color=ff027fff Blend=0x100 Flags=0x8
E/HWComposer(  219): HWC layer[6]::ThebesLayerComposite: dst=[0 1130 720 1280] src=[0 0 720 150] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[7]::ThebesLayerComposite: dst=[437 3 703 40] src=[2 0 268 37] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): Frame rendered

Frame 2:
E/HWComposer(  219): setHwcGeometry: HWC_GEOMETRY_CHANGED = False
E/HWComposer(  219): HWC layer[0]::ThebesLayerComposite: dst=[0 0 720 1280] src=[0 0 720 1280] Tr=0 Blend=0x100 Flags=0x40
E/HWComposer(  219): HWC layer[1]::ThebesLayerComposite: dst=[0 48 720 1130] src=[0 0 720 1082] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[2]::ThebesLayerComposite: dst=[0 1278 2 1280] src=[0 0 2 2] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[3]::ThebesLayerComposite: dst=[0 48 51 339] src=[669 0 720 291] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[4]::ThebesLayerComposite: dst=[61 64 720 1007] src=[0 0 659 943] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[5]::ColorLayerComposite: dst=[104 48 344 54] src=[0 0 240 6] Color=ff027fff Blend=0x100 Flags=0x8
E/HWComposer(  219): HWC layer[6]::ThebesLayerComposite: dst=[0 1130 720 1280] src=[0 0 720 150] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): HWC layer[7]::ThebesLayerComposite: dst=[437 3 703 40] src=[2 0 268 37] Tr=0 Blend=0x105 Flags=0x40
E/HWComposer(  219): Frame rendered

As per above logs, HWC layers [3], [4] and [5] have moved in Frame 2 as compared to Frame 1, but we are reporting that layer tree geometry has not changed. This is the cause of stutter effect.

Comment 4

[Sushil]

Matt,

Can you please help me to point out if I have missed some condition in LayerTreeInvalidation.cpp in the Bugzilla patch: https://bugzilla.mozilla.org/show_bug.cgi?id=965102. As per Comment 3 above, HWC layers [3], [4] and [5] have moved in Frame 2 as compared to Frame 1. So layer tree geometry has changed but I am reporting that layer tree geometry has not changed. This is the cause of stutter effect.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Where is this logging code? I can't find it in the tree. Where are the dst and src rectangles coming from with our layers?

For [5], the src area stays the same but the dst changes. Does this mean the transform changed? That should definitely hit this code here, http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#152

Comment 6

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Where is this logging code? I can't find it in the tree. Where are the dst
> and src rectangles coming from with our layers?

I used this LOGE at http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#505 to dump dst, src and other info for each HWC layer:

LOGE("HWC layer[%d]::%s: dst=[%d %d %d %d] src=[%d %d %d %d] Tr=%x Blend=0x%x Flags=0x%x",
mList->numHwLayers, aLayer->Name(), hwcLayer.displayFrame.left, hwcLayer.displayFrame.top, hwcLayer.displayFrame.right, hwcLayer.displayFrame.bottom, (int)hwcLayer.sourceCropf.left, (int)hwcLayer.sourceCropf.top, (int)hwcLayer.sourceCropf.right, (int)hwcLayer.sourceCropf.bottom, hwcLayer.transform, hwcLayer.blending, hwcLayer.flags);

> For [5], the src area stays the same but the dst changes. Does this mean the
> transform changed? That should definitely hit this code here,
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/
> LayerTreeInvalidation.cpp#152

No, the transform has not changed. It is Color layer and it has moved towards right, that's why left and right co-ordinates of dst have increased. Do you think Color layer should have hit the code, which you mentioned?

I suspect 1 of the following:
1. Does LayerTreeInvalidation.cpp track the change in GetEffectiveVisibleRegion() of a layer ?
2. If entire layer tree is new in Frame 2, and I am missing that in LayerManagerComposite code. How to find out if this layer tree is new/re-created (as compared to last frame) ?
3. state.mOffset.x, state.mOffset.y, state.mSize.width and state.mSize.height used at [1] but LayerTreeInvalidation.cpp is not aware about them. I will check, if this is the case here.

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

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #6)

> I suspect 1 of the following:
> 1. Does LayerTreeInvalidation.cpp track the change in
> GetEffectiveVisibleRegion() of a layer ?

No.

> 2. If entire layer tree is new in Frame 2, and I am missing that in
> LayerManagerComposite code. How to find out if this layer tree is
> new/re-created (as compared to last frame) ?

Set aGeometryChanged to true in this block http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#295

> 3. state.mOffset.x, state.mOffset.y, state.mSize.width and
> state.mSize.height used at [1] but LayerTreeInvalidation.cpp is not aware
> about them. I will check, if this is the case here.

You may also want to check clip rect changes - http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#176

Comment 8

[Sushil]

Yes, I had checked these 2 places and I have checked it again now. It does not not hit these 2 places in entire home screen pan :(
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#295
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#176

I also checked by setting aGeometryChanged = true, in below blocks but they are not being hit in entire home screen pan:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#341
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#367
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#376

Comment 9

[Sushil]

On b2g v2.0 tip, it seems the option to switch to Normal home screen has been removed from Settings. Sotaro, do you know, if there is any other way to set normal home screen ? This issue is not reproducible on Vertical home screen.

Comment 10

[Sushil]

Got it on another use case.

Comment 11

[Sushil]

Matt,

What is the significance of LayerManagerComposite::EndTransaction() call with (aFlags & END_NO_IMMEDIATE_REDRAW) as True at [1]? Does it indicate that the layer tree has been re-created OR a new layer tree? If that is the case, then I should set mGeometryChanged as True, when this happens.

We hit this condition once when Volume pop-up becomes invisible during Video playback hence the layer tree geometry should change. But on the other hand, it gets hit on every frame in Crystal Skull App which does not make sense because Canvas layer is the only layer, on which the handles are updating. All other layer handles are not updating and no layer is moving in frame. So the layer tree geometry should not change in Crystal Skull App. Is the app forcing to create a new layer tree on every frame ?

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

Comment 12

[Matt Woodrow (:mattwoodrow)]

END_NO_IMMEDIATE_REDRAW is used any time we make a modification to the layer tree, it doesn't imply anything specific about the type of changes that were made.

Comment 13

[Sushil]

Thanks for confirmation. Then I should set mGeometryChanged to true, whenever it happens.

Comment 14

[Sushil]

Attachment: Geometry change flag needs to be set for modified layer tree.

The geometry change flag needs to be set when layer tree itself has been modified and handle more cases of layer tree geometry change in layer tree invalidation. Reset the invalid region when HWC composition is done.

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #13)
> Thanks for confirmation. Then I should set mGeometryChanged to true,
> whenever it happens.

I don't think you should, since it happens for *every* layer tree change. It doesn't seem like mGeometryChanged would be false very often in that case.

Comment 16

[Sushil]

Comment on attachment 8449888 [details] [diff] [review]
Geometry change flag needs to be set for modified layer tree.

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +248,5 @@
>      Render();
>      mGeometryChanged = false;
> +  } else {
> +    // Modified layer tree
> +    mGeometryChanged = true;

@Matt,

mGeometryChanged will still be false in use cases where only handles are updating like Video playback (portrait/landscape), Camera/Camcorder preview, Video playback frames when time/seek-bar is not updating, Camcorder recording frames when timer is not updating, etc.

It is needed because when the layer tree gets modified with END_NO_IMMEDIATE_REDRAW flag, ComputeDifferences() does not set mGeometryChanged most of the times, so we need to set it explicitly to inform HWC in next Render() call. For ex: During video playback (with video controls not visible), press volume button. When the Volume Bar goes invisible, the frame geometry has changed since "Volume bar" layer is removed from the layer tree. Hence, END_NO_IMMEDIATE_REDRAW flag is set but I see ComputeDifferences() call does not set mGeometryChanged to "true" in current / next round, which is wrong.

Comment 17

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #16)
> Comment on attachment 8449888 [details] [diff] [review]
> Geometry change flag needs to be set for modified layer tree.
> 
> Review of attachment 8449888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +248,5 @@
> >      Render();
> >      mGeometryChanged = false;
> > +  } else {
> > +    // Modified layer tree
> > +    mGeometryChanged = true;
> 
> @Matt,
> 
> mGeometryChanged will still be false in use cases where only handles are
> updating like Video playback (portrait/landscape), Camera/Camcorder preview,
> Video playback frames when time/seek-bar is not updating, Camcorder
> recording frames when timer is not updating, etc.
> 
> It is needed because when the layer tree gets modified with
> END_NO_IMMEDIATE_REDRAW flag, ComputeDifferences() does not set
> mGeometryChanged most of the times, so we need to set it explicitly to
> inform HWC in next Render() call. For ex: During video playback (with video
> controls not visible), press volume button. When the Volume Bar goes
> invisible, the frame geometry has changed since "Volume bar" layer is
> removed from the layer tree. Hence, END_NO_IMMEDIATE_REDRAW flag is set but
> I see ComputeDifferences() call does not set mGeometryChanged to "true" in
> current / next round, which is wrong.

Ok so there's 3 different ways we can make changes to the layer tree:

* CompositorParent::RecvUpdate
  * This always calls EndTransaction(END_NO_IMMEDIATE_REDRAW)
  * Often makes geometry changes, but not necessarily. We might just change the content of a ThebesLayer or similar

* ImageBridgeParent::RecvUpdate
  * Doesn't call EndTransaction()
  * Won't ever change anything that would require mGeometryChanged = true

* AsyncCompositionManager::TransformShadowTree
  * Applies async scrolling and async animations changes


If you're happy to always set mGeometryChanged = true when we get CompositorParent::RecvUpdate, then your patch is ok.

It might also be simpler to remove all the mGeometryChanged code from LayerTreeProperties::ComputeDifferences and instead do it within TransformShadow tree instead.

Comment 18

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)

> It might also be simpler to remove all the mGeometryChanged code from
> LayerTreeProperties::ComputeDifferences and instead do it within
> TransformShadow tree instead.

I checked AsyncCompositionManager::TransformShadowTree(). During Video playback (when Video controls are not visible), there is no geometry change in consecutive frames and only handles get updated but ApplyAsyncContentTransformToTree() call at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#922 always returns "True". So I do not think this approach will help us to find out if there is no geometry change in layer tree. Plz let me know, if I missed something here. I think LayerTreeProperties::ComputeDifferences approach is fine.

> 
> If you're happy to always set mGeometryChanged = true when we get
> CompositorParent::RecvUpdate, then your patch is ok.

This patch fixes all the cases when HWC expects geometry change but we used to inform that there is no geometry change in the layer tree. For ex: Stutter effect during normal Home screen pan, Volume bar coming up and going down during Video playback, etc. The idea is that we should never have a case where the layer tree geometry changes but we inform HWC that it has not changed. This will lead to issues like stutter effect, stale content, etc. On the other hand, there is no harm in informing HWC that the layer tree geometry has changed in some corner cases due to END_NO_IMMEDIATE_REDRAW flag.

Comment 19

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #18)

> I checked AsyncCompositionManager::TransformShadowTree(). During Video
> playback (when Video controls are not visible), there is no geometry change
> in consecutive frames and only handles get updated but
> ApplyAsyncContentTransformToTree() call at
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> AsyncCompositionManager.cpp#922 always returns "True". So I do not think
> this approach will help us to find out if there is no geometry change in
> layer tree. Plz let me know, if I missed something here. I think
> LayerTreeProperties::ComputeDifferences approach is fine.

Yeah, I don't think the return value is sufficient. You'd need to add code into the function to detect actual changes.

Sticking with ComputeDifferences is fine.

Comment 20

[Sushil]

Attachment: Geometry change flag needs to be set for modified layer tree. r=mattwoodrow

Uploaded HG friendly version of reviewed patch.

Comment 21

[Sushil]

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

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Sushil from comment #21)
> https://hg.mozilla.org/integration/b2g-inbound/rev/3974df6a17f1

i guess this doesn't need checkin needed since its now checked-in right ?

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3974df6a17f1

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Sushil - Can you please comment on why you think this bug blocks bug 1011657?

Comment 25

[Sushil]

(In reply to Lawrence Mandel [:lmandel] from comment #24)
> Sushil - Can you please comment on why you think this bug blocks bug 1011657?

This bug fixes the cases when there is geometry change in the layer tree but we were informing HWC that there is no geometry change. Example of such cases: Few frames during normal Home Screen pan, when Volume Bar comes up and goes down during Video playback, etc. This bug fixes the stutter effect on normal Home Screen pan (on H/W Overlay devices) due to stale content display, as per above mentioned root cause.

Comment 26

[Peter Dolanjski [:pdol]]

Since this is related to the old homescreen which is not planning to ship with 2.0 (that option shouldn't be in Settings in the latest build), it shouldn't block.  Approval can be sought, though.

Comment 27

[Sushil]

(In reply to Peter Dolanjski [:pdol] from comment #26)
> Since this is related to the old homescreen which is not planning to ship
> with 2.0 (that option shouldn't be in Settings in the latest build), it
> shouldn't block.  Approval can be sought, though.

The old home screen was just 1 instance of issue which I have mentioned in Comment 25. Sending incorrect information about layer tree geometry change to HWC (specifically when tree geometry has changed but we inform HWC that it has not changed), can lead to stutter effect in other use cases also, which use partial HWC Composition.

Comment 28

[Inder]

Bhavana -- are we okay with the above justification to take it for 2.0?

Comment 29

[Tony Chung [:tchung]]

(In reply to Inder from comment #28)
> Bhavana -- are we okay with the above justification to take it for 2.0?

Inder, Sushil, do you have any other user-affected examples where the layer tree geometry changes to HWC affects the product outside of horizontal homescreen?  If so, please list them so we can make another triage call after your information.  Thanks, Tony

Comment 30

[Sushil]

As per definition of HWC_GEOMETRY_CHANGED in HWC header: https://github.com/android/platform_hardware_libhardware/blob/master/include/hardware/hwcomposer.h#L281. Framework should report the frame geometry change (i.e. change in layer tree geometry) correctly to HWC. Hence we need the patch in this bug for correct reporting of geometry change.

STR:
1. Play a video and make sure that the video controls are not visible.
2. Press Volume-up button once. Volume bar will be visible, then it will be invisible after a while.
3. Layer tree geometry changes once when Volume bar is visible (as a new layer is added to frame)
4. It changes again when Volume bar becomes invisible (since this layer is now removed from frame).

Refs:
[1]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#427
[2]: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#183

Follow the above STR and log the value of "mGeometryChanged" just before [1]. It is being used at [2] in HwcComposer2D. Compare the value at Step 3 and 4, with and without this patch.

Since above STR takes the full HWC Composition path, you will not observe any issue in UX but partial HWC Composition path can show UX issues like stale content, stuttering, etc.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ce7f53e3274

Comment 32

[[:SalvadorR]]

This feature is no longer implemented.