1765399 - Improve how VsyncDispatchers swap out their VsyncSource

Status: RESOLVED FIXED

(Core :: Graphics, task)

Description

[Markus Stange [:mstange]]

At the moment, if you change the layout.frame_rate pref, we create a hardware or software VsyncSource, and then tell the old VsyncSource's RefreshDriverVsyncDispatcher to swap out its vsync source. The way this works is currently rather brittle, and the switching doesn't happen everywhere it should.

For bug 1759232 I want to uses a similar mechanism when a window moves across screens: On macOS there will eventually be one VsyncDispatcher per window and one VsyncSource per screen. So the VsyncDispatcher would switch out its VsyncSource when the window moves to a different screen. (At the moment there is one global VsyncDispatcher and one global VsyncSource on macOS.)

As part of this refactoring, I'm going to rename RefreshDriverVsyncDispatcher to VsyncDispatcher and remove many direct uses of VsyncSource and replace those uses with uses of VsyncDispatcher.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Rename RefreshTimerVsyncDispatcher to VsyncDispatcher. r=smaug

RefreshTimerVsyncDispatcher manages a lot more than just the
RefreshDriverVsyncTimer these days.

Depends on D144362

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Rename mRefreshTimerNeedsVsync and NotifyRefreshTimerVsyncStatus. r=smaug

Depends on D144363

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Expand the comment for VsyncDispatcher. r=smaug

Depends on D144364

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Register CompositorVsyncDispatcher with VsyncDispatcher instead of directly with the VsyncSource. r=smaug

This makes it so that the VsyncSource doesn't need to keep track of the compositor vsync dispatchers.
And the moving-between-sources logic needs to be handled only for the VsyncDispatcher.

Once we have one VsyncDispatcher per window, we can probably eliminate CompositorVsyncDispatcher.

Depends on D144365

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Add RefreshDriverVsyncDispatcher::GetVsyncRate so that VsyncRefreshDriverTimer no longer needs a pointer to the VsyncSource. r=smaug

Depends on D144366

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Add gfxPlatform::GetGlobalVsyncDispatcher(). r=smaug

Now gfxPlatform and gfxPlatform::mVsyncSource have shared ownership of the VsyncDispatcher.
This is in preparation for reversing the ownership relationship between VsyncSource and VsyncDispatcher.

Depends on D144367

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Use GetGlobalVsyncDispatcher() in RefreshDriverVsyncTimer. r=smaug

Depends on D144368

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Make VsyncParent use VsyncDispatcher instead of VsyncSource. r=smaug

This doesn't change much; VsyncParent was already only using mVsyncSource
to obtain the VsyncDispatcher. (And to get the vsync rate, which it can
now get directly from the dispatcher.)

Depends on D144369

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Move main thread observers to the vsync dispatcher. r=smaug

Main thread observers (previously "generic" observers) are only used
by Windows touchpad scrolling so far.

Depends on D144370

Comment 10

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Move a few more things to GetGlobalVsyncDispatcher. r=smaug

Depends on D144371

Comment 11

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Rename Get/CreateHardwareVsync to GetGlobalVsync, CreateGlobalHardwareVsync and CreateSoftwareVsync. r=smaug

This draws a clearer line between hardware vsync and software vsync.

Depends on D144372

Comment 12

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Replace nsIWidget::GetVsyncSource with nsIWidget::GetVsyncDispatcher. r=smaug

All users of nsIWidget::GetVsyncSource just want the dispatcher anyway.
Getting the dispatcher directly from the widget will allow us to remove VsyncSource::GetVsyncDispatcher.

Depends on D144373

Comment 13

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Invert the relationship between VsyncSource and VsyncDispatcher: The VsyncDispatcher now owns the source. r=smaug

This makes vsync source swapping much more natural.

Furthermore, nothing uses gfxPlatform::GetGlobalVsync anymore, so it is removed.

Depends on D144374

Comment 14

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Move SoftwareVsyncSource into the mozilla::gfx namespace. r=smaug

Depends on D144375

