Closed Bug 914143 Opened 11 years ago Closed 9 years ago

Layers change size when panning

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INCOMPLETE

People

(Reporter: vingtetun, Assigned: roc)

References

Details

(Keywords: regressionwindow-wanted)

Attachments

(4 files, 1 obsolete file)

Increase allowed rects to 32
2.05 KB, patch
Details | Diff | Splinter Review
output3.gz
38.43 KB, application/octet-stream
Details
testcase.html
807 bytes, text/html
Details
print layers and quads rendered
6.38 KB, patch
Details | Diff | Splinter Review
Attached file testcase.html (obsolete) — 
I have initially seen that on b2g but it reproduce on desktop as well.

In order to see layers border I had to activate in about:config :
 - layers.draw-borders to true
 - layers.offmainthreadcomposition.force-basic to true

Then load the attached testcase and try to pan to the bottom. The layer tree is changing while scrolling. From a few tests on the device it seems like having one layer instead is good for fps.
Blocks: 912134
No longer blocks: 912134
Blocks: 912134
As we scroll we seem to be creating additional layers at the bottom and the arrangement keeps changing, causing a lot of invalidation (I think a single layer change cascades down as we reuse layers). Instead I would expect buffer rotation here, or stable layers that get pre-rendered and scroll into view.
This is a major performance problem for FFOS and a regression from 1.1. This will come up in stabilizing 1.2. We should try to fix this asap otherwise we will get a lot of dependent bugs back for all the apps where scrolling is affected.
With paint flashing enabled its also paintfull obvious that we way over-invalidate scrolling the test case.
(In reply to Andreas Gal :gal from comment #3)
> With paint flashing enabled its also paintfull obvious that we way
> over-invalidate scrolling the test case.

I could reproduce only a small amount of over invalidation in the bottom right box but nothing serious on Mac OMTC with border+flashing. Trying it out on b2g next.
This test case overinvalidates a lot less than the real test case but this one is much reduced. The problem is not overinvalidation. We layerize some of the divs where we shouldn't. This should be a single layer.
if you want to try this on b2g, turn on layer highlighting and then scroll the settings app. Use see us layerize multiple random parts as you scroll, with layers changing every few scroll ticks, causing a lot of re-painting of those layers.
We appear to be doing too much work, but I also wanted to point out that the number of green boxes does not necessarily match the number of layers.  So, just seeing a green box doesn't mean that it got a layer.  In this case though, this doesn't account for all of it.  BenWa will add the details of his investigation.
(In reply to Andreas Gal :gal from comment #6)
> Use see us layerize multiple random parts as you scroll,

Here's a video of what I'm seeing in case you're seeing something different:
https://dl.dropboxusercontent.com/s/h6n0k4d9aygq8w9/2013-09-09%2014.06.12.mp4?token_hash=AAGkmnct73WZytLV99WjnazZzH9pstasnj9Viniiw7D4Xg&dl=1

I'm scrolling the page and I see the following happening at each frame:
1) The bounding box of the contents (excluding background color) get a thebes/content layer
2) The bottom may get two smaller rects which are part of the *same* original layer. What happens here is each elements adds to the layer region which we simplify to 3 rects. The first rect is typically most of the layer (think 95%) where everything is merged together leaving 2 rects at the bottom that don't get merged together. Merging is a trade off between a complex region that is hard to paint/long to composite (cpu cycles, draw calls) vs. a merged region that is much larger then the original complex region (memory, bandwidth)

As you scroll the bounding box from 1) changes leading to parts that were previous in the background to get drawn. 2) changes frequently as it depends on how the last 2 rects fit into the large merged region.

What we're seeing here is clever code to minimize the work done for an individual frame not pay off because we're being too clever here. By trying to minimize our layers heavily we take regressions when these tights bounds are not stable like we're seeing here where we would be better off with a looser bound against the parent instead. In this case I see a bit of extra paint flashing but nothing significant. I'm seeing 50 FPS *without* async painting which means we're preparing and compositing 50 synchronous layer transaction which is great. The best way to improve this is to get sub-APZC, async transactions, tiling+draw in tile chunks instead of tiny slivers at 50 FPS.
TL;DR: We decide every frame the bounds of a layer which are a collections of rects. What we're seeing here is the same layer but the boundings rects are changing as you scroll, not new layers.

