1148855 - Rework how display items are assigned to PaintedLayers and make it work the same way with and without APZ

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

At the moment, without APZ, we merge display items that have the same animated geometry root and are in the same container layer into the same PaintedLayer whenever possible, even if it is possible for other animated or scrolled layers to get in between those items later. Once that happens, we split the merged items into separate layers and invalidate both layers.

With APZ, however, we never merge across different animated geometry roots, and end up with lots of layers instead. We do this even if the interleaving other animated geometry root is clipped in such a way that it can never really get in between the other items that we decide to split.

In this bug I'm going to change our layerization in such a way that we use the same behavior both with and without APZ. We will only merge items into the same layer if it's not possible for items from other animated geometry roots to get in between them.

A closely related problem is how we decide to pull up background colors into layers in order to make them opaque. At the moment, without APZ, we happily pull across different animated geometry roots, and then need to invalidate once something moves in between the uniform background and the layer that pulled the color into its background. With APZ, we usually don't pull background colors through different animated geometry roots, except in a few unfortunate cases (e.g. scrollbars, see bug 1136304).
There is one scenario in which we really want to pull background colors across different animated geometry roots: If the background of a scrollable frame is uniform and opaque, we want to be able to pull that background color into the scrolled layer, and we want to do that even if the scrolled layer has a display port that extends beyond the uniform background. (At the moment, when we pull the color, we only look at the visible region of the layer, and don't check whether it's clipped. We only need the clip to have a uniform background, not the whole display port.)
The patch will fix this part, too: We'll only pull background colors if nothing can get in the pull-path, and if we have an actively scrolled frame then we make use of its clip when determining a background color for its contents.

The patch will fix both problems from bug 984219, and bug 1136304, and the remaining problem in bug 922766.

Comment 1

[Markus Stange [:mstange]]

Attachment: MozReview Request: bz://1148855/mstange

/r/6265 - Bug 1148855 - Set overflow:hidden on scrollbar tracks so that layerization knows that the scrollbar thumb won't leave the scrollbar. r=roc
/r/6267 - Bug 1148855 - Mark some ContainerState methods as const. r=roc
/r/6269 - Bug 1148855 - Rework how display items are assigned to PaintedLayers and make it work the same way with and without APZ. r=roc
/r/6271 - Bug 1148855 - Add some tests. r=roc

Pull down these commits:

hg pull review -r ba7c2223d885f6efb211398fac894b5d3fc31152

Comment 2

[Markus Stange [:mstange]]

The tests illustrate most of the problems I'm fixing, and hopefully convey why I had to go all the way to the tree structure instead of arriving at something simpler. I'm also adding a few tests that fail, to point out a few problems with the patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8d8f3ecfc5c

Comment 3

[Markus Stange [:mstange]]

Hmm, reftests don't look as good as they did a few days ago. My recent changes must have broken something.

Comment 4

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

https://reviewboard.mozilla.org/r/6265/#review5253

Ship It!

Comment 5

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

https://reviewboard.mozilla.org/r/6267/#review5255

Ship It!

Comment 6

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

https://reviewboard.mozilla.org/r/6269/#review5257

Can you split the patch into "make non-APZC behave like APZC" and "improve background-color handling (with painted layer data tree)"? I think that would really help us track down perf and correctness regressions.

I can see the logic behind adding the tree but it's an unfortunate amount of new code :-(.

Otherwise it looks pretty good.

::: layout/base/FrameLayerBuilder.cpp
(Diff revision 1)
> +    { return mVisibleAboveRegion.Intersects(aRect); }

