1102631 - Create a software vsync timer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect)

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #1098701 +++

Create a fallback software vsync timer when hardware vsync isn't available. This is also useful fortests.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Create a software vsync timer

Creates a new base::thread that notifies vsync at 60 HZ.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Create a fallback software vsync timer to mimic the functionality we have with hardware vsync timers. This will be useful mostly for testing.

The goal is to recreate a hardware vsync thread with an android::thread and to schedule vsyncs like the Compositor currently schedules Composites. I used an android::thread instead of an nsIThread because it was easy to post tasks/cancel them. We post a delayed task on a software vsync thread every 16 ms which mimics the hardware vsync notification.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8544305 [details] [diff] [review]
Create a software vsync timer

Review of attachment 8544305 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.h
@@ +25,5 @@
>        Display();
>        virtual ~Display();
>        void AddCompositorVsyncDispatcher(mozilla::CompositorVsyncDispatcher* aCompositorVsyncDispatcher);
>        void RemoveCompositorVsyncDispatcher(mozilla::CompositorVsyncDispatcher* aCompositorVsyncDispatcher);
>        // Notified when this display's vsync occurs, on the hardware vsync thread

s/hardware vsync thread/vsync thread/

::: gfx/thebes/gfxPlatform.cpp
@@ +2280,5 @@
> +
> +// Fallback option to use a software timer to mimic vsync. Useful for gtests
> +// To mimic a hardware vsync thread, we create a dedicated software timer
> +// vsync thread.
> +class SoftwareVsyncTimer : public mozilla::gfx::VsyncSource

Please move this class into a new file, SoftwareVsyncSource.h and rename the class also to SoftwareVsyncSource

@@ +2304,5 @@
> +    friend class SoftwareVsyncTimer;
> +
> +  public:
> +    // An ugly hack around Runnables requiring reference counting.
> +    // Since the displays live as long as the whole process, we don't have

What's wrong with making this class actually refcounted? Instead of mGlobalDisplay being a member variable, make it a nsRefPtr<SoftwareDisplay> that gets initialized to a new SoftwareDisplay in the SoftwareVsyncTimer constructor and nulled out in the destructor. It's much better than this hack.

Alternatively, move the timer stuff into the outer class (SoftwareVsyncTimer) which is already refcounted, and just have a dummy SoftwareDisplay which delegates stuff to the SoftwareVsyncTimer.

