Closed Bug 1691160 Opened 4 years ago Closed 4 years ago

If scrollable, the RCD-RSF should get nonempty displayport margins on page load

Categories

(Core :: Panning and Zooming, defect)

defect

Tracking

()

RESOLVED FIXED
87 Branch
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).

Assignee: nobody → botond
Status: NEW → ASSIGNED

Attached is a test that's currently failing.

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.

Regressed by: 1687927
Has Regression Range: --- → yes

Yeah, likely whichever of those 3 bugs that landed last would have caused this.

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.

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.

Just linking the explanation for how this happens so it's easy to find https://bugzilla.mozilla.org/show_bug.cgi?id=1650183#c20

The InitializeRootDisplayport call probably comes from the before-first-paint notification which is fired from UnsuppressAndInvalidate in the presshell.

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.

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.

Flags: needinfo?(botond)

Filed bug 1691355 as a result of auditing the code for other places that would want the same treatment.

Set release status flags based on info from the regressing bug 1687927

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

With the patch the display port height is
1792 wr
3200 non-wr

On try server the height seems to be 2560.

I suspect that's the result of this code, though the discrepancy seems fairly large.

Flags: needinfo?(botond)

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)

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

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.

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

Try push with updated test + fix.

Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bd2b705457a3 Make MaybeCreateDisplayPortInFirstScrollFrameEncountered consider a zero margin display a not having a display port so it sets one. r=botond
Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a8c03c1368dd Add a test that checks that the RCD-RSF has nonempty displayport margins on page load. r=tnikkel
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Regressions: 1692226
Assignee: botond → tnikkel
Regressions: 1692426
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: