Closed
Bug 1037220
Opened 10 years ago
Closed 10 years ago
Reorder rendering to avoid render target switches
Categories
(Core :: Graphics, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: jrmuizel, Assigned: jrmuizel)
References
Details
(Keywords: perf, Whiteboard: [c=fps p= s= u=])
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
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.
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.
Updated•10 years ago
|
Attachment #8454099 -
Attachment is patch: true
Attachment #8454099 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8454099 -
Attachment is obsolete: true
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.
Attachment #8455451 -
Attachment is obsolete: true
Attachment #8455631 -
Flags: review?(matt.woodrow)
Attachment #8455631 -
Flags: review?(bgirard)
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.
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.
Updated•10 years ago
|
Attachment #8455631 -
Flags: review?(matt.woodrow) → review+
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.
*/
Attachment #8455631 -
Flags: review?(bgirard) → review+
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.
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8455631 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8456412 -
Attachment is obsolete: true
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.
Attachment #8458248 -
Flags: review?(matt.woodrow)
Updated•10 years ago
|
Attachment #8458248 -
Flags: review?(matt.woodrow) → review+
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!
Comment 12•10 years ago
|
||
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•10 years ago
|
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•