Comment 15

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Add SoftwareVsyncSource::SetVsyncRate and make XrandrSoftwareVsyncSource vsync rate adjustment threadsafe. r=smaug

randrSoftwareVsyncSource changes the rate on the main thread but reads it on the software vsync thread.

Depends on D144376

Comment 16

[Markus Stange [:mstange]]

Attachment: Bug 1765399 - Don't create a new SoftwareVsyncSource instance when layout.frame_rate is changed to a different value. r=smaug

Depends on D144377

Comment 17

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=6a764ebe41161b9f5916884fbe8377c5994284b6

Comment 18

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a8f5640d7a5946651e6b3ded2138e689c0dfd17

Comment 19

[Markus Stange [:mstange]]

There are two issues currently preventing this from landing:

1. A deadlock with the Wayland vsync source where calling EnableVsync synchronously calls NotifyVsync, so the VsyncDispatcher re-enters its own lock on the main thread. Should be easily fixable by moving the call to EnableVsync outside of the lock.
2. A test failure on Linux where SoftwareVsyncSource destruction hits an assert about the software vsync thread not running. I haven't been able to reproduce this so far. It is possible that some thread machinery has already been torn down by the time that gfxPlatform shutdown destroys the software vsync source, but I'm not sure why we weren't hitting the assert without these patches. I'm still looking into this one.

Comment 20

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #19)

1. A deadlock with the Wayland vsync source where calling EnableVsync synchronously calls NotifyVsync, so the VsyncDispatcher re-enters its own lock on the main thread. Should be easily fixable by moving the call to EnableVsync outside of the lock.

I've made that fix and merged it into the patch "Invert the relationship between VsyncSource and VsyncDispatcher".

2. A test failure on Linux where SoftwareVsyncSource destruction hits an assert about the software vsync thread not running. I haven't been able to reproduce this so far. It is possible that some thread machinery has already been torn down by the time that gfxPlatform shutdown destroys the software vsync source, but I'm not sure why we weren't hitting the assert without these patches. I'm still looking into this one.

This turned out to be really simple: mGlobalHardwareVsyncSource and mSoftwareVsyncSource were set to the same SoftwareVsyncSource, so we were calling Shutdown on the same object twice, which it didn't like. I'm now skipping the second call if they're the same object.

https://treeherder.mozilla.org/jobs?repo=try&revision=413911e41da0bffa2b65f0a874ffd98bd74f5bec

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9115d9d648e
Rename RefreshTimerVsyncDispatcher to VsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/4a30a4b3d30d
Rename mRefreshTimerNeedsVsync and NotifyRefreshTimerVsyncStatus. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3a3a2aa6de9b
Expand the comment for VsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/40c47c498858
Register CompositorVsyncDispatcher with VsyncDispatcher instead of directly with the VsyncSource. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2f3ceca7aefe
Add RefreshDriverVsyncDispatcher::GetVsyncRate so that VsyncRefreshDriverTimer no longer needs a pointer to the VsyncSource. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3890e83660b0
Add gfxPlatform::GetGlobalVsyncDispatcher(). r=smaug
https://hg.mozilla.org/integration/autoland/rev/ce326de1e107
Use GetGlobalVsyncDispatcher() in RefreshDriverVsyncTimer. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5e5a29a99c9e
Make VsyncParent use VsyncDispatcher instead of VsyncSource. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d21fca656853
Move main thread observers to the vsync dispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d3c1f6c420e3
Move a few more things to GetGlobalVsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d5a4004a2955
Rename Get/CreateHardwareVsync to GetGlobalVsync, CreateGlobalHardwareVsync and CreateSoftwareVsync. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e5001537e536
Replace nsIWidget::GetVsyncSource with nsIWidget::GetVsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ee00e3583f42
Invert the relationship between VsyncSource and VsyncDispatcher: The VsyncDispatcher now owns the source. r=smaug
https://hg.mozilla.org/integration/autoland/rev/939b577eee05
Move SoftwareVsyncSource into the mozilla::gfx namespace. r=smaug
https://hg.mozilla.org/integration/autoland/rev/bd164f5cc8b3
Add SoftwareVsyncSource::SetVsyncRate and make XrandrSoftwareVsyncSource vsync rate adjustment threadsafe. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8ff5e213e351
Don't create a new SoftwareVsyncSource instance when layout.frame_rate is changed to a different value. r=smaug