I think the { lines up with 'b', i.e. no indenting at all.

::: layout/base/FrameLayerBuilder.cpp
(Diff revision 1)
> +  nsTArray<UniquePtr<PaintedLayerDataNode>> mChildren;

Why not just nsTArray<PaintedLayerDataNode>?

::: layout/base/FrameLayerBuilder.cpp
(Diff revision 1)
> +  return mTree.UniformBackgroundColor(); 

Trailing whitespace

::: layout/base/FrameLayerBuilder.cpp
(Diff revision 1)
> +PaintedLayerDataNode::FinishChildrenThatIntersect(const nsIntRect& aRect)

Call this FinishChildrenIntersecting

Comment 7

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

https://reviewboard.mozilla.org/r/6271/#review5259

Ship It!

Comment 8

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/6269/#review5267

What should the layer assignment behavior be in the intermediate state? I could make non-APZ work like APZ does now (make the visible above region infinite whenever the animated geometry root changes, so that nothing is merged across different interleaving animated geometry roots), but that might introduce perf regressions without APZ.
Since I'm using the tree not only for background color finding, but also for deciding how to merge layers, I didn't know what intermediate behavior to choose, that's why I didn't split up the patch.

> Why not just nsTArray<PaintedLayerDataNode>?

I'll add this comment above this line:
  // UniquePtr is used here in the sense of "unique ownership", i.e. there is
  // only one owner. Not in the sense of "this is the only pointer to the
  // node": There are two other, non-owning, pointers to our child nodes: The
  // node's respective children point to their parent node with their mParent
  // pointer, and the tree keeps a map of animated geometry root to node in its
  // mNodes member. These outside pointers are the reason that mChildren isn't
  // just an nsTArray<PaintedLayerDataNode> (since the pointers would become
  // invalid whenever the array expands its capacity).

Comment 9

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #8)
> What should the layer assignment behavior be in the intermediate state? I
> could make non-APZ work like APZ does now (make the visible above region
> infinite whenever the animated geometry root changes, so that nothing is
> merged across different interleaving animated geometry roots), but that
> might introduce perf regressions without APZ.
> Since I'm using the tree not only for background color finding, but also for
> deciding how to merge layers, I didn't know what intermediate behavior to
> choose, that's why I didn't split up the patch.

Comment 10

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

(In reply to Markus Stange [:mstange] from comment #8)
> What should the layer assignment behavior be in the intermediate state? I
> could make non-APZ work like APZ does now (make the visible above region
> infinite whenever the animated geometry root changes, so that nothing is
> merged across different interleaving animated geometry roots), but that
> might introduce perf regressions without APZ.
> Since I'm using the tree not only for background color finding, but also for
> deciding how to merge layers, I didn't know what intermediate behavior to
> choose, that's why I didn't split up the patch.

I think it's OK to have an intermediate state where perf regresses. That's still helpful for narrowing down correctness regressions.

Comment 11

[Markus Stange [:mstange]]

Ok.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8585106 [details]
MozReview Request: bz://1148855/mstange

/r/6265 - Bug 1148855 - Set overflow:hidden on scrollbar tracks so that layerization knows that the scrollbar thumb won't leave the scrollbar. r=roc
/r/6267 - Bug 1148855 - Mark some ContainerState methods as const. r=roc
/r/6507 - Bug 1148855 - Intermediate state that unifies APZ and non-APZ layerization behavior somewhat.
/r/6269 - Bug 1148855 - Rework how display items are assigned to PaintedLayers and make it work the same way with and without APZ. r=roc
/r/6271 - Bug 1148855 - Add some tests. r=roc

Pull down these commits:

hg pull -r a103b776d86b81fb8ad6c662fa4fc887ca950ece https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8585106 [details]
MozReview Request: bz://1148855/mstange

/r/6265 - Bug 1148855 - Set overflow:hidden on scrollbar tracks so that layerization knows that the scrollbar thumb won't leave the scrollbar. r=roc
/r/6267 - Bug 1148855 - Mark some ContainerState methods as const. r=roc
/r/6507 - Bug 1148855 - Intermediate state that unifies APZ and non-APZ layerization behavior somewhat.
/r/6269 - Bug 1148855 - Rework how display items are assigned to PaintedLayers and make it work the same way with and without APZ. r=roc
/r/6271 - Bug 1148855 - Add some tests. r=roc

Pull down these commits:

hg pull -r db401a6b6e4f4d8bf4696b5a8c17c86835a41ff5 https://reviewboard-hg.mozilla.org/gecko/

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8585106 [details]
MozReview Request: bz://1148855/mstange

/r/6265 - Bug 1148855 - Set overflow:hidden on scrollbar tracks so that layerization knows that the scrollbar thumb won't leave the scrollbar. r=roc
/r/6267 - Bug 1148855 - Mark some ContainerState methods as const. r=roc
/r/6507 - Bug 1148855 - Intermediate state that unifies APZ and non-APZ layerization behavior somewhat.
/r/6269 - Bug 1148855 - Rework how display items are assigned to PaintedLayers and make it work the same way with and without APZ. r=roc
/r/6271 - Bug 1148855 - Add some tests. r=roc

Pull down these commits:

hg pull -r 0efe08cf4abb8b45a5ed0409ade6cbccc6da03f0 https://reviewboard-hg.mozilla.org/gecko/

Comment 15

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dc45774c118

Comment 16

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b50e39d915f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a855f28ca86d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de7f61c40cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/10e5dd951795
https://hg.mozilla.org/integration/mozilla-inbound/rev/b25a2c3e600a

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b50e39d915f3
https://hg.mozilla.org/mozilla-central/rev/a855f28ca86d
https://hg.mozilla.org/mozilla-central/rev/9de7f61c40cc
https://hg.mozilla.org/mozilla-central/rev/10e5dd951795
https://hg.mozilla.org/mozilla-central/rev/b25a2c3e600a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1148855 - Set overflow:hidden on scrollbar tracks so that layerization knows that the scrollbar thumb won't leave the scrollbar. r=roc

Comment 21

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1148855 - Intermediate state that unifies APZ and non-APZ layerization behavior somewhat.

Comment 22

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1148855 - Rework how display items are assigned to PaintedLayers and make it work the same way with and without APZ. r=roc

Comment 23

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1148855 - Mark some ContainerState methods as const. r=roc

Comment 24

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1148855 - Add some tests. r=roc

Comment 25

[KTucker [:KTucker][Inactive 3/4/2016]]

Can we get this uplifted to 2.2 to resolve bug 1176328?