Consider a different region simplification approach for visible regions

RESOLVED INACTIVE

Status

()

Core
Layout
RESOLVED INACTIVE
3 years ago
9 hours ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created 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?(roc)
Attachment #8612667 - Flags: review?(jmuizelaar)
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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)
(Assignee)

Comment 6

3 years ago
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));
(Assignee)

Updated

3 years ago
Attachment #8612667 - Flags: review+ → review?(roc)
(Assignee)

Comment 9

3 years ago
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
Last Resolved: 3 years ago
status-firefox41: affected → fixed
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
status-firefox41: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla41 → ---
(Assignee)

Updated

2 years ago
Depends on: 1262969
(Assignee)

Comment 18

2 years ago
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.
Blocks: 1256677

Comment 19

9 hours ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago9 hours ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.