Consider aligning/snapping the displayport to 'virtual' tiles where we don't tile

RESOLVED FIXED in Firefox 44

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
We already snap the displayport on platforms where we tile. But I think it would be better if we snapped to tiles everywhere. We have a high overhead to painting (building a displaylist, recomputing a layer tree), so we might be able to count down the CPU time as we scroll.

Also this would have the benefit of removing a difference between our tiling and non tiling platforms. This would make the code, and more importantly, the performance characteristics more similar.
(Assignee)

Updated

3 years ago
Assignee: nobody → bgirard
Whiteboard: gfx-noted
(Assignee)

Comment 1

3 years ago
Soft depends on bug 1191539 for code conflicts.
Depends on: 1191539
(Assignee)

Comment 3

2 years ago
This *may* help with the APZ scrolling regression on Linux
(Assignee)

Comment 4

2 years ago
Created attachment 8671560 [details] [diff] [review]
patch
Attachment #8671560 - Flags: review?(botond)
Comment on attachment 8671560 [details] [diff] [review]
patch

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

Looks pretty good, thanks! I'd like to have a brief look at an updated version before r+'ing.

::: layout/base/nsLayoutUtils.cpp
@@ +950,5 @@
> +    alignmentX = gfxPlatform::GetPlatform()->GetTileWidth();
> +    alignmentY = gfxPlatform::GetPlatform()->GetTileHeight();
> +  } else {
> +    // If we're not drawing with tiles then we need to be careful about not
> +    // hitting the max texture size and we only need 1 draw call per the layer

"1 draw call per the layer" doesn't parse :)

@@ +964,5 @@
>      // Inflate the rectangle by 1 so that we always push to the next tile
>      // boundary. This is desirable to stop from having a rectangle with a
>      // moving origin occasionally being smaller when it coincidentally lines
>      // up to tile boundaries.
>      screenRect.Inflate(1);

We probably want to do this for the non-tiling case as well. I'd move this down to just before the alignment.

@@ +971,5 @@
>      if (alignmentX == 0) {
>        alignmentX = 1;
>      }
>      if (alignmentY == 0) {
>        alignmentY = 1;

We probably want to use 128 in this case as well. I'd move this check up like so:

int alignmentX = 0;
int alignmentY = 0;

if (gfx::Prefs::LayersTilesEnabled()) {
  alignmentX = ...;
  alignmentY = ...;
}

if (alignmentX == 0) {
  alignmentX = 128;
}
if (alignmentY == 0) {
  alignmentY = 128;
}

@@ +978,5 @@
> +  }
> +
> +
> +  ScreenPoint scrollPosScreen = LayoutDevicePoint::FromAppUnits(scrollPos, auPerDevPixel)
> +                              * res;

If you move this to just above where it's first used, you can make the next block an |else| following the above |if|.

@@ +987,5 @@
>      nscoord maxSizeInAppUnits = GetMaxDisplayPortSize(aContent);
>      if (maxSizeInAppUnits == nscoord_MAX) {
>        // Pick a safe maximum displayport size for sanity purposes. This is the
>        // lowest maximum texture size on tileless-platforms (Windows, D3D10).
> +      maxSizeInAppUnits = presContext->DevPixelsToAppUnits(8192 - 3 * alignmentX);

Technically, as alignmentX and alignmentY can be different, we should be doing a calculation with each. We already split it into separate width and height variables below due to the resolution potentially having different x and y scales, might as well start doing that up here.

Also, please add a short comment explaining the "3".

@@ +1029,5 @@
> +    LayoutDeviceRect::FromAppUnits(expandedScrollableRect,
> +                                   auPerDevPixel) * res;
> +
> +  // Make sure the displayport remains within the scrollable rect.
> +  screenRect = screenRect.ForceInside(screenExpScrollableRect - scrollPosScreen);

As we clamp to the expanded scrollable rect below, I think it's redundant to also do it here; let's just remove this.
Attachment #8671560 - Flags: review?(botond) → feedback+
(Assignee)

Comment 6

2 years ago
(In reply to Botond Ballo [:botond] from comment #5)
> @@ +1029,5 @@
> > +    LayoutDeviceRect::FromAppUnits(expandedScrollableRect,
> > +                                   auPerDevPixel) * res;
> > +
> > +  // Make sure the displayport remains within the scrollable rect.
> > +  screenRect = screenRect.ForceInside(screenExpScrollableRect - scrollPosScreen);
> 
> As we clamp to the expanded scrollable rect below, I think it's redundant to
> also do it here; let's just remove this.

Note that ForceInside is not the same as clamping. ForceInside prevents the displayport from shrinking between frames.

Since the clamping and the ForceInside are using different computation I'd rather not touch it to minimize the risk of regressions.
(Assignee)

Comment 7

2 years ago
Created attachment 8672125 [details] [diff] [review]
patch
Attachment #8671560 - Attachment is obsolete: true
Attachment #8672125 - Flags: review?(botond)
(In reply to Benoit Girard (:BenWa) from comment #6)
> (In reply to Botond Ballo [:botond] from comment #5)
> > @@ +1029,5 @@
> > > +    LayoutDeviceRect::FromAppUnits(expandedScrollableRect,
> > > +                                   auPerDevPixel) * res;
> > > +
> > > +  // Make sure the displayport remains within the scrollable rect.
> > > +  screenRect = screenRect.ForceInside(screenExpScrollableRect - scrollPosScreen);
> > 
> > As we clamp to the expanded scrollable rect below, I think it's redundant to
> > also do it here; let's just remove this.
> 
> Note that ForceInside is not the same as clamping. ForceInside prevents the
> displayport from shrinking between frames.
> 
> Since the clamping and the ForceInside are using different computation I'd
> rather not touch it to minimize the risk of regressions.

Good point, I overlooked that.
Comment on attachment 8672125 [details] [diff] [review]
patch

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

Thanks! r+ with coordinate space issue addressed.

::: layout/base/nsLayoutUtils.cpp
@@ +961,5 @@
> +  if (alignmentX == 0) {
> +    alignmentX = 1;
> +  }
> +  if (alignmentY == 0) {
> +    alignmentY = 1;

Is there a reason we're not using 128 here?

@@ +981,5 @@
>        // Pick a safe maximum displayport size for sanity purposes. This is the
>        // lowest maximum texture size on tileless-platforms (Windows, D3D10).
> +      maxSizeInAppUnits = std::min(
> +        presContext->DevPixelsToAppUnits(8192 - MAX_ALIGN_ROUNDING * alignmentX),
> +        presContext->DevPixelsToAppUnits(8192 - MAX_ALIGN_ROUNDING * alignmentY));

I just realized, alignmentX/Y are in Screen coordinates but we're using them as Device coordinates.

To fix this, we can do the subtraction below, where we calculate maxWidthInScreenPixels / maxHeightInScreenPixels.

It might also be nice to change the type of alignmentX/Y to ScreenCoord, to make it clearer what coordinate space they're in.
Attachment #8672125 - Flags: review?(botond) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8673155 [details] [diff] [review]
patch
Attachment #8672125 - Attachment is obsolete: true
Attachment #8673155 - Flags: review+
Comment on attachment 8673155 [details] [diff] [review]
patch

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

::: layout/base/nsLayoutUtils.cpp
@@ +974,5 @@
>        // Pick a safe maximum displayport size for sanity purposes. This is the
>        // lowest maximum texture size on tileless-platforms (Windows, D3D10).
> +      maxSizeAppUnits = std::min(
> +        presContext->DevPixelsToAppUnits(8192),
> +        presContext->DevPixelsToAppUnits(8192));

No need to take a min of a value with itself here.
(Assignee)

Comment 14

2 years ago
Created attachment 8673269 [details] [diff] [review]
patch
Attachment #8673155 - Attachment is obsolete: true
Attachment #8673269 - Flags: review+
Backing this out to see if it fixes some m-e10s(5) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=15725583&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/347d1f4ccbe1
Flags: needinfo?(bgirard)
(Assignee)

Updated

2 years ago
Depends on: 1215596
https://hg.mozilla.org/mozilla-central/rev/6f5bc0795627
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Updated

2 years ago
Flags: needinfo?(bgirard)
Depends on: 1216924
You need to log in before you can comment on or make changes to this bug.