If scrollable, the RCD-RSF should get nonempty displayport margins on page load
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox85 | --- | unaffected |
firefox86 | --- | unaffected |
firefox87 | --- | fixed |
People
(Reporter: botond, Assigned: tnikkel)
References
(Regressed 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(2 files)
If the root content document's root scroll frame is scrollable (and therefore satisfies the "first-encountered async scrollable frame" check in MaybeCreateDisplayPort
), it should have nonempty displayport margins on page load.
This currently does not happen, at least in a test environment. We may have regressed this recently (to be confirmed).
Reporter | ||
Comment 1•4 years ago
|
||
Updated•4 years ago
|
Reporter | ||
Comment 2•4 years ago
|
||
Attached is a test that's currently failing.
Reporter | ||
Comment 3•4 years ago
|
||
Confirmed via Try that bug 1687927 is the regressing bug.
Note, bug 1687927 was the last to land out of {bug 1687886, bug 1687926, bug 1687927}, which leads me to believe that the way the RCD-RSF got nonempty displayport margins previously is via a repaint requested by APZ after the first paint.
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
Yeah, likely whichever of those 3 bugs that landed last would have caused this.
Assignee | ||
Comment 5•4 years ago
|
||
bug 1687886, bug 1687926, and bug 1687927 were fixing a regression. So did we have this bug before the regression was caused? Not sure if that's helpful to know or not.
Reporter | ||
Comment 6•4 years ago
|
||
I was curious about this as well, so I pushed to Try with the test patch applied on top of the commit just prior to the earliest of the three regressing bugs which those bugs fixed; however, it seems like something goes wrong when trying to run tests based on that older (May 2020) m-c.
It's certainly plausible that this issue has been around prior to those bugs, and it just never resulted in user-noticeable checkerboarding because during real usage something else would trigger a repaint soon after load.
Assignee | ||
Comment 7•4 years ago
|
||
Just linking the explanation for how this happens so it's easy to find https://bugzilla.mozilla.org/show_bug.cgi?id=1650183#c20
Assignee | ||
Comment 8•4 years ago
|
||
The InitializeRootDisplayport call probably comes from the before-first-paint notification which is fired from UnsuppressAndInvalidate in the presshell.
Assignee | ||
Comment 9•4 years ago
|
||
Come to think of it now the perfs win that bug 1691160 got could be because of this bug. I'll write a patch to have MaybeCreateDisplayPortInFirstScrollFrameEncountered consider zero margin display ports as needing a full display port and hopefully land it and we'll see if we regress those perf tests again.
Assignee | ||
Comment 10•4 years ago
|
||
Hmm, so with that patch the test fails with webrender. Here are the numbers I get locally.
The multiplier is 3.5. The client height is 863. So the expected display port height is 3020.5.
Without the patch the display port height is
1280 wr
1024 non-wr
With the patch the display port height is
1792 wr
3200 non-wr
On try server the height seems to be 2560.
Assignee | ||
Comment 11•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
Filed bug 1691355 as a result of auditing the code for other places that would want the same treatment.
Updated•4 years ago
|
Comment 13•4 years ago
|
||
Set release status flags based on info from the regressing bug 1687927
Reporter | ||
Comment 14•4 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
Hmm, so with that patch the test fails with webrender. Here are the numbers I get locally.
The multiplier is 3.5. The client height is 863. So the expected display port height is 3020.5.
Without the patch the display port height is
1280 wr
1024 non-wrWith the patch the display port height is
1792 wr
3200 non-wrOn try server the height seems to be 2560.
I suspect that's the result of this code, though the discrepancy seems fairly large.
Reporter | ||
Comment 15•4 years ago
|
||
The values I'm seeing in a local run are:
client height: 617
expected displayport height: 617 x 3.5 = 2159.5
actual displayport height (non-WR): 2304 (2159.5 aligned to 128x128)
actual displayport height (WR): 1792 (2159.5, adjusted by the linked code to 1388.25, then aligned to 256x256)
Assignee | ||
Comment 16•4 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #14)
I suspect that's the result of this code, though the discrepancy seems fairly large.
Yep, it's that code. Output from my printfs
alignmentMultipler 8 4 xMultiplier 1.500000 yMultiplier 3.500000 before
alignmentMultipler 8 4 xMultiplier 1.062500 yMultiplier 1.625000 after
Should we modify the test?
Reporter | ||
Comment 17•4 years ago
|
||
I don't really understand the "alignment multiplier" logic.
The GetDisplayportAlignmentMultiplier()
function computes an "alignment multiplier" which is 1, 2, 4, or 8, with larger viewports resulting in a larger multiplier.
On the content side, this multiplier is applied to the alignment, where it causes the alignment to be 128x128, 256x256, 512x512, or 1024x1024, respectively.
When computing the displayport size on the APZ side, however, the alignment multiplier is used to divide the displayport expansion multipliers (more specifically, the portion representing the margins, such that e.g. with an alignment multiplier of 2, a height multiplier of 3.5 (1 + 2.5 margins) becomes 2.25 (1 + 1.25 margins)).
These two computations are described as "corresponding", but I don't see the correspondence; halving the size of the initial margins can shrink the margins by a lot more, than increasing the alignment from 128x128 to 256x256 can grow them.
Reporter | ||
Comment 18•4 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #16)
Should we modify the test?
Yeah, I think it makes sense to avoid messing with the displayport sizing logic (strange as it seems to be) in this bug. I posted a proposed change to the test which should make it pass on WR as well.
Reporter | ||
Comment 19•4 years ago
|
||
Try push with updated test + fix.
Comment 20•4 years ago
|
||
Comment 21•4 years ago
|
||
Comment 22•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bd2b705457a3
https://hg.mozilla.org/mozilla-central/rev/a8c03c1368dd
Assignee | ||
Updated•4 years ago
|
Description
•