1729811 - wr displayport alignment code acts weirdly

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Timothy Nikkel (:tnikkel)]

Our current displayport code scales down the displayport multiplier based on the size of the composition bounds, the larger the bounds the more we scale the displayport mutliplier down.

https://searchfox.org/mozilla-central/rev/08c493902519265d570250c8e7ce575c8cd6f5b5/gfx/layers/apz/src/AsyncPanZoomController.cpp#4037

On the content side when we compute the displayport rect we use the same [1] multiplier to determine the multiple of 128 to align the display port rect to.

https://searchfox.org/mozilla-central/rev/08c493902519265d570250c8e7ce575c8cd6f5b5/layout/base/DisplayPortUtils.cpp#359

The code comment in AsyncPanZoomController.cpp linked above says that these operations should roughly cancel out. However they do not, there can be a significant difference. For example, for composition bounds between 500 and 1000 we lose between 113-994 pixels, with the average in the middle.

The current code in DisplayPortUtils.cpp linked above says the reason for how this works is "Moving the displayport is relatively expensive with WR so we use a larger alignment that causes the displayport to move less frequently". My memory is that this code was also involved in some talos regressions and was tweaked to fix them, so it could be that decreasing the displayport size is also part of what fixed the talos regressions.

Now this might be fine since the displayport multiplier was tuned for non-wr code and wr is very different so we might want a different multiplier for wr. Calculating it this way though has one problem in that if all of the inputs to this algorithm remain constant except the dev pixels to css pixels ratio then the computed display port changes. Specifically the displayport will be smaller on a higher dpi screen. This seems undesirable, it should be the same. (This happens because the alignment multiplier is determined based on screen pixels.)

[1] the multiplier can be different because we use the composition size to calculate the displayport margins, but we use the base (visible) rect to turn the margins into a displayport rect. This effects seems to have a more modest effect that what is described above though.

Comment 1

[Botond Ballo [:botond]]

S3 as the expected impact is a fairly minor one on displayport sizing.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I poked those displayport stuff both on the content and APZ sides while running the mochitest attached in bug 1671719. (with
apz.y_stationary_size_multiplier=1.5 which is the default value on desktops)

The result is;

DPI	size on APZ	size on content
1.0	720 x 566.875	1024 x 768
2.0	612 x 510.469	1024 x 640
3.0	453.333 x 402.51	682.667 x 682.667
4.0	340 x 278.109	512 x 512

In the case of DPI=4.0 the displayport height on APZ is almost half of the DPI=1.0 case, whereas the height on content side is still 2/3 of the DPI=1.0 case, so the difference gets bigger than higher DPI environments as Timothy mentioned in comment 0.

I guess this divergence comes from the fixed 128 value on the content size, but I am quite unsure Timothy suggested the 128 should be divided by DPI. Or the calculation of the alignment multiplier should be computed independent from DPI, i.e. in CSS pixel units?

Timothy?

Comment 3

[Timothy Nikkel (:tnikkel)]

This is from a long time ago. It'll take me a bit to page this all in. You might be able to figure out a solution quicker then I can respond.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

I guess this divergence comes from the fixed 128 value on the content size, but I am quite unsure Timothy suggested the 128 should be divided by DPI.

I tried this approach, the result is;

DPI	size on APZ	size on content
1.0	720 x 566.875	896 x 640
2.0	612 x 510.469	704 x 544
3.0	453.333 x 402.51	483.55 x 455.117
4.0	340 x 278.109	368 x 304

So now the size on the content is closer to the APZ one. It looks reasonable to me.

That's being said, there's a concern that we've been using the relatively larger displayport on higher DPI environments, specifically Android devices have higher DPIs so with this approach we might see more checkerboarding there?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1729811 - Make GetDisplayportAlignmentMultiplier function continuous rather than descrete. r?tnikkel

Comment 6

[Hiroyuki Ikezoe (:hiro)]

