Skip to content

Commit

Permalink
Bug 1765399 - Invert the relationship between VsyncSource and VsyncDi…
Browse files Browse the repository at this point in the history 
…spatcher: The VsyncDispatcher now owns the source. r=smaug

This makes vsync source swapping much more natural.

The VsyncSource now only has a reference to the VsyncDispatcher for the duration
during which the dispatcher is listening to vsync. Whenever the dispatcher is
not listening to vsync, the source has no reference to the dispatcher and there
is no cycle.

This patch also adds the ability to register multiple dispatchers with the same
source. This ability is not used yet; a vsync source always has zero or one
dispatchers at the moment. It is in preparation for a future patch where there
will be one dispatcher per widget.

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

Differential Revision: https://phabricator.services.mozilla.com/D144375
  • Loading branch information
@mstange
mstange committed May 5, 2022
1 parent b6438a1 commit 3c3cfe8
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 114 deletions.
19 changes: 10 additions & 9 deletions gfx/tests/gtest/TestVsync.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,14 @@ class VsyncTester : public ::testing::Test {
protected:
explicit VsyncTester() {
gfxPlatform::GetPlatform();
mVsyncSource = gfxPlatform::GetPlatform()->GetGlobalVsync();
mVsyncDispatcher = gfxPlatform::GetPlatform()->GetGlobalVsyncDispatcher();
mVsyncSource = mVsyncDispatcher->GetCurrentVsyncSource();
MOZ_RELEASE_ASSERT(mVsyncSource, "GFX: Vsync source not found.");
}

virtual ~VsyncTester() { mVsyncSource = nullptr; }

RefPtr<VsyncDispatcher> mVsyncDispatcher;
RefPtr<VsyncSource> mVsyncSource;
};

