Jeff Muizelaar [:jrmuizel] Assignee
	Description • 10 years ago

Right now we will for example draw the background, switch to a container layers's render target, draw to that, switch back the render target draw the container layer, move on to the next container layer etc.

This render target switching is especially bad for mobile gpus (like adreno that needs to move things in and out of gmem).

This patch switches to a two pass approach that renders all of the container layers first and then composites everything all together.

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 1 • 10 years ago

This patch nearly doubles the framerate during the app -> homescreen transition, but will cause our peak memory usage to increase as we now retain all of the child temporary buffers until they are composited.

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 3 • 10 years ago

This breaks out the original ContainerRender into three functions. ContainerPrepare, ContainerRender, and RenderLayers

ContainerPrepare produces a PreparedData that contains all the information that it computed and is used by ContainerRender.

	Matt Woodrow (:mattwoodrow)
	Comment 4 • 10 years ago

Have we investigated why the app -> homescreen transition needs an intermediate surface? It seems like avoiding that would be an even bigger win. It would also be nice to add tools for gaia developers to use to diagnose this particular performance problem.

	Benoit Girard (:BenWa)
	Comment 5 • 10 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Have we investigated why the app -> homescreen transition needs an
> intermediate surface? It seems like avoiding that would be an even bigger
> win. It would also be nice to add tools for gaia developers to use to
> diagnose this particular performance problem.

Jeff and I discussed avoiding this. We would have to avoid using opacity. I think if we replace the transforms with a fancier animation we could remove the opacity and still have a nice feel. Otherwise we would have to make sure the layer tree only has one child. I think the only way to guarantee that is to use a screenshot layer which isn't ideal.

bug 864829 covers adding performance warnings for things that should be avoided and include this as an example.

	Benoit Girard (:BenWa)
	Comment 6 • 10 years ago

Comment on attachment 8455631 [details] [diff] [review]
Patch with logging and trimed reftests for reproducing

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

r+ with style fix + removing redundant SetRenderTarget switches for nested container layers.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +283,5 @@
> +    layer(aLayer), clipRect(aClipRect), restoreVisibleRegion(aRestoreVisibleRegion), savedVisibleRegion(aVisibleRegion) {}
> +  LayerComposite* layer;
> +  nsIntRect clipRect;
> +  bool restoreVisibleRegion;
> +  nsIntRegion savedVisibleRegion;

These member should have the m prefix. Even if it's a struct.

@@ +346,2 @@
>  
>      compositor->SetRenderTarget(surface);

I think this needs to be closer to the render call otherwise we might do needless switch. It might not paint but at least we wont bet on the driver optimizing this for us.

::: gfx/layers/composite/LayerManagerComposite.h
@@ +322,5 @@
>  
>    virtual Layer* GetLayer() = 0;
>  
>    virtual void RenderLayer(const nsIntRect& aClipRect) = 0;
> +  virtual void Prepare(const nsIntRect& aClipRect) {}

Again I'm a big fan of method documentation.

/**
 * Perform a first pass over the layer tree to prepare intermediate surfaces.
 * This allows us on to avoid framebuffer switches in the middle of our render
 * which is inefficient.
 */

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 7 • 10 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Have we investigated why the app -> homescreen transition needs an
> intermediate surface? It seems like avoiding that would be an even bigger
> win. It would also be nice to add tools for gaia developers to use to
> diagnose this particular performance problem.

I filed bug 1037092 on this issue.

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 10 • 10 years ago

The original patch broke the copying of the background that we do because we don't have a background to copy. This is the interdiff of a patch that only renders to an intermediate early when we don't need to copy the background.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

Are you using an ancient version of hg that's still affected by the long-ago-fixed (like 2.5 or 2.6 timeframe) rebase bug? Because your push touched a ton of Places test files that I don't think were included in your patch. If so, please update hg ASAP. There's nothing that can be done at this point to un-do the history munging this push caused, but we can at least try to avoid it happening again.

Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a555f10c40e5

	Jeff Muizelaar [:jrmuizel] Assignee
	Comment 13 • 10 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Are you using an ancient version of hg that's still affected by the
> long-ago-fixed (like 2.5 or 2.6 timeframe) rebase bug? Because your push
> touched a ton of Places test files that I don't think were included in your
> patch. If so, please update hg ASAP. There's nothing that can be done at
> this point to un-do the history munging this push caused, but we can at
> least try to avoid it happening again.
> 
> Thanks!

Yeah. I was using 2.3. I've fixed that now.