D245181 is the approach I used in comment 4 result.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I pushed perf runs with D245181; https://perf.compare/compare-results?baseRev=7af787614182152eeb6f92d91baf541a077badae&newRev=e73019193982fdde958192d7441c3092cb0d6893&baseRepo=try&newRepo=try&framework=1

Though I expected that D245181 improves talos on Android, unfortunately we haven't yet run talos on Android. Given that there's no improvements/regressions, our Macs on CI uses DPI=1.0.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I wrote a mochitest and it made me confident that the approach is reasonable.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)

Though I expected that D245181 improves talos on Android, unfortunately we haven't yet run talos on Android. Given that there's no improvements/regressions, our Macs on CI uses DPI=1.0.

Note that I've heard that there's no plan to run talos on Android.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I did try to run helper_displayport_estimation.html with apz.y_stationary_size_multiplier=1.5 just like as comment 2.

With the revised version (without using multiplier on the main-thread) there the results are;

DPI	size on APZ	size on content
1.0	720 x 566.875	832 x 640
2.0	540 x 506.531	576 x 544
3.0	360 x 297.188	384 x 320
4.0	270 x 187.875	288 x 208

For some reasons, the sizes on APZ have been changed since comment 2, I guess the reason is the notification "Some of Nightly's security features mau offer less ..." now occupies the content area.

Anyways the results without D245181 as of 80cc20e2740d revision are;

DPI	size on APZ	size on content
1.0	720 x 566.875	1024 x 768
2.0	540 x 506.531	768 x 640
3.0	360 x 297.188	512 x 426.667
4.0	270 x 187.875	384 x 256

So, I would say D245181 will improve the displayport size close to the APZ one.

Comment 11

[Timothy Nikkel (:tnikkel)]

Do you have the composition size that these are being calculated from?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

The composition size is;

DPI	composition size (on APZ)
1.0	640 x 453.5
2.0	480 x 450.25
3.0	320 x 264.167
4.0	240 x 167

To me the height on DPI=2.0 looks taller, I don't know the reason.

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: Screenshot 2025-05-16 at 10.08.16 PM.png

Thanks for that. The actual numbers helps me remember what I was getting at when I originally filed this bug. Sorry that it took me so long to figure that out.

I think the composition bounds is key here because the display size that we compute on the apz side is not intended to be the final display port size. The code comment

https://searchfox.org/mozilla-central/rev/3a5ab81ad35fa4ab14ae75bf7549c8996ca49008/gfx/layers/apz/src/AsyncPanZoomController.cpp#4579

says that we are scaling down the multipliers there so that when we do the alignment bit on the content side we don't expand past what the apz code intends for the display port. So in other words, this apz display port code is making the displayport smaller in that calculation so that when we do the alignment later it will come out closer to what it intended.

When we look at the numbers you provided it seems like it is actually doing a pretty good job at staying around 1.5 multiplier. It gets 1.76, 1.42, 1.61, 1.53 multipliers. The first one is a bit high but the others seems reasonable. Although there is room for improvement this experiment seems to show that this code is working close to how we intend.

When I said in comment 0 "there can be a significant difference. For example, for composition bounds between 500 and 1000 we lose between 113-994 pixels", I think that calculation was keeping everything fixed except for the composition bounds, which is a situation that is more applicable to desktop where we can have windows and scrollable areas that take up significantly less than the full screen. On mobile our window is usually the whole screen, and scrollable smaller than that tend not to contain the main content and where checkerboarding isn't a big worry.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I am afraid, the values might not be suitable for thinking about normal state. The test I've used to collect values are scaled by 2.0x, i.e. the values are on a document that the document was pinch-zoomed in by 2.0x.

I did recollect without the zoom (and without applying D245181)

On Desktop with y_stationary_size_multiplier=3.5 (default)

DPI	displayport (APZ)	displayport (content)	composition	apz factor	content factor
1	1440 x 2040.75	1280 x 2304	1280 x 907	2.25	2.540242558
2	1080 x 1463.31	960 x 900.5	960 x 900.5	1.624997224	1.990005552
3	720 x 858.542	640 x 1194.67	640 x 528.333	1.625001656	2.261206474
4	540 x 542.75	480 x 768	480 x 334	1.625	2.299401198