Expand Down Expand Up @@ -116,7 +118,7 @@ TEST_F(VsyncTester, CompositorGetVsyncNotifications) {
ASSERT_FALSE(mVsyncSource->IsVsyncEnabled());

RefPtr<CompositorVsyncDispatcher> vsyncDispatcher =
new CompositorVsyncDispatcher(mVsyncSource->GetVsyncDispatcher());
new CompositorVsyncDispatcher(mVsyncDispatcher);
RefPtr<TestVsyncObserver> testVsyncObserver = new TestVsyncObserver();

vsyncDispatcher->SetCompositorVsyncObserver(testVsyncObserver);
Expand All @@ -126,10 +128,11 @@ TEST_F(VsyncTester, CompositorGetVsyncNotifications) {
testVsyncObserver->WaitForVsyncNotification();
ASSERT_TRUE(testVsyncObserver->DidGetVsyncNotification());

vsyncDispatcher->SetCompositorVsyncObserver(nullptr);
FlushMainThreadLoop();

vsyncDispatcher = nullptr;
testVsyncObserver = nullptr;

mVsyncSource->DisableVsync();
ASSERT_FALSE(mVsyncSource->IsVsyncEnabled());
}

Expand All @@ -138,22 +141,20 @@ TEST_F(VsyncTester, ChildRefreshDriverGetVsyncNotifications) {
mVsyncSource->DisableVsync();
ASSERT_FALSE(mVsyncSource->IsVsyncEnabled());

RefPtr<VsyncDispatcher> vsyncDispatcher = mVsyncSource->GetVsyncDispatcher();
ASSERT_TRUE(vsyncDispatcher != nullptr);
ASSERT_TRUE(mVsyncDispatcher != nullptr);

RefPtr<TestVsyncObserver> testVsyncObserver = new TestVsyncObserver();
vsyncDispatcher->AddVsyncObserver(testVsyncObserver);
mVsyncDispatcher->AddVsyncObserver(testVsyncObserver);
ASSERT_TRUE(mVsyncSource->IsVsyncEnabled());

testVsyncObserver->WaitForVsyncNotification();
ASSERT_TRUE(testVsyncObserver->DidGetVsyncNotification());

vsyncDispatcher->RemoveVsyncObserver(testVsyncObserver);
mVsyncDispatcher->RemoveVsyncObserver(testVsyncObserver);
testVsyncObserver->ResetVsyncNotification();
testVsyncObserver->WaitForVsyncNotification();
ASSERT_FALSE(testVsyncObserver->DidGetVsyncNotification());

vsyncDispatcher = nullptr;
testVsyncObserver = nullptr;

mVsyncSource->DisableVsync();
Expand Down
95 changes: 50 additions & 45 deletions gfx/thebes/VsyncSource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,60 +17,68 @@
namespace mozilla {
namespace gfx {

VsyncSource::VsyncSource()
: mDispatcherLock("display dispatcher lock"),
mVsyncDispatcherNeedsVsync(false) {
VsyncSource::VsyncSource() : mState("VsyncSource::State") {
MOZ_ASSERT(NS_IsMainThread());
mVsyncDispatcher = new VsyncDispatcher(this);
}

VsyncSource::~VsyncSource() {
MOZ_ASSERT(NS_IsMainThread());
MutexAutoLock lock(mDispatcherLock);
mVsyncDispatcher = nullptr;
}
VsyncSource::~VsyncSource() { MOZ_ASSERT(NS_IsMainThread()); }

// Called on the vsync thread
void VsyncSource::NotifyVsync(const TimeStamp& aVsyncTimestamp,
const TimeStamp& aOutputTimestamp) {
// Called on the vsync thread
MutexAutoLock lock(mDispatcherLock);

// mVsyncDispatcher might be null here if MoveListenersToNewSource
// was called concurrently with this function and won the race to acquire
// mDispatcherLock. In this case the new VsyncSource that is replacing this
// one will handle notifications from now on, so we can abort.
if (!mVsyncDispatcher) {
return;
VsyncId vsyncId;
nsTArray<RefPtr<VsyncDispatcher>> dispatchers;

{
auto state = mState.Lock();
vsyncId = state->mVsyncId.Next();
dispatchers = state->mDispatchers.Clone();
state->mVsyncId = vsyncId;
}

mVsyncId = mVsyncId.Next();
const VsyncEvent event(mVsyncId, aVsyncTimestamp, aOutputTimestamp);
mVsyncDispatcher->NotifyVsync(event);
// Notify our listeners, outside of the lock.
const VsyncEvent event(vsyncId, aVsyncTimestamp, aOutputTimestamp);
for (const auto& dispatcher : dispatchers) {
dispatcher->NotifyVsync(event);
}
}

TimeDuration VsyncSource::GetVsyncRate() {
// If hardware queries fail / are unsupported, we have to just guess.
return TimeDuration::FromMilliseconds(1000.0 / 60.0);
void VsyncSource::AddVsyncDispatcher(VsyncDispatcher* aVsyncDispatcher) {
MOZ_ASSERT(aVsyncDispatcher);
{
auto state = mState.Lock();
if (!state->mDispatchers.Contains(aVsyncDispatcher)) {
state->mDispatchers.AppendElement(aVsyncDispatcher);
}
}

UpdateVsyncStatus();
}

void VsyncSource::MoveListenersToNewSource(
const RefPtr<VsyncSource>& aNewSource) {
MOZ_ASSERT(NS_IsMainThread());
MutexAutoLock lock(mDispatcherLock);
MutexAutoLock newLock(aNewSource->mDispatcherLock);
void VsyncSource::RemoveVsyncDispatcher(VsyncDispatcher* aVsyncDispatcher) {
MOZ_ASSERT(aVsyncDispatcher);
{
auto state = mState.Lock();
state->mDispatchers.RemoveElement(aVsyncDispatcher);
}

aNewSource->mVsyncDispatcher = mVsyncDispatcher;
mVsyncDispatcher->MoveToSource(aNewSource);
mVsyncDispatcher = nullptr;
UpdateVsyncStatus();
}

void VsyncSource::NotifyVsyncDispatcherVsyncStatus(bool aEnable) {
MOZ_ASSERT(NS_IsMainThread());
mVsyncDispatcherNeedsVsync = aEnable;
UpdateVsyncStatus();
// This is the base class implementation. Subclasses override this method.
TimeDuration VsyncSource::GetVsyncRate() {
// If hardware queries fail / are unsupported, we have to just guess.
return TimeDuration::FromMilliseconds(1000.0 / 60.0);
}

void VsyncSource::UpdateVsyncStatus() {
if (!NS_IsMainThread()) {
NS_DispatchToMainThread(NS_NewRunnableFunction(
"VsyncSource::UpdateVsyncStatus",
[self = RefPtr{this}] { self->UpdateVsyncStatus(); }));
return;
}

MOZ_ASSERT(NS_IsMainThread());
// WARNING: This function SHOULD NOT BE CALLED WHILE HOLDING LOCKS
// NotifyVsync grabs a lock to dispatch vsync events
Expand All @@ -79,8 +87,8 @@ void VsyncSource::UpdateVsyncStatus() {
// stop while the vsync thread is in NotifyVsync.
bool enableVsync = false;
{ // scope lock
MutexAutoLock lock(mDispatcherLock);
enableVsync = mVsyncDispatcherNeedsVsync;
auto state = mState.Lock();
enableVsync = !state->mDispatchers.IsEmpty();
}

if (enableVsync) {
Expand All @@ -94,20 +102,17 @@ void VsyncSource::UpdateVsyncStatus() {
}
}

RefPtr<VsyncDispatcher> VsyncSource::GetVsyncDispatcher() {
return mVsyncDispatcher;
}

// static
Maybe<TimeDuration> VsyncSource::GetFastestVsyncRate() {
Maybe<TimeDuration> retVal;
if (!gfxPlatform::Initialized()) {
return retVal;
}

mozilla::gfx::VsyncSource* vsyncSource =
gfxPlatform::GetPlatform()->GetGlobalVsync();
if (vsyncSource && vsyncSource->IsVsyncEnabled()) {
RefPtr<VsyncDispatcher> vsyncDispatcher =
gfxPlatform::GetPlatform()->GetGlobalVsyncDispatcher();
RefPtr<VsyncSource> vsyncSource = vsyncDispatcher->GetCurrentVsyncSource();
if (vsyncSource->IsVsyncEnabled()) {
retVal.emplace(vsyncSource->GetVsyncRate());
}

Expand Down
27 changes: 18 additions & 9 deletions gfx/thebes/VsyncSource.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#define GFX_VSYNCSOURCE_H

#include "nsTArray.h"
#include "mozilla/DataMutex.h"
#include "mozilla/RefPtr.h"
#include "mozilla/Maybe.h"
#include "mozilla/Mutex.h"
Expand Down Expand Up @@ -52,7 +53,10 @@ class VsyncSource {
virtual void NotifyVsync(const TimeStamp& aVsyncTimestamp,
const TimeStamp& aOutputTimestamp);

void NotifyVsyncDispatcherVsyncStatus(bool aEnable);
// Can be called on any thread
void AddVsyncDispatcher(VsyncDispatcher* aDispatcher);
void RemoveVsyncDispatcher(VsyncDispatcher* aDispatcher);

virtual TimeDuration GetVsyncRate();

// These should all only be called on the main thread
Expand All @@ -61,23 +65,28 @@ class VsyncSource {
virtual bool IsVsyncEnabled() = 0;
virtual void Shutdown() = 0;

void MoveListenersToNewSource(const RefPtr<VsyncSource>& aNewSource);

RefPtr<VsyncDispatcher> GetVsyncDispatcher();

// Returns the rate of the fastest enabled VsyncSource or Nothing().
static Maybe<TimeDuration> GetFastestVsyncRate();

protected:
virtual ~VsyncSource();

private:
// Can be called on any thread
void UpdateVsyncStatus();

Mutex mDispatcherLock MOZ_UNANNOTATED;
bool mVsyncDispatcherNeedsVsync;
RefPtr<VsyncDispatcher> mVsyncDispatcher;
VsyncId mVsyncId;
struct State {
// The set of VsyncDispatchers which are registered with this source.
// At the moment, the length of this array is always zero or one.
// The ability to register multiple dispatchers is not used yet; it is
// intended for when we have one dispatcher per widget.
nsTArray<RefPtr<VsyncDispatcher>> mDispatchers;

// The vsync ID which we used for the last vsync event.
VsyncId mVsyncId;
};

DataMutex<State> mState;
};

} // namespace gfx
Expand Down
18 changes: 8 additions & 10 deletions gfx/thebes/gfxPlatform.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3040,21 +3040,19 @@ void gfxPlatform::ReInitFrameRate() {
} else {
gPlatform->mVsyncSource = gPlatform->CreateGlobalHardwareVsyncSource();
}
// Tidy up old vsync source.
if (oldSource) {
oldSource->MoveListenersToNewSource(gPlatform->mVsyncSource);
oldSource->Shutdown();
}

if (gPlatform->mVsyncDispatcher) {
// Our global vsync dispatcher should always stay the same. It should just
// swap out its underlying source.
MOZ_RELEASE_ASSERT(gPlatform->mVsyncDispatcher ==
gPlatform->mVsyncSource->GetVsyncDispatcher());
// Swap out the dispatcher's underlying source.
gPlatform->mVsyncDispatcher->SetVsyncSource(gPlatform->mVsyncSource);
} else {
// Initial assignment of the vsync dispatcher.
gPlatform->mVsyncDispatcher =
gPlatform->mVsyncSource->GetVsyncDispatcher();
new VsyncDispatcher(gPlatform->mVsyncSource);
}

// Shut down the old vsync source.
if (oldSource) {
oldSource->Shutdown();
}
}
}
Expand Down
10 changes: 0 additions & 10 deletions gfx/thebes/gfxPlatform.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -653,16 +653,6 @@ class gfxPlatform : public mozilla::layers::MemoryPressureListener {

static bool UsesOffMainThreadCompositing();

/**
* Get the global vsync source for each platform.
* Should only exist and be valid on the parent process
*/
virtual mozilla::gfx::VsyncSource* GetGlobalVsync() {
MOZ_ASSERT(mVsyncSource != nullptr);
MOZ_ASSERT(XRE_IsParentProcess());
return mVsyncSource;
}

/**
* Returns the global vsync dispatcher. There is only one global vsync
* dispatcher and it stays around for the entire lifetime of the process.
Expand Down
57 changes: 42 additions & 15 deletions widget/VsyncDispatcher.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ void CompositorVsyncDispatcher::Shutdown() {
}

VsyncDispatcher::VsyncDispatcher(gfx::VsyncSource* aVsyncSource)
: mVsyncSource(aVsyncSource), mState("VsyncDispatcher::mState") {
: mState(State(aVsyncSource), "VsyncDispatcher::mState") {
MOZ_ASSERT(XRE_IsParentProcess());
MOZ_ASSERT(NS_IsMainThread());
}
Expand All @@ -99,9 +99,29 @@ VsyncDispatcher::~VsyncDispatcher() {
MOZ_ASSERT(NS_IsMainThread());
}

void VsyncDispatcher::MoveToSource(gfx::VsyncSource* aVsyncSource) {
MOZ_ASSERT(NS_IsMainThread());
mVsyncSource = aVsyncSource;
void VsyncDispatcher::SetVsyncSource(gfx::VsyncSource* aVsyncSource) {
MOZ_RELEASE_ASSERT(aVsyncSource);

auto state = mState.Lock();
if (aVsyncSource == state->mCurrentVsyncSource) {
return;
}

if (state->mIsObservingVsync) {
state->mCurrentVsyncSource->RemoveVsyncDispatcher(this);
aVsyncSource->AddVsyncDispatcher(this);
}
state->mCurrentVsyncSource = aVsyncSource;
}

RefPtr<gfx::VsyncSource> VsyncDispatcher::GetCurrentVsyncSource() {
auto state = mState.Lock();
return state->mCurrentVsyncSource;
}

TimeDuration VsyncDispatcher::GetVsyncRate() {
auto state = mState.Lock();
return state->mCurrentVsyncSource->GetVsyncRate();
}

void VsyncDispatcher::NotifyVsync(const VsyncEvent& aVsync) {
Expand Down Expand Up @@ -195,19 +215,26 @@ void VsyncDispatcher::RemoveMainThreadObserver(VsyncObserver* aObserver) {
}

void VsyncDispatcher::UpdateVsyncStatus() {
if (!NS_IsMainThread()) {
NS_DispatchToMainThread(
NewRunnableMethod("VsyncDispatcher::UpdateVsyncStatus", this,
&VsyncDispatcher::UpdateVsyncStatus));
return;
}
bool wasObservingVsync = false;
bool needVsync = false;
RefPtr<VsyncSource> vsyncSource;

mVsyncSource->NotifyVsyncDispatcherVsyncStatus(NeedsVsync());
}
{
auto state = mState.Lock();
wasObservingVsync = state->mIsObservingVsync;
needVsync =
!state->mObservers.IsEmpty() || !state->mMainThreadObservers.IsEmpty();
state->mIsObservingVsync = needVsync;
vsyncSource = state->mCurrentVsyncSource;
}

bool VsyncDispatcher::NeedsVsync() {
auto state = mState.Lock();
return !state->mObservers.IsEmpty() || !state->mMainThreadObservers.IsEmpty();
// Call Add/RemoveVsyncDispatcher outside the lock, because it can re-enter
// into VsyncDispatcher::NotifyVsync.
if (needVsync && !wasObservingVsync) {
vsyncSource->AddVsyncDispatcher(this);
} else if (!needVsync && wasObservingVsync) {
vsyncSource->RemoveVsyncDispatcher(this);
}
}

} // namespace mozilla
Loading

0 comments on commit 3c3cfe8

Please sign in to comment.