Comment 22

[Marian-Vasile Laza]

Backed out 16 changesets (Bug 1765399) for causing build bustages on RefPtr.h.
Backout link
Push with failures
Failure Log

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41848c615ad4
Rename RefreshTimerVsyncDispatcher to VsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d022091c31b6
Rename mRefreshTimerNeedsVsync and NotifyRefreshTimerVsyncStatus. r=smaug
https://hg.mozilla.org/integration/autoland/rev/821753cf91d4
Expand the comment for VsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c7fba6ee6573
Register CompositorVsyncDispatcher with VsyncDispatcher instead of directly with the VsyncSource. r=smaug
https://hg.mozilla.org/integration/autoland/rev/255887544627
Add RefreshDriverVsyncDispatcher::GetVsyncRate so that VsyncRefreshDriverTimer no longer needs a pointer to the VsyncSource. r=smaug
https://hg.mozilla.org/integration/autoland/rev/91854a447121
Add gfxPlatform::GetGlobalVsyncDispatcher(). r=smaug
https://hg.mozilla.org/integration/autoland/rev/24fe85e709ba
Use GetGlobalVsyncDispatcher() in RefreshDriverVsyncTimer. r=smaug
https://hg.mozilla.org/integration/autoland/rev/22b568631834
Make VsyncParent use VsyncDispatcher instead of VsyncSource. r=smaug
https://hg.mozilla.org/integration/autoland/rev/90c803e44a4a
Move main thread observers to the vsync dispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/4b936873cd7c
Move a few more things to GetGlobalVsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5dd523c09fed
Rename Get/CreateHardwareVsync to GetGlobalVsync, CreateGlobalHardwareVsync and CreateSoftwareVsync. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8ff003cb3864
Replace nsIWidget::GetVsyncSource with nsIWidget::GetVsyncDispatcher. r=smaug
https://hg.mozilla.org/integration/autoland/rev/9689f7bce600
Invert the relationship between VsyncSource and VsyncDispatcher: The VsyncDispatcher now owns the source. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a61caa52f560
Move SoftwareVsyncSource into the mozilla::gfx namespace. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1311628d0756
Add SoftwareVsyncSource::SetVsyncRate and make XrandrSoftwareVsyncSource vsync rate adjustment threadsafe. r=smaug
https://hg.mozilla.org/integration/autoland/rev/805110b54051
Don't create a new SoftwareVsyncSource instance when layout.frame_rate is changed to a different value. r=smaug

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/41848c615ad4
https://hg.mozilla.org/mozilla-central/rev/d022091c31b6
https://hg.mozilla.org/mozilla-central/rev/821753cf91d4
https://hg.mozilla.org/mozilla-central/rev/c7fba6ee6573
https://hg.mozilla.org/mozilla-central/rev/255887544627
https://hg.mozilla.org/mozilla-central/rev/91854a447121
https://hg.mozilla.org/mozilla-central/rev/24fe85e709ba
https://hg.mozilla.org/mozilla-central/rev/22b568631834
https://hg.mozilla.org/mozilla-central/rev/90c803e44a4a
https://hg.mozilla.org/mozilla-central/rev/4b936873cd7c
https://hg.mozilla.org/mozilla-central/rev/5dd523c09fed
https://hg.mozilla.org/mozilla-central/rev/8ff003cb3864
https://hg.mozilla.org/mozilla-central/rev/9689f7bce600
https://hg.mozilla.org/mozilla-central/rev/a61caa52f560
https://hg.mozilla.org/mozilla-central/rev/1311628d0756
https://hg.mozilla.org/mozilla-central/rev/805110b54051