What is expensive is changing our minds about the layer bounds because sometimes we're use a new texture which isn't shown in layer border. Tiling solves this.
Our performance across the board is completely unacceptable, and this behavior is a regression from 1.1. Our choices are to enable tiling this week (FC is Friday), or backtrack whatever caused this. My preferred solution is OMTS and tiling, obviously. Doable?
See what Brad thinks about tiling option.
Flags: needinfo?(blassey.bugs)
(In reply to Andreas Gal :gal from comment #10)
> Our performance across the board is completely unacceptable, and this
> behavior is a regression from 1.1. Our choices are to enable tiling this
> week (FC is Friday), or backtrack whatever caused this. My preferred
> solution is OMTS and tiling, obviously. Doable?

Enabling tiling (and the features that are implied with it like Display Port) is just not feasible by Friday because we will hit regressions with Gralloc. Gralloc/HWC brings many additional problems to tiling that we haven't dealt before on b2g which make this not just a porting problems.

Let's investigate and fix the regression and make sure tiling is ready for the next b2g train.
BenWa will grab this bug and investigate here. roc observed this:

roc: gal: so, the main reason that testcase is slow is because it puts overflow:scroll on both the HTML and BODY elements!
[11:04pm] roc: well, not slow per se, but slightly messed up

I will ask vivien to look at that.
Summary: Layers are created / destroyed when panning → Layers change size when panning
(In reply to Andreas Gal :gal from comment #13)
> BenWa will grab this bug and investigate here. roc observed this:
> 
> roc: gal: so, the main reason that testcase is slow is because it puts
> overflow:scroll on both the HTML and BODY elements!
> [11:04pm] roc: well, not slow per se, but slightly messed up

The testcase is easily fixed by removing "scroll" from one of <html>/<body>, but the settings app can't be fixed that way, so the testcase is OK.
(In reply to Andreas Gal :gal from comment #10)
> this behavior is a regression from 1.1.

Are you sure? I don't think the way we set visible regions has changed significantly since 1.1, and I'm pretty sure this behavior could have happened in 1.1.

It's also not obvious that this would cause performance problems. Reducing the size of the valid region doesn't necessarily cause buffer reallocation, and if it does we should probably fix that directly.
So we need to zero in on whether we're actually getting buffer reallocations --- or new layers --- and figure out why. The former would require layers backend fixes, the latter would require fixes to FrameLayerBuilder.

It is possible that we get a startup effect where the second or third frame requires an extra buffer allocation because the visible region grows. If we're seeing that, we can fix it in FrameLayerBuilder.
With this patch we cleanly create stable layers (16 is sufficient too). The next problem is now that the topmost and bottommost layers grow as they scroll into view, and get completely re-allocated and repainted. At the very least we would have to pre-render them. Still, I don't get why we don't put this all into a single layer and maybe thats the real fix we want.
(In reply to Andreas Gal :gal from comment #17)
> Created attachment 801868 [details] [diff] [review]
> Increase allowed rects to 32
> 
> With this patch we cleanly create stable layers (16 is sufficient too). The
> next problem is now that the topmost and bottommost layers grow as they
> scroll into view, and get completely re-allocated and repainted. At the very
> least we would have to pre-render them. Still, I don't get why we don't put
> this all into a single layer and maybe thats the real fix we want.

With this patch the fps while scrolling the Settings app are ~20fps.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #18)
> (In reply to Andreas Gal :gal from comment #17)
> > Created attachment 801868 [details] [diff] [review]
> > Increase allowed rects to 32
> > 
> > With this patch we cleanly create stable layers (16 is sufficient too). The
> > next problem is now that the topmost and bottommost layers grow as they
> > scroll into view, and get completely re-allocated and repainted. At the very
> > least we would have to pre-render them. Still, I don't get why we don't put
> > this all into a single layer and maybe thats the real fix we want.
> 
> With this patch the fps while scrolling the Settings app are ~20fps.

For what it worth I'm using a keon.
(In reply to Andreas Gal :gal from comment #17)
> With this patch we cleanly create stable layers (16 is sufficient too). The
> next problem is now that the topmost and bottommost layers grow as they
> scroll into view, and get completely re-allocated and repainted. At the very
> least we would have to pre-render them. Still, I don't get why we don't put
> this all into a single layer and maybe thats the real fix we want.

I don't think these are layers. I think these are just multiple rectangles in the visible region (and hence multiple drawn quads) for a single layer.
Attached file output3.gz — 
I pulled mozilla-central, made a debug B2G build with the patch in bug 914426 and another patch that just flips gDumpCompositorTree to true, put the build on my dev phone, opened the settings app and scrolled up and down for a bit.

My layer-tree dump doesn't show me anything like what you guys are seeing. There is always only one scrolled layer (the one with the transform with a negative Y translation), and it always has only one rectangle in its visible region.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> So we need to zero in on whether we're actually getting buffer reallocations
> --- or new layers --- and figure out why. The former would require layers
> backend fixes, the latter would require fixes to FrameLayerBuilder.

Basically, if we keep (re)allocating textures or graphics buffers while scrolling then we need to dig into exactly why. If we aren't, then this bug isn't worth fixing.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) 
> Basically, if we keep (re)allocating textures or graphics buffers while
> scrolling then we need to dig into exactly why. If we aren't, then this bug
> isn't worth fixing.

I played a little bit with Andreas hack and instead of increasing the parameter value of SimplifyOutward from 4/8 to 32, I reduce it to 0 for all occurences. It's not a fix in any cases but for some reasons it seems to help a little bit fps on my keon.

It's pretty late here so I could misread something and so I will check in a few hours with someone next to me.
Attached file testcase.html — 
Same testcase with only one overflow:scroll on the body element.
Attachment #801556 - Attachment is obsolete: true
There are the quads we render on the desktop in stead state (after all layers timed out):

CompositorOGL::DrawQuad Rect(0.000000 0.000000 2880.000000 130.000000) Clip(0.000000 0.000000 2880.000000 1704.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 0.000000 2.000000 130.000000) Clip(0.000000 0.000000 2880.000000 1704.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(2.000000 0.000000 2876.000000 2.000000) Clip(0.000000 0.000000 2880.000000 1704.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(2878.000000 0.000000 2.000000 130.000000) Clip(0.000000 0.000000 2880.000000 1704.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(2.000000 128.000000 2876.000000 2.000000) Clip(0.000000 0.000000 2880.000000 1704.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 130.000000 2880.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 130.000000 2.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(2.000000 130.000000 2876.000000 2.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(2878.000000 130.000000 2.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(2.000000 1702.000000 2876.000000 2.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)

Excluding the UI:

CompositorOGL::DrawQuad Rect(0.000000 130.000000 2880.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)

The moment I start scrolling:

CompositorOGL::DrawQuad Rect(0.000000 130.000000 2880.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 1184.000000 2880.000000 1208.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 2392.000000 1448.000000 128.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 2520.000000 2880.000000 124.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 2644.000000 1448.000000 114.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)

roc, these are all separate layers. I print this in CompositorOGL::DrawQuad. We only render one quad per layer than spans the whole layer (see ContainerRender in ContainerLayerComposite.cpp).

During scrolling we create multiple redundant layers and are invalidating them. I am at this point pretty confident that this is causing a big part of our FFOS performance problem w.r.t. to scrolling. Vivien attached a new testcase with only a single overflow region.
We tried this testcase on b2g18 and b2g26. It has multiple layers on b2g18 as well. So while this test case is valid and buggy, its likely not the narrowest case we have to fix. Vivien is trying to make a test case that only breaks on 26. I will try to find out why this case creates so many layers.
A nicer version of the above:

Rest state:


ContainerLayerComposite::RenderLayer Offset(0 0) Clip(0 0)
ThebesLayerComposite::RenderLayer Offset(0 0) Clip(0 0)
CompositorOGL::DrawQuad Rect(0.000000 0.000000 2880.000000 130.000000) Clip(0.000000 0.000000 2880.000000 1704.000000) Offset(0.000000 0.000000)
ContainerLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
ThebesLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
CompositorOGL::DrawQuad Rect(0.000000 130.000000 2880.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
EndFrame()

Again with UI removed:

ContainerLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
ThebesLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
CompositorOGL::DrawQuad Rect(0.000000 130.000000 2880.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
EndFrame()

While moving: (UI removed)

ContainerLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
ThebesLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
CompositorOGL::DrawQuad Rect(0.000000 130.000000 2880.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
ThebesLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
CompositorOGL::DrawQuad Rect(0.000000 3400.000000 2880.000000 1260.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 4660.000000 1448.000000 128.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 4788.000000 2880.000000 124.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
CompositorOGL::DrawQuad Rect(0.000000 4912.000000 1448.000000 62.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
ContainerLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
ThebesLayerComposite::RenderLayer Offset(0 0) Clip(0 130)
CompositorOGL::DrawQuad Rect(2847.000000 130.000000 33.000000 1574.000000) Clip(0.000000 130.000000 2880.000000 1574.000000) Offset(0.000000 0.000000)
EndFrame()

As you can see we have several thebes layers in play here.
Vivien, can you try to change this from 8 to 32:

https://bugzilla.mozilla.org/attachment.cgi?id=718799&action=diff

and see if that affects settings?
it is sounding like we're too late in the game to enable tiles
Flags: needinfo?(blassey.bugs)
(In reply to Andreas Gal :gal from comment #30)
> Vivien, can you try to change this from 8 to 32:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=718799&action=diff
> 
> and see if that affects settings?

I tried and it does not seems to have an impact on settings fps.
Roc, can you own this or suggest if Matt or nrc could pick it up?
Assignee: nobody → roc
Applying bug 915342 shows that on my mac we overfill by 2x when scrolling the test case. Thats far worse than any regular content. I am not sure I fully trust my measurement methodology, but something is up here.
(I will go and measure with bug 915342 on the device with settings and contacts next)
Is there still an issue here?
Flags: needinfo?(21)
Closing this as incomplete due to inactivity and lack of response from the reporter. Feel free to reopen the bug if the issue still reproduces on a current build.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(21)
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: