Closed Bug 1191539 Opened 4 years ago Closed 4 years ago

APZ DisplayPort appears to shrink incorrectly during sync scrolling

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(3 files)

STR:
1) Turn on the minimap.
2) Scroll on some pages long pages near the top/bottom with the scrollbar.

you will notice that, at least according to the minimap, we're shrinking the display port incorrectly and temporarily. If this is correct we're throwing out valid content and repainting it later.

I see this on gmail.com in my inbox list. The display port is large enough to fit the entire page without repainting but this bug might be causing us to paint as we scroll anyways.

I don't get this using the trackpad scrolling.
Attached image bug-dp.gif
It's pretty hard to tell from your gif but most likely what you're seeing here is that when the user scrolls fast we shrink the displayport margins to paint faster and keep up better. It will perform worse in cases where the user scrolls super-fast and then abruptly reverses direction or stops scrolling but that should be a less common scenario than scrolling super-fast and letting it drift to a slower speed.
Version: 39 Branch → Trunk
I'm not sure that's it but it's hard to tell. I don't think it's even implemented yet on mac. It also only occurs with scrollbar and not flings.

Also there's really trivial scenarios that it should be handling without hitting this bug. If the page is just the right size the entire page fits within the display port. I still see us shrink the displayport when we have the entire page painted.
Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats
Attachment #8653034 - Flags: review?(bugmail.mozilla)
Assignee: nobody → bgirard
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review15503

::: layout/base/nsLayoutUtils.cpp:923
(Diff revision 1)
> +  nsRect frameRect = nsLayoutUtils::CalculateExpandedScrollableRect(frame);

Please call this expandedScrollableRect or scrollableRect rather than frameRect. The frame rect implies something different.

::: layout/base/nsLayoutUtils.cpp:926
(Diff revision 1)
> +  ScreenPoint screenScrollPos = LayoutDevicePoint::FromAppUnits(scrollPos,

To reduce rounding problems I would prefer if you subtracted the scrollPos in app units and then did a single conversion to Screen pixels.
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats
Attachment #8653034 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review15601

Thanks!
Attachment #8653034 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review15601

Thanks!
https://hg.mozilla.org/mozilla-central/rev/758f3df6c016
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
This caused a regression in the tcheck numbers: bug 1199683.
Depends on: 1200228
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
we lost some nice e10s improvements to windows tp5 scroll and tscroll-asap, but fixed an android tcheck2 regression:
http://alertmanager.allizom.org:8080/alerts.html?rev=f25ab0817babf1dcbf71d9e46f80c85ca9b55cdb&showAll=1&testIndex=0&platIndex=0 (the regressions here are items that were improvements a few days ago)
Still trying to fix this. It's possible the proper patch to do this wont have tcheck2 regressions:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a0c037437d6
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

I’m not sure how to re-request review with MozReview. I no longer ForceInside if we’re not tiling because it doesn’t work well and I fixed the emulator oranges + perma orange issue.

I think in the future we're going to want to move the displayport in increments (even if we don't tile) so I'm not overly interested in fixing that.
Attachment #8653034 - Flags: review+ → review?(bugmail.mozilla)
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

This patch is wrong. I need to think about this some more.
Attachment #8653034 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats
Attachment #8653034 - Flags: review?(bugmail.mozilla)
Haven't looked at the new patch yet but https://bugzilla.mozilla.org/show_bug.cgi?id=1194851#c15 might explain why you've been having so much trouble with this...
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review16907

::: layout/base/nsLayoutUtils.cpp:925
(Diff revision 4)
> +  ScreenRect screenExpScrollableRect =

Move this into the if (tiles) block since it's not used anywhere outside of it

::: layout/base/nsLayoutUtils.cpp:929
(Diff revision 4)
> +  ScreenPoint scrollPosScreen = LayoutDevicePoint::FromAppUnits(scrollPos, auPerDevPixel)

Move this back inside the if (tiles) block where it was before; it's not used outside anywhere

::: layout/base/nsLayoutUtils.cpp:967
(Diff revision 4)
> -    float w = alignmentX * ceil(screenRect.XMost() / alignmentX) - x;
> +    float w = alignmentX * ceil(screenRect.width / alignmentX + 1);

Can you provide examples of numbers where this produces a more desirable result? As far as I can tell this will give a width one tile wider than before in most cases and I don't see why that is desirable.

e.g. for alignmentX = 10, screenRect.x = 12, screenRect.width = 83. This used to give x = 10, w = 90 and with your change it gives x = 10, w = 100.

And regardless it seems this change is orthogonal to everything else in this patch and should be moved to a separate bug if there's a good reason for it.
Attachment #8653034 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> ::: layout/base/nsLayoutUtils.cpp:967
> (Diff revision 4)
> > -    float w = alignmentX * ceil(screenRect.XMost() / alignmentX) - x;
> > +    float w = alignmentX * ceil(screenRect.width / alignmentX + 1);
> 
> Can you provide examples of numbers where this produces a more desirable
> result? As far as I can tell this will give a width one tile wider than
> before in most cases and I don't see why that is desirable.

