Closed Bug 1204136 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 3 obsolete files)

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: nobody → bgirard
Whiteboard: gfx-noted
Soft depends on bug 1191539 for code conflicts.
Depends on: 1191539
This *may* help with the APZ scrolling regression on Linux
Attached patch patch (obsolete) — Splinter Review
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+
(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.
Attached patch patch (obsolete) — Splinter Review
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+
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch patchSplinter Review
Attachment #8673155 - Attachment is obsolete: true
Attachment #8673269 - Flags: review+
Depends on: 1215596
https://hg.mozilla.org/mozilla-central/rev/6f5bc0795627
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.