965102 - Optimize use of HWC_GEOMETRY_CHANGED flag.

Status: RESOLVED FIXED

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

Description

[Sushil]

Currently, HWC_GEOMETRY_CHANGED flag is being set on each frame in HwcComposer2D. If we can find a way to get more information from Gecko framework to find out if there is no geometry change in this frame as compared to previous frame, then HWC_GEOMETRY_CHANGED can be set appropriately by HwcComposer2D, which can be utilized by HAL to invoke any optimization.
For example: Video playback use case.

Comment 1

[Diego Wilson [:diego]]

Nick,

Do you know if there's a simple way of knowing if the layer tree geometry changed other than caching the previous frame's layer tree?

Comment 2

[Sushil]

Yes, caching previous frame's layer tree should be the last resort. I am trying to find a better way to know this.

Comment 3

[Nick Cameron [:nrc]]

(In reply to Diego Wilson [:diego] from comment #1)
> Nick,
> 
> Do you know if there's a simple way of knowing if the layer tree geometry
> changed other than caching the previous frame's layer tree?

By geometry do you mean size, transform, etc.? If so I don't think there is an easy to know when it changes. If you want to know if the contents have changed you could record the invalid region.

Comment 4

[Diego Wilson [:diego]]

(In reply to Nick Cameron [:nrc] from comment #3)
> (In reply to Diego Wilson [:diego] from comment #1)
> > Nick,
> > 
> > Do you know if there's a simple way of knowing if the layer tree geometry
> > changed other than caching the previous frame's layer tree?
> 
> By geometry do you mean size, transform, etc.? If so I don't think there is
> an easy to know when it changes. If you want to know if the contents have
> changed you could record the invalid region.

Can two layers generate the same invalid region? If so, that won't work for this bug's purposes.

Comment 5

[Sushil]

Nick, yes Geometry change of a frame includes addition/removal of a layer or change in size (destination rectangle) or transform of layer(s) as compared to last frame. I believe invalid region will only help here if the geometry does not change.

Comment 6

[Nick Cameron [:nrc]]

Afraid I'm out of ideas, sorry. Perhaps mattwoodrow can help.

Matt - please see comment 1.

Comment 7

[Matt Woodrow (:mattwoodrow)]

The LayerProperties [1] class can track changes to a layer tree, and we have it setup for the compositor layer tree [2] (though only enabled for BasicCompositor).

Currently it only computes the region of the window that is invalid, but it could be easy extended to detect layer geometry changes.


[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.h#34
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#156

Comment 8

[Matt Woodrow (:mattwoodrow)]

How big do we expect the wins to be here?

I have a plan for implementing this, not sure if I should prioritise working on it though.

Comment 9

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> How big do we expect the wins to be here?
> 
In Video playback and Camera preview, the full screen non-updating black background layer can be skipped in HWC_BLIT Composition path. Hence, it will improve performance and power numbers. In general, all non-updating lower z-order layers can be skipped from composition in HWC_BLIT path, if geometry of current frame does not change as compared to previous frame.

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #9)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > How big do we expect the wins to be here?
> > 
> In Video playback and Camera preview, the full screen non-updating black
> background layer can be skipped in HWC_BLIT Composition path. Hence, it will
> improve performance and power numbers. In general, all non-updating lower
> z-order layers can be skipped from composition in HWC_BLIT path, if geometry
> of current frame does not change as compared to previous frame.

So is this a per-layer flag, or a global setting?

It seems like we'd need to know geometry and content changes on a per-layer basis in order to skip composition of layers entirely.

Comment 11

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> 
> So is this a per-layer flag, or a global setting?
> 

This is per-frame (list) flag.

> It seems like we'd need to know geometry and content changes on a per-layer
> basis in order to skip composition of layers entirely.

In HWC prepare call, the framework just needs to inform HAL if geometry of current frame has changed or not, via HWC_GEOMETRY_CHANGED flag. HAL will decide if it is going to skip composition of non-updating layers or not based on HWC_GEOMETRY_CHANGED flag and composition type. Since this optimization cannot be invoked in full HWC_OVERLAY path.

Comment 12

[Sushil]

Matt,

I checked by commenting out LAYERS_BASIC check at [1]. So it lands at [2] in all frames where this bug is intended to benefit, i.e. when frame geometry does not change (like Video playback and Camera preview). And it lands at [3] when frame geometry change happens (like when UI pops-up during Video playback).

So this bug depends on following 2 changes:
1. Enable [1] for LAYERS_OPENGL.
2. mClonedLayerTreeProperties is private member of LayerManagerComposite class. So either we need to make it visible to HwcComposer2D and set it to null after Render() call at [4]. OR we need to set boolean flag in "mRoot" (Container layer) at [2] which will tell HwcComposer2D that frame geometry has not changed.

I noticed that "invalid" region at [2] is always empty, which is expected as of now. It does not affect this bug but I believe it should be computed correctly for any future need, for ex: the Video layer region should be invalid in Video playback use-case, even when frame geometry does not change.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#155
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#222
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#227
[4] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#241

Comment 13

[Sushil]

OR we could add a bool flag parameter in TryRender() in b2g v1.4. Not sure, if we want to make this change in v1.3 as well. Diego, what do you think for this part ?

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #12)
> Matt,
> 
> I checked by commenting out LAYERS_BASIC check at [1]. So it lands at [2] in
> all frames where this bug is intended to benefit, i.e. when frame geometry
> does not change (like Video playback and Camera preview). And it lands at
> [3] when frame geometry change happens (like when UI pops-up during Video
> playback).

Is that always the case? I'd still expect us to get to [2] even with geometry changes. We should only get to [3] if the entire layer tree gets rebuilt.


> 
> So this bug depends on following 2 changes:
> 1. Enable [1] for LAYERS_OPENGL.
> 2. mClonedLayerTreeProperties is private member of LayerManagerComposite
> class. So either we need to make it visible to HwcComposer2D and set it to
> null after Render() call at [4]. OR we need to set boolean flag in "mRoot"
> (Container layer) at [2] which will tell HwcComposer2D that frame geometry
> has not changed.

I think we could pass a flag into BeginFrame that specifies that the geometry hasn't changed. Then we can forward that onto TryRender().

> 
> I noticed that "invalid" region at [2] is always empty, which is expected as
> of now. It does not affect this bug but I believe it should be computed
> correctly for any future need, for ex: the Video layer region should be
> invalid in Video playback use-case, even when frame geometry does not change.

Again, is it always empty? Or only for the video case? If it's the latter, then I suspect that's bug 966679.

Comment 15

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> 
> Is that always the case? I'd still expect us to get to [2] even with
> geometry changes. We should only get to [3] if the entire layer tree gets
> rebuilt.
> 
Yes, it gets to [2] even when geometry changes in some cases. Is there a way to filter out "no geometry change" here?

> > 
> > I noticed that "invalid" region at [2] is always empty, which is expected as
> > of now. It does not affect this bug but I believe it should be computed
> > correctly for any future need, for ex: the Video layer region should be
> > invalid in Video playback use-case, even when frame geometry does not change.
> 
> Again, is it always empty? Or only for the video case? If it's the latter,
> then I suspect that's bug 966679.

Most of the times, it is empty (for ex: Home screen panning). It is non-empty only for few frames, for ex: Open/Close an App.

Comment 16

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #15)
 
> Yes, it gets to [2] even when geometry changes in some cases. Is there a way
> to filter out "no geometry change" here?

You'd need to modify LayerProperties to return this information. It should be fairly simple. If we add to the region for any reason other than because of area in GetInvalidRegion(), then set a flag marking that the geometry changed.

 
> Most of the times, it is empty (for ex: Home screen panning). It is
> non-empty only for few frames, for ex: Open/Close an App.

That seems bad. Feel free to debug why this is :) The layer tree must be different, since we're getting different rendering. Either the changes happen outside of the Clone/ComputeDifference pair, or the difference computation has bugs.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Sushil from comment #11)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> 
> In HWC prepare call, the framework just needs to inform HAL if geometry of
> current frame has changed or not, via HWC_GEOMETRY_CHANGED flag. HAL will
> decide if it is going to skip composition of non-updating layers or not
> based on HWC_GEOMETRY_CHANGED flag and composition type. Since this
> optimization cannot be invoked in full HWC_OVERLAY path.

Hi Sushil,
Do you mean that if we have the same HwcLayer as previous frame's HwcLayer list, we can compose the list without HWC_GEOMETRY_CHANGED flag(same buffer size, same transform, but different buffer content)?

Comment 18

[Sushil]

(In reply to Jerry Shih[:jerry] from comment #17)
> (In reply to Sushil from comment #11)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> > 
> > In HWC prepare call, the framework just needs to inform HAL if geometry of
> > current frame has changed or not, via HWC_GEOMETRY_CHANGED flag. HAL will
> > decide if it is going to skip composition of non-updating layers or not
> > based on HWC_GEOMETRY_CHANGED flag and composition type. Since this
> > optimization cannot be invoked in full HWC_OVERLAY path.
> 
> Hi Sushil,
> Do you mean that if we have the same HwcLayer as previous frame's HwcLayer
> list, we can compose the list without HWC_GEOMETRY_CHANGED flag(same buffer
> size, same transform, but different buffer content)?

In Comment 11, I mentioned "non-updating" layers which means that buffer content (buffer handle) of HwcLayer has not changed as compared to previous frame.

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Sorry, I'm still confused.
Does the same buffer content or the "buffer handle" in comment 18 mean the data in the graphic buffer is no change?

The flag HWC_GEOMETRY_CHANGED is a per-frame setting.
If we want to call hwc->set() without HWC_GEOMETRY_CHANGED, we need to have the same buffer size, same transform, and same buffer content as previous frame's HwcLayer list.
If so, why we call compose? The screen is the same as prevoius frame.
Thanks.

Comment 20

[Sushil]

(In reply to Jerry Shih[:jerry] from comment #19)
> Sorry, I'm still confused.
> Does the same buffer content or the "buffer handle" in comment 18 mean the
> data in the graphic buffer is no change?

Yes

> 
> The flag HWC_GEOMETRY_CHANGED is a per-frame setting.
> If we want to call hwc->set() without HWC_GEOMETRY_CHANGED, we need to have
> the same buffer size, same transform, and same buffer content as previous
> frame's HwcLayer list.

Please note there are multiple layers in the list. So HWC_GEOMETRY_CHANGED should take care of the entire list.
If geometry of any layer has changed OR if a layer is added to/removed from list, HWC_GEOMETRY_CHANGED should be set.
Here is definition of this flag:
/*
 * HWC_GEOMETRY_CHANGED is set by SurfaceFlinger to indicate that the list
 * passed to (*prepare)() has changed by more than just the buffer handles
 * and acquire fences.
 */
HWC_GEOMETRY_CHANGED = 0x00000001,

> If so, why we call compose? The screen is the same as prevoius frame.

If you are trying to say that current frame is exact same as last frame (all layers are exact same) and even then we compose, then there is a bigger issue in framework, which is not related to Composition (GPU/HWC). HwcComposer2D just honours the composition request from framework in the sequence:
LayerManagerComposite::EndTransaction() --> Render() --> composer2D->TryRender()

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Sushil from comment #20)
> (In reply to Jerry Shih[:jerry] from comment #19)
> > If so, why we call compose? The screen is the same as prevoius frame.
> 
> If you are trying to say that current frame is exact same as last frame (all
> layers are exact same) and even then we compose, then there is a bigger
> issue in framework, which is not related to Composition (GPU/HWC).
> HwcComposer2D just honours the composition request from framework in the
> sequence:
> LayerManagerComposite::EndTransaction() --> Render() -->
> composer2D->TryRender()

If we have no change(no transform and no buffer content change), we will not trigger the LayerManagerComposite::EndTransaction(). Thus, the screen will remain as the previous frame.
So, would we need to handle the non-HWC_GEOMETRY_CHANGED case?

Comment 22

[Sushil]

LayerManagerComposite::EndTransaction() happens and it will still happen on non-HWC_GEOMETRY_CHANGED cases because the current frame is not same as previous frame as some layer handles have changed even though geometry of frame has not changed. For ex: Video playback and Camera preview use cases. That's where, we need this flag so that HAL can decide to skip composition of "non-updating" layers in current frame since geometry has not changed.

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Thanks, Sushil.

So, in video playback and camera preview use cases, the total layer number and all layer geometry are not change. The only chage is the GraphicBuffer handle(it implies the buffer content is also change). We replace it with a new camera preview handle.
In this case, we can use non-HWC_GEOMETRY_CHANGED flag for HWC.
Is my thought right? I'm confused by comment 20.

Comment 24

[Sushil]

(In reply to Jerry Shih[:jerry] from comment #23)
> Thanks, Sushil.
> 
> So, in video playback and camera preview use cases, the total layer number
> and all layer geometry are not change. The only chage is the GraphicBuffer
> handle(it implies the buffer content is also change). We replace it with a
> new camera preview handle.
> In this case, we can use non-HWC_GEOMETRY_CHANGED flag for HWC.
> Is my thought right? I'm confused by comment 20.

Yes.

Comment 25

[Sushil]

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

> You'd need to modify LayerProperties to return this information. It should
> be fairly simple. If we add to the region for any reason other than because
> of area in GetInvalidRegion(), then set a flag marking that the geometry
> changed.
> 

Matt,

I was looking into it. But there are few problems:
- From [1], it never gets into the actual geometry change block which is [2]. The only place which looks promising to get into geometry change block is [3].

- Setting a flag at [2] and reading on "mClonedLayerTreeProperties" after [1] because the previous LayerProperties object has already been deleted and its a new object with default flag value.

- Also I cannot set a LayerManager flag at [3] and read at [4] because it is BasicLayerManager object at [3] and LayerManagerComposite object at [4]. So both are different objects unless I need to add a static member in LayerManager class, which does not look good to me, to achieve this.

Since HWC_GEOMETRY_CHANGED flag is needed for HwcComposer2D only so I want to avoid all unnecessary framework changes and planning to restrict changes to LayerManagerComposite class. I would rely on new layer tree notification at [5]. The only limitation is there is no Render() call in same cycle in which new layer tree notification comes. So I just need to set HWC_GEOMETRY_CHANGED in next draw cycle and then have it to be non-HWC_GEOMETRY_CHANGED until I get the next new layer tree notification. It looks promising in non-HWC_GEOMETRY_CHANGED use cases which I am targeting in this bug.

[1]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#222
[2]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#155
[3]: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1247
[4]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#354
[5]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#227

Comment 26

[Sushil]

So it will be a state in LayerManagerComposite instead of a flag.

- mTreeGeometry can be = NEW / CHANGED / SAME
- If NEW or CHANGED, geometry_changed will be true and passed to TryRender.
- If SAME, geometry_changed will be false and passed to TryRender.

I will upload a WIP patch for feedback/review.

Comment 27

[Sushil]

It works fine in the planned Video and Camera use-cases. But I see an issue when UI seek-bar is up and clip time increments during video playback. It seems a frame is being considered as non geometry changed but actually it is not. I need to debug or think more about approach.

Comment 28

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #25)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> 
> > You'd need to modify LayerProperties to return this information. It should
> > be fairly simple. If we add to the region for any reason other than because
> > of area in GetInvalidRegion(), then set a flag marking that the geometry
> > changed.
> > 
> 
> Matt,
> 
> I was looking into it. But there are few problems:
> - From [1], it never gets into the actual geometry change block which is
> [2]. The only place which looks promising to get into geometry change block
> is [3].

This is a bug. I will have a fix up in bug 66679 today hopefully.

I think once we fix this, then the rest should be easy.

LayerProperties::ComputeDifferences should return a flag/bool to indicate if the returned result region contains/doesn't contain any geometry changes. We decide this by checking if we hit any of the geometry changed paths inside ComputeDifferences, like [2].

Comment 29

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #28)
 
> This is a bug. I will have a fix up in bug 66679 today hopefully.
> 

Is bug # correct ?

Comment 30

[Matt Woodrow (:mattwoodrow)]

966679, sorry.

Comment 31

[Sushil]

Can I pick patches in Bug # 966679 and check on v1.4 ? I believe they will not work on v1.3.

Comment 32

[Matt Woodrow (:mattwoodrow)]

Go for it. I've pushed them to try, will try land them today.

Comment 33

[Sushil]

Compilation error:
gecko/gfx/layers/ipc/LayerTransactionParent.cpp:209:5: error: 'AutoResolveRefLayers' was not declared in this scope
gecko/gfx/layers/ipc/LayerTransactionParent.cpp:209:26: error: expected ';' before 'resolve'

The dependency patch listed in bug 966679, has landed long back so hopefully I am not missing that.

Comment 34

[Sushil]

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

> This is a bug. I will have a fix up in bug 66679 today hopefully.
> 
> I think once we fix this, then the rest should be easy.
> 
> LayerProperties::ComputeDifferences should return a flag/bool to indicate if
> the returned result region contains/doesn't contain any geometry changes. We
> decide this by checking if we hit any of the geometry changed paths inside
> ComputeDifferences, like [2].

Matt,

I checked with patches of Bug 966679, it still has same issue. From LayerProperties::ComputeDifferences() call at [1], it never gets into the geometry change block which is [2]. So returning a flag from this call will not help.
I think we need to add a static/global flag in LayerPropertiesBase which keeps track of geometry change like [2]. 

[1]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#223
[2]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/LayerTreeInvalidation.cpp#155

Comment 35

[Sushil]

Attachment: Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately.

Gecko framework changes to check if the geometry of layer tree has changed as compared to last frame, to set / unset HWC_GEOMETRY_CHANGED flag. It will enable HAL to invoke any optimization, if the geometry of current frame (layer tree) has not changed.

Comment 36

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8398242 [details] [diff] [review]
Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately.

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

Looks pretty good overall, just a few things I think we should fix before landing.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +150,4 @@
>          mLayer->GetOpacity() != mOpacity ||
>          transformChanged) 
>      {
> +      aGeometryChanged = true;

This will register opacity changes as geometry changes, I doubt you want that (it's a pretty common thing to animate).

@@ +169,2 @@
>      visible.Xor(mVisibleRegion, mLayer->GetVisibleRegion());
>      AddTransformedRegion(result, visible, mTransform);

Visible region changes need to set aGeometryChanged = true.

@@ +342,5 @@
>    {
>    }
>  
> +  virtual nsIntRegion ComputeChangeInternal(NotifySubDocInvalidationFunc aCallback,
> +                                            bool& aGeometryChanged)

I think we should just have the bool& aGeometryChanged (or maybe bool* = nullptr so other calls don't need to be changed) on the public function too rather than trying to set it the result on the ContainerLayer.

@@ +431,2 @@
>      result = result.Union(OldTransformedBounds());
>      return result;

Don't we need to call SetGeometryChanged here too?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +228,5 @@
>      mClonedLayerTreeProperties = nullptr;
>  
> +    // If computed invalid region is empty, then it is new layer tree.
> +    // We want to be on safer side so if invalid region changes as
> +    // compared to last frame, we consider it as geometry change.

I don't really understand this condition, or why we need code here at all. We should just return the geometryChanged bool as an outparam of ComputeDifferences and rely on that. invalid.IsEmpty() should be false for a new layer tree, we return the entire bounds of the ContainerLayer in that case. If just doing this is exposing bugs, then we should fix those instead of having a fallback that guesses.

We should store the bool on LayerManagerComposite. too, rather than changing the public API of ContainerLayer.

::: widget/gonk/HwcComposer2D.cpp
@@ +779,4 @@
>          return false;
>      }
>  
> +    setHwcGeometry(aRoot);

Just pass the bool into TryRender instead of retrieving it from the layer.

Comment 37

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #36)
> ::: gfx/layers/LayerTreeInvalidation.cpp
> @@ +150,4 @@
> >          mLayer->GetOpacity() != mOpacity ||
> >          transformChanged) 
> >      {
> > +      aGeometryChanged = true;
> 
> This will register opacity changes as geometry changes, I doubt you want
> that (it's a pretty common thing to animate).
> 
Yes, it is needed. Consider a smaller RGBA layer sitting on top of full screen layer. So if opacity of smaller layer changes from 1 (opaque) to 0 (transparent) but handles and geometry of both layers have not changed, so HAL should not invoke optimization in this case since the underlying portion of bigger layer is visible due to transparency of smaller layer (in current frame). So framework should report HWC_GEOMETRY_CHANGED in this case.

> ::: widget/gonk/HwcComposer2D.cpp
> @@ +779,4 @@
> >          return false;
> >      }
> >  
> > +    setHwcGeometry(aRoot);
> 
> Just pass the bool into TryRender instead of retrieving it from the layer.

Will do. I thought of doing it like this but TryRender interface of composer2D needs to be changed.

Comment 38

[Sushil]

Attachment: Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately.

Matt, 

I am uploading patch with the review comments addressed.

Comment 39

[Sushil]

Please review the patch and provide your feedback.

This patch works fine. But I see an issue in use case when Video is paused and user touches the screen so that transparent UI seek-bar layer and Video title layers become visible. During this animation, the ComputeDifferences() call from LayerManagerComposite returns empty invalid region plus mGeometryChanged as "false" for 30 frames. I think it is an issue because at-least the visibility and opacity of some layers in frame is changing, which should be giving the animation effect. So either I am missing to set mGeometryChanged as "true" in ComputeDifferences() somewhere or invalid region calculation is wrong in such cases. Hence, the animation does not look smooth as I am passing HWC_GEOMETRY_CHANGED as "false" to HAL in those 30 frames in this use case. If I add |invalid.IsEmpty()| check in this patch, which I had in my previous patch, it works fine.

Comment 40

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #39)
> Please review the patch and provide your feedback.
> 
> This patch works fine. But I see an issue in use case when Video is paused
> and user touches the screen so that transparent UI seek-bar layer and Video
> title layers become visible. During this animation, the ComputeDifferences()
> call from LayerManagerComposite returns empty invalid region plus
> mGeometryChanged as "false" for 30 frames. I think it is an issue because
> at-least the visibility and opacity of some layers in frame is changing,
> which should be giving the animation effect. So either I am missing to set
> mGeometryChanged as "true" in ComputeDifferences() somewhere or invalid
> region calculation is wrong in such cases. Hence, the animation does not
> look smooth as I am passing HWC_GEOMETRY_CHANGED as "false" to HAL in those
> 30 frames in this use case. If I add |invalid.IsEmpty()| check in this
> patch, which I had in my previous patch, it works fine.

This is because LayerTreeInvalidation is checking GetOpacity(), and it should be GetLocalOpacity(). The latter will take async animations into account, the former won't. There's two places in that file that need to be fixed. GetTransform() should do the same.

Comment 41

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8400327 [details] [diff] [review]
Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately.

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +152,5 @@
>          transformChanged) 
>      {
> +      if (aGeometryChanged != nullptr) {
> +        *aGeometryChanged = true;
> +      }

For the opacity changes, you should only report geometry changes if we transition from 1 to not-1 and the reverse. Changes between other values shouldn't need to do this, and will give us better performance when animating opacity (a common case).

@@ +429,2 @@
>  {
>    NS_ASSERTION(aRoot, "Must have a layer tree to compare against!");

I think I'd prefer if we made a local bool geometryChanged value here and always pass that in to ComputeChange. That way we can make the parameter a reference instead of a pointer and avoid the null checks above.

Then just null check once before copying the result to aGeometryChanged.

Comment 42

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #40)
> This is because LayerTreeInvalidation is checking GetOpacity(), and it
> should be GetLocalOpacity(). The latter will take async animations into
> account, the former won't. There's two places in that file that need to be
> fixed. GetTransform() should do the same.

Yes, GetLocalOpacity() fixes the animation issue.

Comment 43

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #41)
> Comment on attachment 8400327 [details] [diff] [review]
> Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately.
> 
> Review of attachment 8400327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerTreeInvalidation.cpp
> @@ +152,5 @@
> >          transformChanged) 
> >      {
> > +      if (aGeometryChanged != nullptr) {
> > +        *aGeometryChanged = true;
> > +      }
> 
> For the opacity changes, you should only report geometry changes if we
> transition from 1 to not-1 and the reverse. Changes between other values
> shouldn't need to do this, and will give us better performance when
> animating opacity (a common case).
> 

The intermediate opacity values are rare, they are seen only during detailed animations like fading in/out of UI seek-bar layer in Video use-case and App entry/exit animation. I do not want to break such animations by invoking any HAL optimization. HAL should simply honour the layer list sent by framework in such cases.

Comment 44

[Sushil]

Attachment: Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately.

Matt,

I have addressed the feedback comments. Please review this patch.

Comment 45

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8400918 [details] [diff] [review]
Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately.

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

Looks good!

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +441,3 @@
>      return result;
>    } else {
> +    bool geometryChanged = (aGeometryChanged != nullptr) ? *aGeometryChanged : false;

No need to read the input value of aGeometryChanged here, it should be treated as uninitialized.

Comment 46

[Matt Woodrow (:mattwoodrow)]

(In reply to Sushil from comment #43)

> 
> The intermediate opacity values are rare, they are seen only during detailed
> animations like fading in/out of UI seek-bar layer in Video use-case and App
> entry/exit animation. I do not want to break such animations by invoking any
> HAL optimization. HAL should simply honour the layer list sent by framework
> in such cases.

I don't see what the risk of doing this is, and it's a few lines of code. That said it doesn't really matter, so up to you.

Comment 47

[Sushil]

(In reply to Matt Woodrow (:mattwoodrow) from comment #45)
> ::: gfx/layers/LayerTreeInvalidation.cpp
> @@ +441,3 @@
> >      return result;
> >    } else {
> > +    bool geometryChanged = (aGeometryChanged != nullptr) ? *aGeometryChanged : false;
> 
> No need to read the input value of aGeometryChanged here, it should be
> treated as uninitialized.

I kept this to carry forward the geometry change indicated by ComputeDifferences() in EndTransaction() call when (aFlags & END_NO_IMMEDIATE_REDRAW) is True, to the next Render() call. To handle this use case:

EndTransaction: (aFlags & END_NO_IMMEDIATE_REDRAW)=True, invalid=[Full Screen] mGeometryChanged = TRUE
EndTransaction: (aFlags & END_NO_IMMEDIATE_REDRAW)=False, invalid=[0 0 0 0] mGeometryChanged = FALSE

Since there is no Render() call when (aFlags & END_NO_IMMEDIATE_REDRAW) is True, so if there is new layer-tree indication (geometry change) by ComputeDifferences(), it should be carry forward to next immediate Render() call to inform HwcComposer2D. What is the significance of calling EndTransaction() with END_NO_IMMEDIATE_REDRAW flag ?

Comment 48

[Sushil]

For example: Video playback with video controls visible on screen, here is sequence of frames when the circular progress marker moves on seek-bar:

EndTransaction: invalid=[0 437 720 406] mGeometryChanged = False
EndTransaction: invalid=[0 437 720 406] mGeometryChanged = False
EndTransaction: invalid=[176 1133 300 46] mGeometryChanged = True
EndTransaction: END_NO_IMMEDIATE_REDRAW flag is set...No Render !
EndTransaction: invalid=[0 0 0 0] mGeometryChanged = False
EndTransaction: invalid=[0 437 720 406] mGeometryChanged = False
EndTransaction: invalid=[0 437 720 406] mGeometryChanged = False

Since the progress marker layer [46x46] has moved, the geometry of layer tree has changed. But there is no Render call (due to END_NO_IMMEDIATE_REDRAW flag), so HAL needs to be informed that layer tree geometry has changed in the next immediate Render call. That is why, we need the change in Comment 45.

Comment 49

[Matt Woodrow (:mattwoodrow)]

Ok, that's fine then.

END_NO_IMMEDIATE_REDRAW means only update the layers, but don't do a composite. It's what we use when we receive a transaction from content.

Comment 50

[Sushil]

Attachment: Gecko changes to set HWC_GEOMETRY_CHANGED flag appropriately. r=mattwoodrow

Uploading HG friendly version of the reviewed patch.

Comment 51

[Jason Smith [:jsmith]]

Not a blocker - there's no rationale here justifying the 1.4 need.

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/458e4922a69f

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/458e4922a69f

Comment 54

[Inder]

Preeti, this needs to be uplifted to v1.4 as this will improve the power numbers on 8x26 target.
Sushil, can you please add some details here in summary how this improves the power numbers.

Comment 55

[Preeti Raghunath(:Preeti)]

Sushil

Can you please test the patch and provide test results? Can certainly look at uplifting to 1.4

Comment 56

[Sushil]

Here is brief summary on how this patch should improve power numbers:

On devices with less number of H/W overlays, if the number of HWC layers in a frame exceed the number of H/W overlays, then composition falls back to partial HWC Composition. This happens all the time in critical use cases like Camera preview and Video playback (in Landscape mode when Video controls are visible) and happens most of the time in other use cases. In partial HWC Composition, few layers get composed by HWC and remaining layers gets composed on FrameBuffer layer by GPU.

So, if geometry of current frame has not changed as compared to last frame and HWC_GEOMETRY_CHANGED flag is being appropriately unset by Gecko framework, HAL can skip GPU composition of non-updating layers in the frame and it can re-use the layers which have not changed, so there is no need to invoke GPU Composition in current frame. Hence it will improve power numbers by avoiding GPU Composition.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6894e2565c5e

Comment 58

[[:RJ]]

Not enough information with current STR to write test case to address bug.