The problem is we're aligning the two edges independently from each other. In practice (as show by the minimap) this means that when you scroll, we will shrink the display port on one frame, then increase it after scrolling a few pixels. If the unaligned displayport isn't changing size, we shouldn't change the size of the aligned display port. This is bad for recycling and performance.

> And regardless it seems this change is orthogonal to everything else in this
> patch and should be moved to a separate bug if there's a good reason for it.

It's also causing the display port to change size sporadically causing it to disagree with the compositor and shrink. It's required to fix this bug.
(In reply to Benoit Girard (:BenWa) from comment #21)
> The problem is we're aligning the two edges independently from each other.
> In practice (as show by the minimap) this means that when you scroll, we
> will shrink the display port on one frame, then increase it after scrolling
> a few pixels.

Can you provide specific numbers that illustrate this problem?
patch (to make the logging clearer):

     screenRect += scrollPosScreen;
+    printf("Before %s\n", ToString(screenRect).c_str());
     float x = alignmentX * floor(screenRect.x / alignmentX);
     float y = alignmentY * floor(screenRect.y / alignmentY);
     float w = alignmentX * ceil(screenRect.XMost() / alignmentX) - x;
     float h = alignmentY * ceil(screenRect.YMost() / alignmentY) - y;
     screenRect = ScreenRect(x, y, w, h);
+    printf("After %s\n", ToString(screenRect).c_str());
     screenRect -= scrollPosScreen;

Before (-1,3252.78,1887,6977.5)
After (-512,3072,2560,7168)

Before (-1,3283.48,1887,6977.5)
After (-512,3072,2560,7680)

Problem is pretty clear from watching the minimap and it matches up with the invalidation flashes.

If we prefer not rounding out but snapping to the closest point I can do that implementation instead if that's your concern.
Thanks! I understand the problem better now, and I agree that your changes there are fine. Rounding out should be ok. r+ with the other nits from comment 20 addressed.
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats
Attachment #8653034 - Flags: review?(bugmail.mozilla)
Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

I addressed the comment 20 nits. This should be r=kats.
Attachment #8653034 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a259b679e13556208f53378ad0b1ad312c68c672
Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats
Blocks: 1204136
https://hg.mozilla.org/mozilla-central/rev/a259b679e135
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
The patch is triggered major (10%-20%) TP5/Page switch regressions. Those test would be impacted by a larger displayport while not gaining any benefit. This is probably because we're rounding-out the display port. We will have to do an adjustment. I didn't think the results would be so severe.
Alright I checked locally. I don't think the problem is the rounding anymore.

I think the issue is that we ForceInside which moves the displayport down during the page load. Effectively this patches causes us to paint a displayport on page load where before we effectively had a suppressed displayport.
If we don't mind I'd like for us to accept this regression. Before we were getting the right thing we want by accident.

I filed bug 1205025 to intentionally suppress the displayport.
(Meta-comment: I feel like even when we are able to reproduce these regressions locally it's pretty hard to be certain that we can diagnose them properly... do we need more tooling to be able to track down these perf regressions with certainty?)
Attached patch patchSplinter Review
Looking bad, the + 1 wasn't necessary in the end. It's preventing the displayport suppression from properly working, adding an extra row of tiles below.
Attachment #8661445 - Flags: review?(bugmail.mozilla)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8661445 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/61baabe617c9786b3ce315709a39fa17e78b15fc
Bug 1191539 - Don't further increase the size of the displayport when rounding up. r=kats
https://hg.mozilla.org/mozilla-central/rev/61baabe617c9
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
and the regressions are gone!
Comment on attachment 8661445 [details] [diff] [review]
patch

This doesn't work the way you intended it to. Try it with an alignment of 10, 10 and a rect of (9, 9, 10, 10).
Attachment #8661445 - Flags: review-
are we going to accept these tps and tp5 main rss regressions on osx:
http://alertmanager.allizom.org:8080/alerts.html?rev=6e112d30103e1f13d93f3380d83c50485b30b172&showAll=1&testIndex=0&platIndex=0

I am fine either way, it looks like we tried and then backed things out.  Do let me know what the plan is.
I think we should accept the regressions at least until Benoit comes back from vacation and has a chance to take another look.
sounds good, we will let BenWa make the call here!
Merge of the backout of the last part: https://hg.mozilla.org/mozilla-central/rev/6e112d30103e

The bug as filed is still fixed. We'll need to fix the perf regressions in a separate bug.
Depends on: 1223639
Bug 957668 used Intersect when it should have used ForceInside, which is what caused this bug, marking dependency.
Blocks: 957668
You need to log in before you can comment on or make changes to this bug.