Align the displayport differently
Categories
(Core :: Graphics, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox82 | --- | fixed |
People
(Reporter: nical, Assigned: kats)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
We currently align the position and the size of the displayport separately. To make sure that the aligned displayport contains the original one we should floor the min position and ceil the max position.
In addition we use different alignments for the position and size. This is because webrender wants large alignements of the position to avoid frequent displaylist and scene builds, but enlarging the size makes for larger displaylists and scenes, causing large tscrollx regressions.
The combination of these two things forces us to deal with corner cases to make sure the alignment doesn't move the displayport by more than its margin and other factors.
It would be less error prone if we could pick a single algnment and round out the unaligned DP with it.
To do that we need to either accept a large tscrollx regression or figure out how to improve scene building performance (on that test). In any case it would be useful to better understand why this test is so sensible to displayport sizes.
|
Assignee | |
Comment 1•5 years ago
|
||
Does WR care if the aligned displayport size flip-flops from one paint to the next? Or does it only care about the position? i.e. if we did something like this:
paint 1: y=0, h=1024
paint 2: y=0, h=1152
paint 3: y=0, h=1280
paint 4: y=0, h=1408
paint 5: y=512, h=1024
so that the position moves by 512 increments and the size moves by 128 increments, resulting in the size changing each paint. Would WR behave will in that scenario or not?
|
Reporter | |
Comment 2•5 years ago
|
||
If the size changes we have to rebuild the display list and rebuild the scene which is pretty expensive. Whenever we change the size we might as well change the position, and the more often we do either, the more load we put on the CPU.
|
|
Assignee | |
Comment 3•5 years ago
|
||
In https://bugzilla.mozilla.org/show_bug.cgi?id=1635472#c7 nical proposed a couple of approaches to deal with the tscrollx regression from using a larger alignment for the displayport for WR. I came up with another option that I think should work well. It boils down to scaling the alignment with the size of the scrollable frame. So the larger the scrollable frame, the larger alignment we will use. This should work well for WR because the concern is mostly around large scrollers, and having a large displayport shift frequently. With this change those large displayports will shift less frequently. Smaller scrollers with smaller displayports will shift frequently but will be cheaper to render so won't matter as much.
In addition, I'm scaling the displayport multipliers down for the these large scrollers, because the large alignment inherently causes the final displayport to be much bigger than the unaligned one. This helps mitigate the performance impact of overly-large displayports.
Also note that tscrollx has been disabled since early August (bug 1651311 comment 18) but I'm going to try and re-enable it. I'm not sure if that will happen before or after making this change, but I'm hopeful that either way this change won't introduce too much of a tscrollx regression.
Current WIP is at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c77c43541f14b8df410bf0807eedae94a3480814 - nical, if you have some time to download a build and try it out that would be great. There's still a gtest failure there that I need to address, and do more manual testing but it's a fairly complete WIP with test.
|
|
Assignee | |
Comment 4•5 years ago
|
||
Whoops, I just noticed I sent the patches to bug 1648641 instead of this bug as I had intended. Will fix that shortly. Also here is a perfherder comparison with and without the patches: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7c9ef88542bcce5dc0d388dab817d9cc7557a29e&newProject=try&newRevision=31aad1bb65077773908493661d05e57a9fbe02ab&framework=1
No significant impact on tscrollx, as I had hoped.
|
|
Assignee | |
Comment 5•5 years ago
|
||
|
|
Assignee | |
Comment 6•5 years ago
|
||
No functional changes here, just refactoring code that is already duplicated
in a bunch of tests.
|
|
Assignee | |
Comment 7•5 years ago
|
||
This ensures that even with the displayport margins dropped to zero we don't
get any checkerboarding. This currently fails with WR enabled, because the
displayport alignment code moves the displayport so it doesn't enclose the
unaligned displayport, and that's bad.
|
|
Assignee | |
Comment 8•5 years ago
|
||
The next patch requires gfxVars::UseWebRender to work in gtests, so we need
to make sure gfxPlatform is initialized correctly for all the tester classes.
|
|
Assignee | |
Comment 9•5 years ago
|
||
The displayport is aligned to screen pixels by an alignment number; there was
code to increase that alignment number for the WebRender codepath. This is
desirable because it causes the displayport to move less frequently, which
provides better performance characteristics with WebRender. However, the
way that was implemented, it was possible for the aligned displayport to not
completely enclose the unaligned displayport, which could then result in
perma-checkerboarding.
This patch removes that code and replaces it with a simpler approach to scale
up the existing alignment number by a factor based on the size of the scroller's
base rect (roughly how much of the scroller is visible to the user).
This does make the displayports larger, more so for scrollers with more visible
area. This has negative performance implications, so we mitigate that by also
scaling down the displayport margin multiplier by the same scaling factor. This
isn't perfect, but seems to work well enough in practice.
|
|
Assignee | |
Comment 10•5 years ago
|
||
Updated try push for correctness: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c483e1d7a4a60b6ed2d42ea13740d9476987a2af
and performance: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=d33a4deb1d75b8b67b5fd58810e7e4737ab53481
Not all the talos jobs are done but linux-qr and macosx-qr are not regressing vs the baseline:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7c9ef88542bcce5dc0d388dab817d9cc7557a29e&newProject=try&newRevision=d33a4deb1d75b8b67b5fd58810e7e4737ab53481&framework=1
So I'm going to go ahead and land.
|
||
Comment 11•5 years ago
|
||
|
||
Comment 12•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/e7fa30480419
https://hg.mozilla.org/mozilla-central/rev/146ebd1dd27b
https://hg.mozilla.org/mozilla-central/rev/8b4f9898cdce
https://hg.mozilla.org/mozilla-central/rev/5a2f1cfc33ad
https://hg.mozilla.org/mozilla-central/rev/a39390791308
Description
•