1169502 - Consider a different region simplification approach for visible regions

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

On http://www.w3.org/conf/2013sf/ the page foreground on the right has two layers, and the front layer constantly changes its shape during scrolling as elements are added and removed. Whenever the shape changes, large empty parts of the visible region are frequently invalidated because they are added or removed to the layer's visible region due to region simplification.

The thing about region simplification that annoys me the most is the fact that it affects parts of the region that are far away from where the actual change happens.

In this bug I'm proposing a different way of simplifying regions. It's probably easiest to understand by just reading the code:

/**
 * An alternative to nsIntRegion that keeps the region simple by snapping all
 * accumulated rects outwards to tiles. It keeps track of the bounds of the
 * true unsimplified region so that the result still has the bounds you'd
 * expect.
 * This approach doesn't guarantee a maximum number of rectangles in the
 * region, but it has the advantage that region simplification doesn't merge
 * rectangles that are very far apart; the simplification impact is local.
 * This representation also has the property of being canonical: the result
 * is independent of the order in which the rectangles are added.
 */
struct nsIntRegionSimplifiedToTiles
{
  nsIntRegionSimplifiedToTiles(const nsIntPoint& aTileOrigin,
                               const nsIntSize& aTileSize)
    : mTileOrigin(aTileOrigin)
    , mTileSize(aTileSize)
  {}

  nsIntRegion Get() const { return mBoundedRegion; }

  void Accumulate(const nsIntRect& aRect)
  {
    mBounds = mBounds.Union(aRect);
    nsIntRect rect = aRect - mTileOrigin;
    rect.InflateToMultiple(mTileSize);
    mSimplifiedRegion.OrWith(rect + mTileOrigin);
    mBoundedRegion = mSimplifiedRegion.Intersect(mBounds);
  }

  bool IsEmpty() const { return mBounds.IsEmpty(); }
  void SetEmpty()
  { mSimplifiedRegion.SetEmpty(); mBounds.SetEmpty(); }
  nsIntRegion Intersect(const nsIntRegion& aRegion) const
  { return Get().Intersect(aRegion); }
  bool Intersects(const nsIntRect& aRect) const
  { return Get().Intersects(aRect); }
  nsIntRect GetBounds() const { return mBounds; }

private:
  nsIntRegion mSimplifiedRegion;
  nsIntRect mBounds;
  nsIntRegion mBoundedRegion;
  nsIntPoint mTileOrigin;
  nsIntSize mTileSize;
};


With this change we're still invalidating empty areas of a layer when its bounds change, but overall the paint flashing on that page looks a lot less flickery.

I'm wondering whether the bounds-preserving property is really necessary, but we can look at that later.

Comment 1

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r=roc

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8612667 [details]
MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r=roc

Comment 3

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

Comment on attachment 8612667 [details]
MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

https://reviewboard.mozilla.org/r/9625/#review8527

::: layout/base/FrameLayerBuilder.cpp:98
(Diff revision 2)
> +  nsIntRegion mBoundedRegion;

Why store mBoundedRegion explictly? Instead we can calculate it on demand.

Comment 4

[Markus Stange [:mstange]]

Because I think Get() is getting multiple times after each rect gets added, mostly through Intersects(). I could calculate it on demand and cache it (plus add an isCached flag that Accumulate resets). Would you prefer that?

Comment 5

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

I think Intersects can be implemented efficiently without building the bounded region by intersecting it with mBounds and then checking for intersection with mSimplifiedRegion. So I'm not convinced it saves work to store the bounded region explicitly.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8612667 [details]
MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

Comment 7

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

Comment on attachment 8612667 [details]
MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

https://reviewboard.mozilla.org/r/9625/#review8637

Ship It!

Comment 8

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

Sorry, I didn't mean to r+ that and I'm over very slow Wifi :-(

I think the Intersects method is more accurate if we write 
  return mRegion.Intersects(mBounds.Intersect(aRect));

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8612667 [details]
MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

Comment 10

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

Comment on attachment 8612667 [details]
MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc

https://reviewboard.mozilla.org/r/9625/#review9269

Ship It!

Comment 11

[Markus Stange [:mstange]]

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

Comment 12

[Markus Stange [:mstange]]

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

Comment 13

[Markus Stange [:mstange]]

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

Comment 14

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc153acac9df

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for making test_leaf_layers_partition_browser_window.xul permafail on Win7/8 debug. Sorry it didn't get caught prior to being merged around.
https://treeherder.mozilla.org/logviewer.html#?job_id=10805348&repo=mozilla-inbound

https://hg.mozilla.org/mozilla-central/rev/5d9ffe1fcb5c

Comment 17

[Markus Stange [:mstange]]

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

Comment 18

[Markus Stange [:mstange]]

I investigated the test_leaf_layers_partition_browser_window.xul failure. Here's what happens:
On Windows, the <tabbrowser> has 1px of space on its left, right, and bottom edges. Browser XUL puts ColorLayers in those borders.
The PaintedLayer for the chrome contains all of the stuff in the toolbars at the top (total height is 87px), plus the <tabbrowser> background color (which has the 1px space around it). But in non-e10s we subtract the tabbrowser background color from the chrome PaintedLayer's visible region again because there's an opaque ContainerLayer for the tab content on top of it.

With tiled regions, however, we add the background color to the tiled region (which causes simplification) and then later remove it from the nsIntRegion that the tiled region gets converted to. After that, that PaintedLayer's visible region still contains two small strips an the very left and right of the window, 256px high.

An easy way of getting around this problem is to move the <tabbrowser> background color into a different layer, which is what I'm trying in bug 1262969.

Comment 19

[Markus Stange [:mstange]]

This code was removed and WR doesn't have this problem.