On mobile with y_stationary_size_multiplier=1.5 (default) on 1080x1920 screen device (I used adb -s emulator-5554 shell wm size 1080x1920)

DPI	displayport (APZ)	displayport (content)	composition	apz factor	content factor
1	1215 x 1998	1080 x 2560	1080 x 1776	1.125	1.441441441
2	607.5 x 999	540 x 1280	540 x 888	1.125	1.441441441
3	405 x 666	360 x 853.333	360 x 592	1.125	1.441440878
4	303.75 x 499.5	270 x 640	270 x 444	1.125	1.441441441

(note that the original y_stationary_size_multiplier value was wrong. on Desktop it's 3.5, on mobile it's 1.5. I did re-collect again)

Comment 15

[Timothy Nikkel (:tnikkel)]

Thanks for doing that testing and producing that data!

The mobile ones seem pretty reasonable. The desktop ones are a little inconsistent, and a little under sized (compared to the multiplier). What do you think?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Yeah, agree. The desktop factor numbers look small on both of APZ and content, given that the multiplier is 3.5. Though the values with DPI=1.0 look relatively reasonable. So to me, if, on desktops, the factor values were approximately equal to the DPI=1.0 ones, that would be our goal here? I am totally unsure how to tweak the calculation though.

One thing I should note that even on desktop with DPI higher than 1.0 the browser window size is fullscreen because of the DPI.

Comment 17

[Timothy Nikkel (:tnikkel)]

I think the weirdness that I observed in comment 0 is easier to observe if you increase the comp bounds size 1 by 1. Ie if they are plotted. Because there would be big discontinuous jumps when the comp bounds crosses certain sizes. Looking at the computed content display port multiplier around these points would be interesting.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch adds a gtest dumping composition height and displayport height

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: The result of the gtest in the previous comment

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: A chart showing the result

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Now I completely understand what Timothy meant.

In the chart in comment 20, you can see 3 cliffs at composition height is 500, 100 and 2000.

Anyways, the line is basically linear so ideally we should use a single fixed value just like the y_stationary_size_multiplier pref, that's the original intention?

So I wonder whether we can directly use the 1.625 without the alignment, with 1.625 we will regress in most cases though. Or just using 1.0 which is calculated by (the displayport height (3250) at 2000 - the displayport height (1750) at 500) / (2000 - 500).

Well, 1.0 would not be applicable, since it will not expand the composition bounds at all.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: A chart with the continuos function

Okay I figured out;

The GetDisplayportAlignmentMultiplier returns;

when the composition size is between 501 and 1000 it returns 2
when the composition size is between 1001 and 2000 it returns 4
when the composition size is between 2001 and 4000 it returns 8

It's discrete. Making the function a continuous function would eliminate the cliffs. I think now, it simply returns composition size / 500.

(I originally wrote pow(2, composition size /500), but I realized power is not necessary.)

Comment 23

[Hiroyuki Ikezoe (:hiro)]

With the dividing y 500 approach, tp5_scroll tests significantly regress. The reason is that displayport size will be increased.

In the case of tp5o_scroll tests, the composition size is: (960 x 631.5).

The original displayport size is: (960 x 1792).
With dividing 500, it's: (960 x 2099.97).
With dividing 250, it's: (960 x 1615.37).

I'd go with the 250 approach.

Comment 24

[Phabricator Automation]

Comment on attachment 9478451 [details]
Bug 1729811 - Make GetDisplayportAlignmentMultiplier function continuous rather than descrete. r?tnikkel

Revision D245181 was moved to bug 172981. Setting attachment 9478451 [details] to obsolete.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1729811 - Allow 1px * devscale difference in the displayport comparison. r?botond

document.documentElement.clientHeight is also rounded, and it's
multiplied devscroll to convert it into screen coords, thus we need to
allow 1px * devscale in the comparison.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9251acf24ae6
Allow 1px * devscale difference in the displayport comparison. r=botond