@@ +2366,5 @@
> +    {
> +      MOZ_ASSERT(IsInSoftwareVsyncThread());
> +      TimeStamp nextVsync = aVsyncTimestamp + mVsyncRate;
> +      TimeDuration delay = nextVsync - TimeStamp::Now();
> +      MOZ_ASSERT(delay.ToMilliseconds() > 0);

I think you still need to handle the case where delay is negative, because you can't guarantee that the runnable will run in time.

@@ +2368,5 @@
> +      TimeStamp nextVsync = aVsyncTimestamp + mVsyncRate;
> +      TimeDuration delay = nextVsync - TimeStamp::Now();
> +      MOZ_ASSERT(delay.ToMilliseconds() > 0);
> +
> +      mCurrentVsyncTask = NewRunnableMethod(this,

mCurrentVsyncTask is accessed on both the main thread (in DisableVsync) and the SoftwareVsyncThread (here), needs locking or something.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Updated with feedback from comment 5.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8544719 [details] [diff] [review]
Create a software vsync timer

Review of attachment 8544719 [details] [diff] [review]:
-----------------------------------------------------------------

Much better, but I want the shutdown sequence to be sorted out before r+'ing.

::: gfx/thebes/SoftwareVsyncSource.cpp
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +#include "nsThreadUtils.h"
> +#include "SoftwareVsyncSource.h"

include SoftwareVsyncSource.h first, then the rest in alphabetical order.

@@ +8,5 @@
> +#include "SoftwareVsyncSource.h"
> +#include "base/thread.h"
> +
> +using namespace mozilla;
> +using namespace mozilla::gfx;

In general I don't like broad namespace things like this in files that get unified-built because it can cause compiler errors when things get unified differently. It might be better to move this class into the mozilla::gfx namespace, or use specific items from the namespace that you need, or just qualify the use sites.

@@ +63,5 @@
> +
> +bool
> +SoftwareDisplay::IsVsyncEnabled()
> +{
> +  return mVsyncEnabled;

Will this ever be called off the main thread? If so might be worth wrapping mVsyncEnabled in the mCurrentTaskMonitor as well.

@@ +103,5 @@
> +
> +SoftwareDisplay::~SoftwareDisplay()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  delete mVsyncThread;

Seems to me like this won't actually shut down properly. Consider the case where the vsync thread has just started executing NotifyVsync. At this point the SoftwareVsyncSource is destroyed, which calls DisableVsync(). That will call Cancel() on mCurrentVsyncTask (which has no effect since the task is already in progress). The SoftwareDisplay is ref-counted so it won't get deleted from the SoftwareVsyncSource destructor; the running task has a ref to it and must run to completion. As part of running to completion, the code in ScheduleNextVsync will run, which will create another ref to the SoftwareDisplay object as part of the NewRunnableMethod. So there will always be a refptr to the SoftwareDisplay and it will never get destroyed.

I think somewhere in ScheduleNextVsync you need to actually check mVsyncEnabled and not schedule more tasks if it's false.

::: gfx/thebes/SoftwareVsyncSource.h
@@ +20,5 @@
> +class CancelableTask;
> +
> +class SoftwareDisplay MOZ_FINAL : public mozilla::gfx::VsyncSource::Display
> +{
> +  friend class SoftwareVsyncSource;

Why is this a friend?

@@ +27,5 @@
> +  public:
> +  // If we used the real Release, we would get a Release() AFTER the VsyncSource
> +  // cleaned up this display, which would be a double free.
> +  //NS_IMETHOD_(MozExternalRefCountType) AddRef(void) { return 1; }
> +  //NS_IMETHOD_(MozExternalRefCountType) Release(void) { return 1; }

Remove this stuff

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Updated with feedback from comment 7.

> Seems to me like this won't actually shut down properly.

Ahh yeah. That's the nice thing about having it not be a pointer, still overall cleaner though! Thanks!

Comment 9

[Mason Chang [Inactive] [:mchang]]

Just found out while writing some tests, since we have a reference while we post a task to the message loop, it is often the last reference on the software vsync thread. Since this is the last reference, the destructor runs on the software vsync thread which causes an assert to fail in here - http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/thread.cc?from=base/thread.cc&case=true#110

I think the proper solution is to call base::Thread::Stop() and base::Thread::Start() when we enable/disable vsync. http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/thread.h?from=base/thread.h#67

This also properly mimics OS X and b2g, will fixup.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Updated to start / stop the thread when we Enable/Disable vsync. The call to base::thread::Stop blocks until the thread exits.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8544883 [details] [diff] [review]
Create a software vsync timer

Review of attachment 8544883 [details] [diff] [review]:
-----------------------------------------------------------------

There's a deadlock in this version. See if you can figure it out. Should be an easy fix and apart from that it looks good.

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Fixed the deadlock case where on the main thread, we call DisableVsync() while on the software vsync thread, it could be in ScheduleNextVsync(). Since the lock in DisableVsync was for the whole method, main thread would wait for the thread to stop while the thread would wait for the lock. I scoped the lock in DisableVsync to just set mVsyncEnabled and to cancel the current task.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8545373 [details] [diff] [review]
Create a software vsync timer

Review of attachment 8545373 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

::: gfx/thebes/SoftwareVsyncSource.cpp
@@ +51,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mVsyncThread->IsRunning());
> +  {
> +    mozilla::MonitorAutoLock lock(mCurrentTaskMonitor);

Add a comment on the open brace "// scope lock"

Comment 14

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Carrying r+, updated with feedback from comment 13.

Comment 15

[Mason Chang [Inactive] [:mchang]]

Attachment: Create a software vsync timer

Uploaded wrong patch, this is the correct one.

Comment 16

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26c24b7c06ae

Successful try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=78d627394d91

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26c24b7c06ae