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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Benoit Girard (:BenWa)]

Soft depends on bug 1191539 for code conflicts.

Comment 2

[Benoit Girard (:BenWa)]

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

Comment 3

[Benoit Girard (:BenWa)]

This *may* help with the APZ scrolling regression on Linux

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 5

[Botond Ballo [: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.

Comment 6

[Benoit Girard (:BenWa)]

(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.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 8

[Botond Ballo [: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 9

[Botond Ballo [:botond]]

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.

Comment 10

[Benoit Girard (:BenWa)]

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

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 12

[Botond Ballo [:botond]]

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.

Comment 13

[Benoit Girard (:BenWa)]

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

Comment 14

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 15

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fe97ae33f4887ba73e2fa7ae7ba764d3e40119
Bug 1204136 - Align DisplayPort on non-tiling platform. r=botond

Comment 16

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

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

Comment 17

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5bc07956271a643b581fc2bc716e6df8544e17
Bug 1204136 - Align DisplayPort on non-tiling platform. r=botond

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f5bc0795627

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f5bc0795627