Comment 27

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/9251acf24ae6

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dcbba838865a
Make GetDisplayportAlignmentMultiplier function continuous rather than descrete. r=tnikkel

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f2fd0c018c9
Revert "Bug 1729811 - Make GetDisplayportAlignmentMultiplier function continuous rather than descrete. r=tnikkel" for causing gtest failures @ APZCPanningTester

Comment 30

[amarc]

Backed out for causing gtest failures @ APZCPanningTester

• Backout link
• Push with failures
• Failure Log

Comment 31

[Hiroyuki Ikezoe (:hiro)]

These gtests use empty composition size for some reasons, thus the new display calculation is broken with the empty composition size. I did change the code to limit the minimum number to 1.0.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7ffcfacad0f6
https://hg.mozilla.org/integration/autoland/rev/205de2e9a789
Make GetDisplayportAlignmentMultiplier function continuous rather than descrete. r=tnikkel

Comment 33

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/205de2e9a789

Comment 34

[Mayank Bansal]

Seems to improve all SP3-TODOMVC-* tests.
Example: 10% on sp3-todomvc-angular
Are these improvements expected?

Comment 35

[Hiroyuki Ikezoe (:hiro)]

yes.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Note that on certain sizes of scroll container, such as 500px height just like bug 1969584, there should be performance regressions.

Comment 37

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #32)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7ffcfacad0f6
https://hg.mozilla.org/integration/autoland/rev/205de2e9a789
Make GetDisplayportAlignmentMultiplier function continuous rather than
descrete. r=tnikkel

Perfherder has detected a browsertime performance change from push 205de2e9a78957673f7c2f78f74ef22cc4eafbab.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
18%	speedometer3 TodoMVC-JavaScript-ES6-Webpack-Complex-DOM/DeletingAllItems/Async	linux1804-64-shippable-qr	fission webrender	2.77 -> 2.29	Before/After
14%	speedometer3 TodoMVC-React-Complex-DOM/DeletingAllItems/Async	linux1804-64-shippable-qr	fission webrender	2.92 -> 2.51	Before/After
14%	speedometer3 TodoMVC-React-Complex-DOM/DeletingAllItems/Async	linux1804-64-nightlyasrelease-qr	fission webrender	3.03 -> 2.60	Before/After
14%	speedometer3 TodoMVC-JavaScript-ES6-Webpack-Complex-DOM/DeletingAllItems/Async	macosx1470-64-shippable	fission webrender	2.68 -> 2.31	Before/After
13%	speedometer3 TodoMVC-React-Complex-DOM/DeletingAllItems/Async	macosx1470-64-shippable	fission webrender	2.77 -> 2.40	Before/After
...	...	...	...	...	...
2%	speedometer3 TodoMVC-Lit-Complex-DOM/total	macosx1500-aarch64-shippable	fission webrender	11.65 -> 11.41	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 45361

The following documentation link provides more information about this command.

Comment 38

[Alex Finder]

(In reply to Pulsebot from comment #32)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7ffcfacad0f6
https://hg.mozilla.org/integration/autoland/rev/205de2e9a789
Make GetDisplayportAlignmentMultiplier function continuous rather than
descrete. r=tnikkel

Perfherder has detected a talos performance change from push 205de2e9a78957673f7c2f78f74ef22cc4eafbab.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
10%	tp5o_scroll_paint_skip	macosx1470-64-shippable	e10s fission stylo webrender-sw	0.40 -> 0.36
6%	tp5o_scroll	macosx1470-64-shippable	e10s fission stylo webrender	1.29 -> 1.21
6%	tp5o_scroll	macosx1470-64-shippable	e10s fission stylo webrender-sw	1.18 -> 1.11
6%	tp5o_scroll_paint_skip	macosx1470-64-shippable	e10s fission stylo webrender	0.44 -> 0.42
3%	tp5o_scroll	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	1.49 -> 1.44

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 45412

The following documentation link provides more information about this command.