926268 - Layers in Settings app changes size, causes buffer re-allocations

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

While scrolling in the Settings app with layer border the content the layer keeps changing sizes. I don't know this for a fact but I suspects it is what's responsible for the sporadic buffer (re)allocations while scrolling. We need to investigate and fix/mitigate this somehow.

Comment 1

[Benoit Girard (:BenWa)]

Some progress. Looks like the thebes buffer is somehow getting size 0:
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3
I/Gecko   ( 9847): Buffer changed size 0x43dbd608, 0, 0, to 320, 3

Comment 2

[Benoit Girard (:BenWa)]

Now that the inspector is working well on b2g that should be the first thing to look into.

Comment 3

[Benoit Girard (:BenWa)]

Note that the impact of this has been mitigated in the settings app by bug 938785 but the underlying problem is still there.

Comment 4

[Benoit Girard (:BenWa)]

I can reproduce this on this page:
http://www.reddit.com/r/Minecraft/comments/1rwzbo/nanofarm_v3_stack_of_wheat_in_35_seconds/

A relatively simple forum page with a fixed position background. We're very slow to scroll this page.

Comment 5

[Benoit Girard (:BenWa)]

Matt, for a page like the one in Comment 4 we can solve a large class of problem like that if we don't shrink scrolling layer and have them fit the entire scroll frame. We're trying too hard to save memory and suffer in these kinds of cases.

What do you think?

Comment 6

[Matt Woodrow (:mattwoodrow)]

What I see on desktop doesn't match the original description, but it's interesting.

The fixed position image appears to be between the text and the page background, so we sometimes have to put the text in it's own (component alpha) layer.

We detect that most of the text doesn't actually cover the image, so this gets merged with the background.

Sometimes (but not all of them time?) we think some of the text at the same Y position as the image does cover it, so we push this block of text into a new layer.

Looks like a bug or two here, I'll look into it with a debugger today.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't include the whole frame bounds if we're only painting one border size

This fixes the bug causing us to reallocate buffers on reddit.

I doubt it does anything for your b2g test case though, this was most likely an unrelated bug.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8342077 [details] [diff] [review]
Don't include the whole frame bounds if we're only painting one border size

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

::: layout/base/nsDisplayList.cpp
@@ +2557,5 @@
> +  } else {
> +    nsMargin border = styleBorder->GetComputedBorder();
> +    nsRegion result;
> +    if (border.top > 0) {
> +      result.Or(result, nsRect(borderBounds.X(), borderBounds.Y(), borderBounds.Width(), border.top));

Just assign since we know it's empty currently.

@@ +2567,5 @@
> +      result.Or(result, nsRect(borderBounds.X(), borderBounds.YMost() - border.bottom, borderBounds.Width(), border.bottom));
> +    }
> +    if (border.left > 0) {
> +      result.Or(result, nsRect(borderBounds.X(), borderBounds.Y(), border.left, borderBounds.Height()));
> +    }

Why use a region here? Just use a rect and UnionRect.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't include the whole frame bounds if we're only painting one border size v2

Comment 10

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/052685e62b34

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/052685e62b34

Comment 12

[Andreas Gal :gal]

Can we check whether this fixes the B2G test case and if not file a bug to continue the work there?

Comment 13

[Benoit Girard (:BenWa)]

Settings app painting is fine now.