Open Bug 1169502 Opened 5 years ago Updated 3 years ago

Consider a different region simplification approach for visible regions

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

REOPENED

People

(Reporter: mstange, Assigned: mstange)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r=roc
Attachment #8612667 - Flags: review?(roc)
Attachment #8612667 - Flags: review?(jmuizelaar)
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
Attachment #8612667 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8612667 - Flags: review?(roc)
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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
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
Attachment #8612667 - Attachment description: MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r=roc → MozReview Request: Bug 1169502 - Simplify visible regions to tiles, not to a number of rects. r=jrmuizel, r?roc
Attachment #8612667 - Flags: review+ → review?(roc)
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!
Attachment #8612667 - Flags: review?(roc) → review+
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));
Attachment #8612667 - Flags: review+ → review?(roc)
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 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!
https://hg.mozilla.org/mozilla-central/rev/cc153acac9df
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla41 → ---
Depends on: 1262969
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.
You need to log in before you can comment on or make changes to this bug.