1048667 - Implement Vsync dispatch framework

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect, P1)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Before we finish all stuff in the bug 987529, we can provide the vsync dispatch framework first. We will have a separated vsync dispatcher thread in chrome process, and have a 16.6ms period function to implement the vsync related stuff.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part1: silk preference. v1

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync event protocol. v1

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part3: vsync dispatcher framework. v1

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Par4: register hwc hw vsync event. v1

Comment 5

[Peter Chang[:pchang]]

Comment on attachment 8467568 [details] [diff] [review]
[WIP] Part2: vsync event protocol. v1

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

::: gfx/layers/ipc/PVsyncEvent.ipdl
@@ +21,5 @@
> +  NotifyVsyncEvent(VsyncData aVsyncData);
> +
> +parent:
> +  EnableVsyncEventNotification();
> +  DisableVsyncEventNotification();

simplify to SetVsyncEventNotification(bool enable)

::: gfx/layers/ipc/VsyncEventChild.cpp
@@ +8,5 @@
> +#include "nsXULAppAPI.h"
> +
> +//#define PRINT_VSYNC_DEBUG
> +#ifdef PRINT_VSYNC_DEBUG
> +#define VSYNC_DEBUG_MESSAGE printf_stderr("bignose tid:%d %s",gettid(),__PRETTY_FUNCTION__)

Please remove your name from debug log and change the debug log based on debug/error/warning.

@@ +22,5 @@
> +
> +PVsyncEventChild*
> +VsyncEventChild::Create(Transport* aTransport, ProcessId aOtherProcess)
> +{
> +  VSYNC_DEBUG_MESSAGE;

Do we need to log every functions?

@@ +26,5 @@
> +  VSYNC_DEBUG_MESSAGE;
> +
> +  MOZ_RELEASE_ASSERT(NS_IsMainThread());
> +  //MOZ_ASSERT(NS_IsMainThread());
> +

Please change to MOZ_ASSERT for all of your patches.

@@ +61,5 @@
> +
> +bool VsyncEventChild::RecvNotifyVsyncEvent(const VsyncData& aVsyncData)
> +{
> +  VSYNC_DEBUG_MESSAGE;
> +

Do you miss //TODO here?

::: gfx/layers/ipc/VsyncEventParent.cpp
@@ +9,5 @@
> +#include "nsXULAppAPI.h"
> +
> +//#define PRINT_VSYNC_DEBUG
> +#ifdef PRINT_VSYNC_DEBUG
> +#define VSYNC_DEBUG_MESSAGE printf_stderr("bignose tid:%d %s",gettid(),__PRETTY_FUNCTION__)

Please remove your name and refine the debug level

@@ +25,5 @@
> +ConnectVsyncEventParent(PVsyncEventParent* aVsyncEventParent,
> +                        Transport* aTransport,
> +                        ProcessHandle aOtherProcess)
> +{
> +  VSYNC_DEBUG_MESSAGE;

Refine the debug message.

@@ +35,5 @@
> +VsyncEventParent::StartUp()
> +{
> +  VSYNC_DEBUG_MESSAGE;
> +
> +  MOZ_RELEASE_ASSERT(NS_IsMainThread());

Please use MOZ_ASSERT

::: gfx/thebes/gfxPlatform.cpp
@@ +525,5 @@
>  #endif
> +
> +#if defined MOZ_WIDGET_GONK && ANDROID_VERSION >= 17
> +        if (gfxPrefs::SilkEnabled()) {
> +            mozilla::layers::VsyncEventParent::StartUp();

Why do we need to check Android version here?
You already checked the android version on GonkVsyncDispatcher::StartUpVsyncEvent().

Comment 6

[Peter Chang[:pchang]]

Comment on attachment 8467569 [details] [diff] [review]
[WIP] Part3: vsync dispatcher framework. v1

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

In my opinion, it is better to merge part2 and part3 because part3 depends on part2.
I didn't see any obvious reason to split into two parts.

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +15,5 @@
> +#include "nsXULAppAPI.h"
> +
> +//#define DEBUG_VSYNC
> +#ifdef DEBUG_VSYNC
> +#define VSYNC_PRINT(...) do { printf_stderr("bignose " __VA_ARGS__); } while (0)

refine debug log

@@ +29,5 @@
> +static MessageLoop* sVsyncDispatchMessageLoop = nullptr;
> +
> +static scoped_refptr<GonkVsyncDispatcher> sGonkVsyncDispatcher;
> +
> +class ArrayDataHelper

Please rename ArrayDataHelper because this name has no relation with CheckVsyncNotification()

@@ +30,5 @@
> +
> +static scoped_refptr<GonkVsyncDispatcher> sGonkVsyncDispatcher;
> +
> +class ArrayDataHelper
> +{

I have another idea about mVsyncEventParentList.
You can define a enum of vsync module list, like the following.
enum {
   InputDispatcher = 0,
   Compositor,
   Refreshdriver,
   MAX_NUM
}
You can declare a atomic array with MAX_NUM size and register/unregister specific module by setting atomic_array[i] as 1 or 0.
And then have another atomic counter to record how many modules registered.

With above changes, you don't need the PostMessage to update the module list.

@@ +61,5 @@
> +  }
> +};
> +
> +static bool
> +CreateThread()

Rename to CreateVsyncDispatchThread

@@ +85,5 @@
> +GonkVsyncDispatcher::GetInstance()
> +{
> +  return sGonkVsyncDispatcher;
> +}
> +

Can we separate these functions that called by chrome or content? Or it is common function.

Ex:
/**********/
func called by chrome
...
...
/*********/
func called by content
...
....

@@ +96,5 @@
> +  // only b2g need to create a new thread
> +  MOZ_RELEASE_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +  //MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +
> +  CreateThread();

check the return value of CreateThread()

@@ +177,5 @@
> +
> +void
> +GonkVsyncDispatcher::StartUpVsyncEvent()
> +{
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {

Change to MOZ_ASSERT

@@ +179,5 @@
> +GonkVsyncDispatcher::StartUpVsyncEvent()
> +{
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    if (!mInitVsyncEventGenerator){
> +      mInitVsyncEventGenerator = true;

s/mInitVsyncEventGenerator/mInitHWVsync

@@ +189,5 @@
> +        //TODO:
> +        //init hw vsync event here.
> +      }
> +#endif
> +      if (!mUseHWVsyncEventGenerator) {

s/mUseHWCsyncEventGenerator/mUseHWVsync

@@ +192,5 @@
> +#endif
> +      if (!mUseHWVsyncEventGenerator) {
> +        //TODO:
> +        //init software vsync event here.
> +      }

Please dump which vsync type is using in debug/release build.

@@ +200,5 @@
> +
> +void
> +GonkVsyncDispatcher::ShutDownVsyncEvent()
> +{
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {

Change to MOZ_ASSERT

@@ +225,5 @@
> +
> +void
> +GonkVsyncDispatcher::EnableVsyncDispatch(bool aEnable)
> +{
> +  mEnableVsyncDispatch = aEnable;

s/mEnableVsyncDispatch/mEnable  because this is inside GonkVysncDispatcher.

@@ +234,5 @@
> +{
> +   int count = 0;
> +
> +   count += mVsyncEventParentList.Length();
> +

return mVsyncEventParentList.Length();
Is this thread-safe?

@@ +241,5 @@
> +
> +void
> +GonkVsyncDispatcher::EnableVsyncEvent(bool aEnable)
> +{
> +  if (mInitVsyncEventGenerator) {

Add MOZ_ASSERT about mInitHWVsync

@@ +306,5 @@
> +  VSYNC_PRINT("DispatchVsyncEvent, time:%lld, frame:%u", aTimestamp, aFrameNumber);
> +
> +  if (!mEnableVsyncDispatch || !mNeedVsyncEvent) {
> +    return;
> +  }

Why not declare framenumber here? And increase here.
Please declare frameNum in GonkVsyncDispatcher.

::: widget/gonk/GonkVsyncDispatcher.h
@@ +24,5 @@
> +public:
> +  // Start up VsyncDispatcher on internal thread
> +  static void StartUp();
> +  // Start up VsyncDispatcher on current thread
> +  static void StartUpOnCurrentThread();

Add comment to mention which fun is called by content, which one is called by chrome

@@ +44,5 @@
> +
> +  // Register ipc VsyncEventParent
> +  void RegisterVsyncEventParent(layers::VsyncEventParent* aVsyncEventParent);
> +  void UnregisterVsyncEventParent(layers::VsyncEventParent* aVsyncEventParent);
> +

Move Register/Unregister to vysncDispatcher

@@ +47,5 @@
> +  void UnregisterVsyncEventParent(layers::VsyncEventParent* aVsyncEventParent);
> +
> +  // Check the observer number in VsyncDispatcher to enable/disable vsync event
> +  // notification.
> +  void CheckVsyncNotification();

Change name???

@@ +51,5 @@
> +  void CheckVsyncNotification();
> +
> +  // Get VsyncDispatcher's message loop
> +  MessageLoop* GetMessageLoop();
> +

Move GetMessageLoop() to VsyncDispatcher

Comment 7

[Boris Chiou [:boris]]

Comment on attachment 8467569 [details] [diff] [review]
[WIP] Part3: vsync dispatcher framework. v1

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

::: widget/VsyncDispatcher.h
@@ +14,5 @@
> +/*
> + * We would like to do some tasks aligned with vsync event. People can implement
> + * this class to do this stuff.
> + */
> +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>

Using "base::RefCountedThreadSafe<>" is only for scoped_ptr<>, right? It looks like only chromium and webrtc use scoped_ptr<>, so is it possible to use mozilla-style smart pointers, such as nsRefPtr?

Comment 8

[u459114]

Comment on attachment 8467741 [details] [diff] [review]
[WIP] Par4: register hwc hw vsync event. v1

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

::: widget/gonk/HwcComposer2D.h
@@ +31,5 @@
>  
>  namespace mozilla {
>  
> +class VsyncDispatcher;
> +

HwcComposer2D does not need to know VsyncDispatcher.

Define prototype of HW sync callback function(object refcount base callback object) here and implement callback function at VsyncDispatcher side. So that HwcComposer2D is decouple with VsyncDispatcher

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Merge all patch. Move gonk related code out of gecko.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part1: silk preference. v2

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync protocol and dispatch framework. v1

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part3: implement GonkVsyncDispatcher. v1

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to peter chang[:pchang][:peter] from comment #5)
> Comment on attachment 8467568 [details] [diff] [review]
> [WIP] Part2: vsync event protocol. v1
> 
> Review of attachment 8467568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/PVsyncEvent.ipdl
> @@ +21,5 @@
> > +  NotifyVsyncEvent(VsyncData aVsyncData);
> > +
> > +parent:
> > +  EnableVsyncEventNotification();
> > +  DisableVsyncEventNotification();
> 
> simplify to SetVsyncEventNotification(bool enable)

Rename to Re/UnregisterVsyncEvent(). It's more close to the underlying behavior.

> ::: gfx/layers/ipc/VsyncEventChild.cpp
> @@ +8,5 @@
> > +#include "nsXULAppAPI.h"
> > +
> > +//#define PRINT_VSYNC_DEBUG
> > +#ifdef PRINT_VSYNC_DEBUG
> > +#define VSYNC_DEBUG_MESSAGE printf_stderr("bignose tid:%d %s",gettid(),__PRETTY_FUNCTION__)
> 
> Please remove your name from debug log and change the debug log based on
> debug/error/warning.

remove all of these develop debug message.

> @@ +22,5 @@
> > +
> > +PVsyncEventChild*
> > +VsyncEventChild::Create(Transport* aTransport, ProcessId aOtherProcess)
> > +{
> > +  VSYNC_DEBUG_MESSAGE;
> 
> Do we need to log every functions?

checked

> @@ +26,5 @@
> > +  VSYNC_DEBUG_MESSAGE;
> > +
> > +  MOZ_RELEASE_ASSERT(NS_IsMainThread());
> > +  //MOZ_ASSERT(NS_IsMainThread());
> > +
> 
> Please change to MOZ_ASSERT for all of your patches.
> 

checked

> @@ +61,5 @@
> > +
> > +bool VsyncEventChild::RecvNotifyVsyncEvent(const VsyncData& aVsyncData)
> > +{
> > +  VSYNC_DEBUG_MESSAGE;
> > +
> 
> Do you miss //TODO here?

checked

> ::: gfx/layers/ipc/VsyncEventParent.cpp
> @@ +9,5 @@
> > +#include "nsXULAppAPI.h"
> > +
> > +//#define PRINT_VSYNC_DEBUG
> > +#ifdef PRINT_VSYNC_DEBUG
> > +#define VSYNC_DEBUG_MESSAGE printf_stderr("bignose tid:%d %s",gettid(),__PRETTY_FUNCTION__)
> 
> Please remove your name and refine the debug level

checked

> @@ +25,5 @@
> > +ConnectVsyncEventParent(PVsyncEventParent* aVsyncEventParent,
> > +                        Transport* aTransport,
> > +                        ProcessHandle aOtherProcess)
> > +{
> > +  VSYNC_DEBUG_MESSAGE;
> 
> Refine the debug message.

checked

> @@ +35,5 @@
> > +VsyncEventParent::StartUp()
> > +{
> > +  VSYNC_DEBUG_MESSAGE;
> > +
> > +  MOZ_RELEASE_ASSERT(NS_IsMainThread());
> 
> Please use MOZ_ASSERT

checked

> ::: gfx/thebes/gfxPlatform.cpp
> @@ +525,5 @@
> >  #endif
> > +
> > +#if defined MOZ_WIDGET_GONK && ANDROID_VERSION >= 17
> > +        if (gfxPrefs::SilkEnabled()) {
> > +            mozilla::layers::VsyncEventParent::StartUp();
> 
> Why do we need to check Android version here?
> You already checked the android version on
> GonkVsyncDispatcher::StartUpVsyncEvent().

remove the android version checking for startup/shutdown

Comment 14

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to peter chang[:pchang][:peter] from comment #6)
> Comment on attachment 8467569 [details] [diff] [review]
> [WIP] Part3: vsync dispatcher framework. v1
> 
> Review of attachment 8467569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In my opinion, it is better to merge part2 and part3 because part3 depends
> on part2.
> I didn't see any obvious reason to split into two parts.
> 
> ::: widget/gonk/GonkVsyncDispatcher.cpp
> @@ +15,5 @@
> > +#include "nsXULAppAPI.h"
> > +
> > +//#define DEBUG_VSYNC
> > +#ifdef DEBUG_VSYNC
> > +#define VSYNC_PRINT(...) do { printf_stderr("bignose " __VA_ARGS__); } while (0)
> 
> refine debug log

checked

> @@ +29,5 @@
> > +static MessageLoop* sVsyncDispatchMessageLoop = nullptr;
> > +
> > +static scoped_refptr<GonkVsyncDispatcher> sGonkVsyncDispatcher;
> > +
> > +class ArrayDataHelper
> 
> Please rename ArrayDataHelper because this name has no relation with
> CheckVsyncNotification()

move ArrayDataHelper::add/remove function into vsync dispatcher class body.

> @@ +30,5 @@
> > +
> > +static scoped_refptr<GonkVsyncDispatcher> sGonkVsyncDispatcher;
> > +
> > +class ArrayDataHelper
> > +{
> 
> I have another idea about mVsyncEventParentList.
> You can define a enum of vsync module list, like the following.
> enum {
>    InputDispatcher = 0,
>    Compositor,
>    Refreshdriver,
>    MAX_NUM
> }
> You can declare a atomic array with MAX_NUM size and register/unregister
> specific module by setting atomic_array[i] as 1 or 0.
> And then have another atomic counter to record how many modules registered.
> 
> With above changes, you don't need the PostMessage to update the module list.
> 

If we don't use PostMessage, We need to implement the lock-free array/list structure or use other synchronization method. Maybe we can check this part later.

> @@ +61,5 @@
> > +  }
> > +};
> > +
> > +static bool
> > +CreateThread()
> 
> Rename to CreateVsyncDispatchThread
> 

checked

> @@ +85,5 @@
> > +GonkVsyncDispatcher::GetInstance()
> > +{
> > +  return sGonkVsyncDispatcher;
> > +}
> > +
> 
> Can we separate these functions that called by chrome or content? Or it is
> common function.
> 
> Ex:
> /**********/
> func called by chrome
> ...
> ...
> /*********/
> func called by content
> ...
> ....
> 

add some suffix(OnChrome/OnContent) to prompt the function usage.

> @@ +96,5 @@
> > +  // only b2g need to create a new thread
> > +  MOZ_RELEASE_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> > +  //MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> > +
> > +  CreateThread();
> 
> check the return value of CreateThread()

Check the thread status in CreateThread().
CreateThread() will not return value.


> @@ +177,5 @@
> > +
> > +void
> > +GonkVsyncDispatcher::StartUpVsyncEvent()
> > +{
> > +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> 
> Change to MOZ_ASSERT

checked

> @@ +179,5 @@
> > +GonkVsyncDispatcher::StartUpVsyncEvent()
> > +{
> > +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> > +    if (!mInitVsyncEventGenerator){
> > +      mInitVsyncEventGenerator = true;
> 
> s/mInitVsyncEventGenerator/mInitHWVsync
> 
> @@ +189,5 @@
> > +        //TODO:
> > +        //init hw vsync event here.
> > +      }
> > +#endif
> > +      if (!mUseHWVsyncEventGenerator) {
> 
> s/mUseHWCsyncEventGenerator/mUseHWVsync

rename the mUseHWCsyncEventGenerator and mInitVsyncEventGenerator.
use mInitVsync and mUseHWVsync

> @@ +192,5 @@
> > +#endif
> > +      if (!mUseHWVsyncEventGenerator) {
> > +        //TODO:
> > +        //init software vsync event here.
> > +      }
> 
> Please dump which vsync type is using in debug/release build.
> 

checked

> @@ +200,5 @@
> > +
> > +void
> > +GonkVsyncDispatcher::ShutDownVsyncEvent()
> > +{
> > +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> 
> Change to MOZ_ASSERT
>

checked

> @@ +225,5 @@
> > +
> > +void
> > +GonkVsyncDispatcher::EnableVsyncDispatch(bool aEnable)
> > +{
> > +  mEnableVsyncDispatch = aEnable;
> 
> s/mEnableVsyncDispatch/mEnable  because this is inside GonkVysncDispatcher.
> 

checked

> @@ +234,5 @@
> > +{
> > +   int count = 0;
> > +
> > +   count += mVsyncEventParentList.Length();
> > +
> 
> return mVsyncEventParentList.Length();
> Is this thread-safe?
> 

This function is used internally, it's only called at vsync dispatch thread.

> @@ +241,5 @@
> > +
> > +void
> > +GonkVsyncDispatcher::EnableVsyncEvent(bool aEnable)
> > +{
> > +  if (mInitVsyncEventGenerator) {
> 
> Add MOZ_ASSERT about mInitHWVsync
> 

checked

> @@ +306,5 @@
> > +  VSYNC_PRINT("DispatchVsyncEvent, time:%lld, frame:%u", aTimestamp, aFrameNumber);
> > +
> > +  if (!mEnableVsyncDispatch || !mNeedVsyncEvent) {
> > +    return;
> > +  }
> 
> Why not declare framenumber here? And increase here.
> Please declare frameNum in GonkVsyncDispatcher.
> 

refined.

> ::: widget/gonk/GonkVsyncDispatcher.h
> @@ +24,5 @@
> > +public:
> > +  // Start up VsyncDispatcher on internal thread
> > +  static void StartUp();
> > +  // Start up VsyncDispatcher on current thread
> > +  static void StartUpOnCurrentThread();
> 
> Add comment to mention which fun is called by content, which one is called
> by chrome
> 

checked

> @@ +44,5 @@
> > +
> > +  // Register ipc VsyncEventParent
> > +  void RegisterVsyncEventParent(layers::VsyncEventParent* aVsyncEventParent);
> > +  void UnregisterVsyncEventParent(layers::VsyncEventParent* aVsyncEventParent);
> > +
> 
> Move Register/Unregister to vysncDispatcher
> 

checked

> @@ +47,5 @@
> > +  void UnregisterVsyncEventParent(layers::VsyncEventParent* aVsyncEventParent);
> > +
> > +  // Check the observer number in VsyncDispatcher to enable/disable vsync event
> > +  // notification.
> > +  void CheckVsyncNotification();
> 
> Change name???
> 

checked

> @@ +51,5 @@
> > +  void CheckVsyncNotification();
> > +
> > +  // Get VsyncDispatcher's message loop
> > +  MessageLoop* GetMessageLoop();
> > +
> 
> Move GetMessageLoop() to VsyncDispatcher

checked

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Boris Chiou [:boris] from comment #7)
> Comment on attachment 8467569 [details] [diff] [review]
> [WIP] Part3: vsync dispatcher framework. v1
> 
> Review of attachment 8467569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/VsyncDispatcher.h
> @@ +14,5 @@
> > +/*
> > + * We would like to do some tasks aligned with vsync event. People can implement
> > + * this class to do this stuff.
> > + */
> > +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>
> 
> Using "base::RefCountedThreadSafe<>" is only for scoped_ptr<>, right? It
> looks like only chromium and webrtc use scoped_ptr<>, so is it possible to
> use mozilla-style smart pointers, such as nsRefPtr?

The task which post to base::MessageLoop needs to have AddRef/Release function, so VsyncDispatcher use base::RefCountedThreadSafe<> here.
I would like to use all base::* in VsyncDispatcher instead of mixing nsXXX and base::* together.

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to C.J. Ku[:cjku] from comment #8)
> Comment on attachment 8467741 [details] [diff] [review]
> [WIP] Par4: register hwc hw vsync event. v1
> 
> Review of attachment 8467741 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.h
> @@ +31,5 @@
> >  
> >  namespace mozilla {
> >  
> > +class VsyncDispatcher;
> > +
> 
> HwcComposer2D does not need to know VsyncDispatcher.
> 
> Define prototype of HW sync callback function(object refcount base callback
> object) here and implement callback function at VsyncDispatcher side. So
> that HwcComposer2D is decouple with VsyncDispatcher

In attachment 8469051 [details] [diff] [review], only HwcComposer2D will include VsyncDispatcher header.
The GonkVsyncDispatcher need to use HwcComposer2D.
Only GonkVsyncDispatcher and HwcComposer2D has coupling.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: silk preference

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync protocol and dispatch framework. v2

fix indent

Comment 19

[u459114]

Comment on attachment 8469071 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v2

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

::: widget/VsyncDispatcher.h
@@ +24,5 @@
> + * module to dispatch vsync event to other module.
> + * We need to implement the platform dependent vsync related function for each
> + * platform(ex: StartUpVsyncEvent()).
> + */
> +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>

Please separate VyncDispatcher into VsyncDispatcherClient and VsyncDispatcherHost

@@ +33,5 @@
> +  static VsyncDispatcher* GetInstance();
> +
> +  // Start up VsyncDispatcher on chrome process. It will create a internal
> +  // thread for VsyncDispatcher message loop.
> +  static void StartUpOnChrome();

Why we need more static functions for a singleton object?

@@ +57,5 @@
> +
> +  void SetVsyncEventChild(layers::VsyncEventChild* aVsyncEventChild);
> +
> +  // Get VsyncDispatcher's message loop
> +  MessageLoop* GetMessageLoop();

Why public this function?

::: widget/shared/VsyncDispatcher.cpp
@@ +21,5 @@
> +using namespace layers;
> +
> +static base::Thread* sVsyncDispatchThread = nullptr;
> +static MessageLoop* sVsyncDispatchMessageLoop = nullptr;
> +

Move these two static globals into VsyncDispatcher's private segment

@@ +54,5 @@
> +#endif
> +
> +    MOZ_RELEASE_ASSERT(vsyncDispatcher);
> +
> +    sVsyncDispatcher = vsyncDispatcher;

nsContentUtils::RegisterShutdownObserver

@@ +70,5 @@
> +  CreateVsyncDispatchThread();
> +
> +  if (sVsyncDispatcher == nullptr) {
> +    // Force to create the vsync dispatcher here.
> +    GetInstance();

No need

@@ +89,5 @@
> +  }
> +
> +  if (sVsyncDispatcher == nullptr) {
> +    // Force to create the vsync dispatcher here.
> +    GetInstance();

No need

@@ +95,5 @@
> +}
> +
> +/*static*/ void
> +VsyncDispatcher::ShutDown()
> +{

When we want to shutdown VsyncDispatcher? VsyncEvent actor detroy?
Change this function to non static

void VsyncDispather::ShutDown()
{
  GetMessageLoop()->(FROM_HERE,
                     NewRunnableMethod(this,
                     &VsyncDispatcher::ShutDownInteral));
}

Do all the work in VsyncDispatcher::ShutDownInteral.
And I don't think we have to terminate VsyncDispatcher in shutdown function.

Comment 20

[Peter Chang[:pchang]]

Comment on attachment 8469071 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v2

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

::: gfx/layers/ipc/PVsyncEvent.ipdl
@@ +13,5 @@
> +{
> +  int64_t timeStamp;
> +  uint32_t frameNumber;
> +};
> +

Please add comment to explain this protocol here

::: gfx/layers/ipc/VsyncEventChild.cpp
@@ +25,5 @@
> +  }
> +
> +  VsyncEventChild* vsyncEventChild = nullptr;
> +
> +  VsyncDispatcher::StartUpOnContent();

Please replace above calls with VsyncDispatcher::GetInstance()->SetVsyncEventChild(vsyncEventChild);

::: gfx/layers/ipc/VsyncEventParent.cpp
@@ +27,5 @@
> +VsyncEventParent::StartUp()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  VsyncDispatcher::StartUpOnChrome();

Remove above calls and VsyncDispatcher will be created at VsyncEventParent::Create()
And I think you can remove this StartUP func.

@@ +35,5 @@
> +VsyncEventParent::ShutDown()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  VsyncDispatcher::ShutDown();

Is here a right place to call VsyncDispatcher::ShutDown()?

::: gfx/thebes/gfxPlatform.cpp
@@ +526,5 @@
> +
> +#ifdef MOZ_WIDGET_GONK
> +        if (gfxPrefs::SilkEnabled()) {
> +            mozilla::layers::VsyncEventParent::StartUp();
> +        }

After changing the place to initial VsyncDispatcher, we don't need this call here.

::: widget/VsyncDispatcher.h
@@ +24,5 @@
> + * module to dispatch vsync event to other module.
> + * We need to implement the platform dependent vsync related function for each
> + * platform(ex: StartUpVsyncEvent()).
> + */
> +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>

Right now we only have startup and DispatchVsync for content and chrome.
I would prefer to add chrome checking inside the startup and DispatchVsync func.

@@ +44,5 @@
> +
> +  // This function is called by vsync event generator.
> +  // Post a notify task to VsyncDispatcher's message loop.
> +  // The timestamp is microsecond.
> +  void NotifyVsync(int64_t aTimestamp);

s/aTimestamp/aVsyncTimeInMS

@@ +48,5 @@
> +  void NotifyVsync(int64_t aTimestamp);
> +
> +  // Dispatch vsync to observer
> +  void DispatchVsyncEventOnChrome(int64_t aTimestamp, uint32_t aFrameNumber);
> +  void DispatchVsyncEventOnContent(int64_t aTimestamp, uint32_t aFrameNumber);

s/aTimestamp/aVsyncTimeInMS

::: widget/shared/VsyncDispatcher.cpp
@@ +23,5 @@
> +static base::Thread* sVsyncDispatchThread = nullptr;
> +static MessageLoop* sVsyncDispatchMessageLoop = nullptr;
> +
> +static scoped_refptr<VsyncDispatcher> sVsyncDispatcher;
> +

I think you can move sVsyncDispatcher as VsyncDispatchers' private member

@@ +181,5 @@
> +int
> +VsyncDispatcher::GetVsyncObserverCount() const
> +{
> +  int count = 0;
> +

Add TODO to mention we will have more different VsyncObserver coming, like compostior.

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync protocol and dispatch framework. v3

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part3: implement GonkVsyncDispatcher. v2

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to C.J. Ku[:cjku] from comment #19)
> Comment on attachment 8469071 [details] [diff] [review]
> [WIP] Part2: vsync protocol and dispatch framework. v2
> 
> Review of attachment 8469071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/VsyncDispatcher.h
> @@ +24,5 @@
> > + * module to dispatch vsync event to other module.
> > + * We need to implement the platform dependent vsync related function for each
> > + * platform(ex: StartUpVsyncEvent()).
> > + */
> > +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>
> 
> Please separate VyncDispatcher into VsyncDispatcherClient and
> VsyncDispatcherHost
> 

I still use one VyncDispatcher for both chrome and content process.
I use some process type checking(chrome or content) to do the corresponding behavior.

> @@ +33,5 @@
> > +  static VsyncDispatcher* GetInstance();
> > +
> > +  // Start up VsyncDispatcher on chrome process. It will create a internal
> > +  // thread for VsyncDispatcher message loop.
> > +  static void StartUpOnChrome();
> 
> Why we need more static functions for a singleton object?
> 
checked

> @@ +57,5 @@
> > +
> > +  void SetVsyncEventChild(layers::VsyncEventChild* aVsyncEventChild);
> > +
> > +  // Get VsyncDispatcher's message loop
> > +  MessageLoop* GetMessageLoop();
> 
> Why public this function?
> 

Perhaps We would like to post some task running at vsync dispatcher thread in the future.

> ::: widget/shared/VsyncDispatcher.cpp
> @@ +21,5 @@
> > +using namespace layers;
> > +
> > +static base::Thread* sVsyncDispatchThread = nullptr;
> > +static MessageLoop* sVsyncDispatchMessageLoop = nullptr;
> > +
> 
> Move these two static globals into VsyncDispatcher's private segment
> 

checked 

> @@ +54,5 @@
> > +#endif
> > +
> > +    MOZ_RELEASE_ASSERT(vsyncDispatcher);
> > +
> > +    sVsyncDispatcher = vsyncDispatcher;
> 
> nsContentUtils::RegisterShutdownObserver
> 

I use base::scope_ptr to hold the VsyncDispatcher.
It will be destroyed at progoram exit.
VsyncDispatcher is still alive even though the ipc is shutdown.

> @@ +70,5 @@
> > +  CreateVsyncDispatchThread();
> > +
> > +  if (sVsyncDispatcher == nullptr) {
> > +    // Force to create the vsync dispatcher here.
> > +    GetInstance();
> 
> No need
> 

checked

> @@ +89,5 @@
> > +  }
> > +
> > +  if (sVsyncDispatcher == nullptr) {
> > +    // Force to create the vsync dispatcher here.
> > +    GetInstance();
> 
> No need
> 

checked

> @@ +95,5 @@
> > +}
> > +
> > +/*static*/ void
> > +VsyncDispatcher::ShutDown()
> > +{
> 
> When we want to shutdown VsyncDispatcher? VsyncEvent actor detroy?
> Change this function to non static
> 

checked

> void VsyncDispather::ShutDown()
> {
>   GetMessageLoop()->(FROM_HERE,
>                      NewRunnableMethod(this,
>                      &VsyncDispatcher::ShutDownInteral));
> }
> 
> Do all the work in VsyncDispatcher::ShutDownInteral.
> And I don't think we have to terminate VsyncDispatcher in shutdown function.

checked

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to peter chang[:pchang][:peter] from comment #20)
> Comment on attachment 8469071 [details] [diff] [review]
> [WIP] Part2: vsync protocol and dispatch framework. v2
> 
> Review of attachment 8469071 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/PVsyncEvent.ipdl
> @@ +13,5 @@
> > +{
> > +  int64_t timeStamp;
> > +  uint32_t frameNumber;
> > +};
> > +
> 
> Please add comment to explain this protocol here
> 

checked

> ::: gfx/layers/ipc/VsyncEventChild.cpp
> @@ +25,5 @@
> > +  }
> > +
> > +  VsyncEventChild* vsyncEventChild = nullptr;
> > +
> > +  VsyncDispatcher::StartUpOnContent();
> 
> Please replace above calls with
> VsyncDispatcher::GetInstance()->SetVsyncEventChild(vsyncEventChild);
> 

The StartUp function is used by Content and Chrome, so I would like to set the VsyncEventChild individually.

For content startup:
  VsyncDispatcher::GetInstance()->StartUp()
  VsyncDispatcher::GetInstance()->SetVsyncEventChild(vsyncEventChild);

shutdown:
  VsyncDispatcher::GetInstance()->SetVsyncEventChild(nullptr);
  VsyncDispatcher::GetInstance()->Shutdown()
  
> ::: gfx/layers/ipc/VsyncEventParent.cpp
> @@ +27,5 @@
> > +VsyncEventParent::StartUp()
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  VsyncDispatcher::StartUpOnChrome();
> 
> Remove above calls and VsyncDispatcher will be created at
> VsyncEventParent::Create()
> And I think you can remove this StartUP func.
> 

checked
In chrome, we will create the vsync dispatcher out of the VsyncEvent protocol.

> @@ +35,5 @@
> > +VsyncEventParent::ShutDown()
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  VsyncDispatcher::ShutDown();
> 
> Is here a right place to call VsyncDispatcher::ShutDown()?
> 

checked
I choose the place at gfxPlatform::Shutdown().

> ::: gfx/thebes/gfxPlatform.cpp
> @@ +526,5 @@
> > +
> > +#ifdef MOZ_WIDGET_GONK
> > +        if (gfxPrefs::SilkEnabled()) {
> > +            mozilla::layers::VsyncEventParent::StartUp();
> > +        }
> 
> After changing the place to initial VsyncDispatcher, we don't need this call
> here.
> 

I change the init to gfxPlatform::Init().

> ::: widget/VsyncDispatcher.h
> @@ +24,5 @@
> > + * module to dispatch vsync event to other module.
> > + * We need to implement the platform dependent vsync related function for each
> > + * platform(ex: StartUpVsyncEvent()).
> > + */
> > +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>
> 
> Right now we only have startup and DispatchVsync for content and chrome.
> I would prefer to add chrome checking inside the startup and DispatchVsync
> func.
> 

checked

> @@ +44,5 @@
> > +
> > +  // This function is called by vsync event generator.
> > +  // Post a notify task to VsyncDispatcher's message loop.
> > +  // The timestamp is microsecond.
> > +  void NotifyVsync(int64_t aTimestamp);
> 
> s/aTimestamp/aVsyncTimeInMS
> 

aTimestampUS


> @@ +48,5 @@
> > +  void NotifyVsync(int64_t aTimestamp);
> > +
> > +  // Dispatch vsync to observer
> > +  void DispatchVsyncEventOnChrome(int64_t aTimestamp, uint32_t aFrameNumber);
> > +  void DispatchVsyncEventOnContent(int64_t aTimestamp, uint32_t aFrameNumber);
> 
> s/aTimestamp/aVsyncTimeInMS
> 
> ::: widget/shared/VsyncDispatcher.cpp
> @@ +23,5 @@
> > +static base::Thread* sVsyncDispatchThread = nullptr;
> > +static MessageLoop* sVsyncDispatchMessageLoop = nullptr;
> > +
> > +static scoped_refptr<VsyncDispatcher> sVsyncDispatcher;
> > +
> 
> I think you can move sVsyncDispatcher as VsyncDispatchers' private member
> 

checked

> @@ +181,5 @@
> > +int
> > +VsyncDispatcher::GetVsyncObserverCount() const
> > +{
> > +  int count = 0;
> > +
> 
> Add TODO to mention we will have more different VsyncObserver coming, like
> compostior.

checked

Comment 25

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync protocol and dispatch framework. v4

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part3: implement GonkVsyncDispatcher. v3

Comment 27

[Peter Chang[:pchang]]

Comment on attachment 8469870 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v4

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

::: gfx/layers/ipc/PVsyncEvent.ipdl
@@ +19,5 @@
> +
> +/*
> + * We would like to do some tasks aligned with vsync event. We use this
> + * protocol to notify vsync event between chrome and content process.
> + */

The PVsyncEvent protocol is used to notify Vsync events from chrome to content process and also provides the interfaces for content to register/unregister Vsync events.

::: gfx/layers/ipc/VsyncEventChild.cpp
@@ +25,5 @@
> +  }
> +
> +  VsyncEventChild* vsyncEventChild = nullptr;
> +
> +  VsyncDispatcher::GetInstance()->StartUp();

Why not call  VsyncDispatcher::GetInstance()->SetVsyncEventChild(vsyncEventChild) here?

::: widget/VsyncDispatcher.h
@@ +67,5 @@
> +  // Sent vsync event to all registered content processes
> +  void NotifyContentProcess(int64_t aTimestampUS, uint32_t aFrameNumber);
> +
> +  // Generate frame number and call dispatch event.
> +  void NotifyVsyncTask(int64_t aTimestampUS);

new line here

@@ +82,5 @@
> +
> +  void EnableVsyncDispatchTask(bool aEnable);
> +
> +  // Return total registered object number.
> +  int GetVsyncObserverCount() const;

new line here

@@ +96,5 @@
> +      aList->AppendElement(aItem);
> +    }
> +
> +    CheckVsyncObserverNumber();
> +  }

Please move implementation to cpp

::: widget/shared/VsyncDispatcher.cpp
@@ +25,5 @@
> +
> +/*static*/ VsyncDispatcher*
> +VsyncDispatcher::GetInstance()
> +{
> +  MutexAutoLock lock(mVsyncDispatcherMutex);

I would prefer not to have this lock but initialize VsyncDispatcher in early place.

@@ +33,5 @@
> +
> +    //TODO: put other platform's VsyncDispatcher here
> +#ifdef MOZ_WIDGET_GONK
> +    vsyncDispatcher = new GonkVsyncDispatcher();
> +    MOZ_RELEASE_ASSERT(vsyncDispatcher, "New GonkVsyncDispatcher failed.");

nit: failed to create GonkVsyncDispatcher

@@ +60,5 @@
> +
> +void
> +VsyncDispatcher::StartUp()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "VsyncDispatcher StartUp() should in main thread.");

Does this condition works for chrome and content process?

@@ +102,5 @@
> +VsyncDispatcher::VsyncDispatcher()
> +  : mVsyncDispatchThread(nullptr)
> +  , mVsyncDispatchMessageLoop(nullptr)
> +  , mVsyncEventChild(nullptr)
> +  , mEnableDispatch(true)

why do we init mEnableDispatch as true?

@@ +112,5 @@
> +VsyncDispatcher::~VsyncDispatcher()
> +{
> +  if (mVsyncDispatchThread) {
> +    delete mVsyncDispatchThread;
> +  }

please clear mVsyncEventChild/mVsyncDispatchMessageLoop

Comment 28

[Peter Chang[:pchang]]

Comment on attachment 8469872 [details] [diff] [review]
[WIP] Part3: implement GonkVsyncDispatcher. v3

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

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +29,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "StartUpVsyncEvent should at main thread");
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
> +      "StartUpVsyncEvent should be called in chrome");
> +

if (mInitVsync)
  return;

@@ +32,5 @@
> +      "StartUpVsyncEvent should be called in chrome");
> +
> +  if (!mInitVsync){
> +    mInitVsync = true;
> +    mUseHWVsync = false;

mUseHWVsync = false is redundant here.

@@ +40,5 @@
> +    if (gfxPrefs::SilkHWVsyncEnabled()) {
> +      // init hw vsync event
> +      HwcComposer2D::GetInstance()->RegisterHwcEventCallback();
> +      if (HwcComposer2D::GetInstance()->HasHWVsync()) {
> +        HwcComposer2D::GetInstance()->RegisterVsyncDispatcher(this);

I think we discuss that we will pass the callback to HWC here. Is it right?

@@ +41,5 @@
> +      // init hw vsync event
> +      HwcComposer2D::GetInstance()->RegisterHwcEventCallback();
> +      if (HwcComposer2D::GetInstance()->HasHWVsync()) {
> +        HwcComposer2D::GetInstance()->RegisterVsyncDispatcher(this);
> +        mUseHWVsync = true;

How to guarantee HW vsync is on since you only register callback?

@@ +42,5 @@
> +      HwcComposer2D::GetInstance()->RegisterHwcEventCallback();
> +      if (HwcComposer2D::GetInstance()->HasHWVsync()) {
> +        HwcComposer2D::GetInstance()->RegisterVsyncDispatcher(this);
> +        mUseHWVsync = true;
> +        printf_stderr("GonkVsyncDispatcher: use hwc vsync");

Need to change log level

@@ +48,5 @@
> +    }
> +#endif
> +    if (!mUseHWVsync) {
> +      //TODO: init software vsync event here.
> +      printf_stderr("GonkVsyncDispatcher: use soft vsync");

Need to change log level

@@ +76,5 @@
> +
> +void
> +GonkVsyncDispatcher::EnableVsyncEvent(bool aEnable)
> +{
> +  MOZ_ASSERT(mInitVsync, "VsyncEvent is not ready");

MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
"ShutDownVsyncEvent should be called in chrome");

Comment 29

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to peter chang[:pchang][:peter] from comment #27)
> Comment on attachment 8469870 [details] [diff] [review]
> [WIP] Part2: vsync protocol and dispatch framework. v4
> 
> Review of attachment 8469870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/PVsyncEvent.ipdl
> @@ +19,5 @@
> > +
> > +/*
> > + * We would like to do some tasks aligned with vsync event. We use this
> > + * protocol to notify vsync event between chrome and content process.
> > + */
> 
> The PVsyncEvent protocol is used to notify Vsync events from chrome to
> content process and also provides the interfaces for content to
> register/unregister Vsync events.
> 

checked

> ::: gfx/layers/ipc/VsyncEventChild.cpp
> @@ +25,5 @@
> > +  }
> > +
> > +  VsyncEventChild* vsyncEventChild = nullptr;
> > +
> > +  VsyncDispatcher::GetInstance()->StartUp();
> 
> Why not call 
> VsyncDispatcher::GetInstance()->SetVsyncEventChild(vsyncEventChild) here?
> 

checked

> ::: widget/VsyncDispatcher.h
> @@ +67,5 @@
> > +  // Sent vsync event to all registered content processes
> > +  void NotifyContentProcess(int64_t aTimestampUS, uint32_t aFrameNumber);
> > +
> > +  // Generate frame number and call dispatch event.
> > +  void NotifyVsyncTask(int64_t aTimestampUS);
> 
> new line here
> 
> @@ +82,5 @@
> > +
> > +  void EnableVsyncDispatchTask(bool aEnable);
> > +
> > +  // Return total registered object number.
> > +  int GetVsyncObserverCount() const;
> 
> new line here
> 
> @@ +96,5 @@
> > +      aList->AppendElement(aItem);
> > +    }
> > +
> > +    CheckVsyncObserverNumber();
> > +  }
> 
> Please move implementation to cpp
> 

This is function template, and I call the class member function CheckVsyncObserverNumber() in it.
So it will place at header.

> ::: widget/shared/VsyncDispatcher.cpp
> @@ +25,5 @@
> > +
> > +/*static*/ VsyncDispatcher*
> > +VsyncDispatcher::GetInstance()
> > +{
> > +  MutexAutoLock lock(mVsyncDispatcherMutex);
> 
> I would prefer not to have this lock but initialize VsyncDispatcher in early
> place.
> 

checked
We call VsyncDispatcher::GetInstance() at gfxPlatform::Init().
Assume that this is the first time to use GetInstance() and there is no other user to call GetInstance().

> @@ +33,5 @@
> > +
> > +    //TODO: put other platform's VsyncDispatcher here
> > +#ifdef MOZ_WIDGET_GONK
> > +    vsyncDispatcher = new GonkVsyncDispatcher();
> > +    MOZ_RELEASE_ASSERT(vsyncDispatcher, "New GonkVsyncDispatcher failed.");
> 
> nit: failed to create GonkVsyncDispatcher
> 

checked

> @@ +60,5 @@
> > +
> > +void
> > +VsyncDispatcher::StartUp()
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread(), "VsyncDispatcher StartUp() should in main thread.");
> 
> Does this condition works for chrome and content process?
> 

Chrome:
  call StartUp() in gfxPlatform::Init() => main thread
Content:
  call StartUp() in VsyncEventChild::Create() => main thread

> @@ +102,5 @@
> > +VsyncDispatcher::VsyncDispatcher()
> > +  : mVsyncDispatchThread(nullptr)
> > +  , mVsyncDispatchMessageLoop(nullptr)
> > +  , mVsyncEventChild(nullptr)
> > +  , mEnableDispatch(true)
> 
> why do we init mEnableDispatch as true?
> 

After StartUp(), we will start to dispatch vsync by default.

> @@ +112,5 @@
> > +VsyncDispatcher::~VsyncDispatcher()
> > +{
> > +  if (mVsyncDispatchThread) {
> > +    delete mVsyncDispatchThread;
> > +  }
> 
> please clear mVsyncEventChild/mVsyncDispatchMessageLoop

checked

Comment 30

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to peter chang[:pchang][:peter] from comment #28)
> Comment on attachment 8469872 [details] [diff] [review]
> [WIP] Part3: implement GonkVsyncDispatcher. v3
> 
> Review of attachment 8469872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/GonkVsyncDispatcher.cpp
> @@ +29,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread(), "StartUpVsyncEvent should at main thread");
> > +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
> > +      "StartUpVsyncEvent should be called in chrome");
> > +
> 
> if (mInitVsync)
>   return;
> 

checked

> @@ +32,5 @@
> > +      "StartUpVsyncEvent should be called in chrome");
> > +
> > +  if (!mInitVsync){
> > +    mInitVsync = true;
> > +    mUseHWVsync = false;
> 
> mUseHWVsync = false is redundant here.
> 

checked

> @@ +40,5 @@
> > +    if (gfxPrefs::SilkHWVsyncEnabled()) {
> > +      // init hw vsync event
> > +      HwcComposer2D::GetInstance()->RegisterHwcEventCallback();
> > +      if (HwcComposer2D::GetInstance()->HasHWVsync()) {
> > +        HwcComposer2D::GetInstance()->RegisterVsyncDispatcher(this);
> 
> I think we discuss that we will pass the callback to HWC here. Is it right?
> 

If we use a HWC callback, we also need to create another type of call back for other platform.
I prefer to register VsyncDispatcher into vsync module.

> @@ +41,5 @@
> > +      // init hw vsync event
> > +      HwcComposer2D::GetInstance()->RegisterHwcEventCallback();
> > +      if (HwcComposer2D::GetInstance()->HasHWVsync()) {
> > +        HwcComposer2D::GetInstance()->RegisterVsyncDispatcher(this);
> > +        mUseHWVsync = true;
> 
> How to guarantee HW vsync is on since you only register callback?
> 

Actually, the b2g will call GonkDisplay::SetEnable(true) several times during the booting. SetEnable() will enable the HW vsync.
But we can call EnableVsync() explicitly here.

> @@ +42,5 @@
> > +      HwcComposer2D::GetInstance()->RegisterHwcEventCallback();
> > +      if (HwcComposer2D::GetInstance()->HasHWVsync()) {
> > +        HwcComposer2D::GetInstance()->RegisterVsyncDispatcher(this);
> > +        mUseHWVsync = true;
> > +        printf_stderr("GonkVsyncDispatcher: use hwc vsync");
> 
> Need to change log level
> 

checked

> @@ +48,5 @@
> > +    }
> > +#endif
> > +    if (!mUseHWVsync) {
> > +      //TODO: init software vsync event here.
> > +      printf_stderr("GonkVsyncDispatcher: use soft vsync");
> 
> Need to change log level
> 
> @@ +76,5 @@
> > +
> > +void
> > +GonkVsyncDispatcher::EnableVsyncEvent(bool aEnable)
> > +{
> > +  MOZ_ASSERT(mInitVsync, "VsyncEvent is not ready");
> 
> MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
> "ShutDownVsyncEvent should be called in chrome");

checked

Comment 31

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync protocol and dispatch framework. v5

Comment 32

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part3: implement GonkVsyncDispatcher. v4

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8469999 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v5

Hi Ben,
Could you please review the PVsyncEvent protocol in silk?
We have a wiki page for silk design.
https://wiki.mozilla.org/Project_Silk


protocol related file:
---
dom/ipc/ContentChild.cpp
dom/ipc/ContentChild.h
dom/ipc/ContentParent.cpp
dom/ipc/ContentParent.h
dom/ipc/PContent.ipdl
gfx/layers/ipc/PVsyncEvent.ipdl
gfx/layers/ipc/VsyncEventChild.cpp
gfx/layers/ipc/VsyncEventChild.h
gfx/layers/ipc/VsyncEventParent.cpp
gfx/layers/ipc/VsyncEventParent.h
gfx/layers/moz.build

Comment 34

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8470000 [details] [diff] [review]
[WIP] Part3: implement GonkVsyncDispatcher. v4

Hi Michael,
This patch enable/disable the hwc hw vsync event in Bug 987527.
We use this as the hw vsync event in project silk. We will implement the software vsync event at another bug.

Comment 35

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Milan,
Could you please help us to recommend the reviewer for the pref in attachment 8469050 [details] [diff] [review] and the VsyncDispatcher in attachment 8469999 [details] [diff] [review].

The VsyncDispatcher will use PVsyncEvent protocol, so it's hard to review if I split the attachment 8469999 [details] [diff] [review] into two patch.

The VsyncDispatcher related file:
---
gfx/layers/ipc/VsyncEventChild.cpp
gfx/layers/ipc/VsyncEventParent.cpp
gfx/thebes/gfxPlatform.cpp
widget/VsyncDispatcher.h
widget/moz.build
widget/shared/VsyncDispatcher.cpp
widget/shared/moz.build

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8469050 [details] [diff] [review]
[WIP] Part1: silk preference. v2

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

::: gfx/thebes/gfxPrefs.h
@@ +194,5 @@
>    DECL_GFX_PREF(Once, "gfx.work-around-driver-bugs",           WorkAroundDriverBugs, bool, true);
>  
>    DECL_GFX_PREF(Live, "gfx.draw-color-bars",                   CompositorDrawColorBars, bool, false);
>  
> +#ifdef MOZ_WIDGET_GONK

Rather than #ifdef in here, we should have a clean setup in the gfxPrefs.h, and change the values in b2g/app/b2g.js instead.  So, no #ifdef MOZ_WIDGET_GONK, just declare both prefs as false, and then set them to true as appropriate in b2g/app/b2g.js

@@ +196,5 @@
>    DECL_GFX_PREF(Live, "gfx.draw-color-bars",                   CompositorDrawColorBars, bool, false);
>  
> +#ifdef MOZ_WIDGET_GONK
> +  // Do tick and compose aligned with vsync.
> +  DECL_GFX_PREF(Once, "gfx.silk",                              SilkEnabled, bool, false);

Bad idea to put code name in the source code, so we should not use "silk" in either preference names or method names. Something like gfx.frameuniformity, gfx.frameuniformity.hw-vsync for the preference names and FrameUniformityEnabled, FrameUniformityVsyncEnabled for the method names would make more sense.

Comment 37

[Milan Sreckovic [:milan] (needinfo for best results)]

Also (and perhaps first), the overall design should be reviewed and "r+"-ed in bug 987532 (see my request in https://bugzilla.mozilla.org/show_bug.cgi?id=987532#c15)

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8470000 [details] [diff] [review]
[WIP] Part3: implement GonkVsyncDispatcher. v4

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

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +40,5 @@
> +  }
> +
> +  mInitVsync = true;
> +
> +#if ANDROID_VERSION >= 17

Why do we need this #if?  Isn't HWVsync already only enabled when the version is >= 17?

@@ +67,5 @@
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
> +      "ShutDownVsyncEvent should be called in chrome");
> +
> +  if (mInitVsync) {
> +#if ANDROID_VERSION >= 17

Same comment as above.  We should always unregister if mUseHWVsync is true, no need to wrap it in #if ANDROID_VERSION >= 17.

@@ +76,5 @@
> +    if (!mUseHWVsync) {
> +      //TODO:
> +      //release software vsync event here.
> +    }
> +  }

Should we now set mInitVsync to false?

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8469999 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v5

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

::: widget/VsyncDispatcher.h
@@ +29,5 @@
> + * module to dispatch vsync event to other module.
> + * We need to implement the platform dependent vsync related function for each
> + * platform(ex: StartUpVsyncEvent()).
> + */
> +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>

I'm a bit confused on the VsyncDispather memory management and ownership.  It's a singleton?  Is it getting released on shutdown?  Where is the reference counting coming into the picture if we have a singleton?  Are others supposed to be holding on to the singleton or always call GetInstance()?

Comment 40

[Avi Halachmi (:avih)]

(I didn't review the patches, so if it's already handled, apologies).

What happens with multiple monitors where each has its own vsync signal? Does the framework support it?

While mostly a desktop thing, multiple/external display is supported by Android and possibly others, so I think the framework should at least be extendable to support more than one display, and of course it should behave gracefully with multiple displays from day one, on systems where multiple displays are supported.

Comment 41

[Milan Sreckovic [:milan] (needinfo for best results)]

Good topic.  This is really "dealing with events and scheduling paints", so I don't see ever being able to deal with multiple sources, or we'd have to paint separately for each one.  I can see us being able to switch what the source of the sync signal is, but I wouldn't worry to much about it right now - that kind of a change shouldn't be too difficult to add after the fact.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Milan Sreckovic [:milan] (PTO 8/11 - 8/15) from comment #39)
> Comment on attachment 8469999 [details] [diff] [review]
> [WIP] Part2: vsync protocol and dispatch framework. v5
> 
> Review of attachment 8469999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/VsyncDispatcher.h
> @@ +29,5 @@
> > + * module to dispatch vsync event to other module.
> > + * We need to implement the platform dependent vsync related function for each
> > + * platform(ex: StartUpVsyncEvent()).
> > + */
> > +class VsyncDispatcher : public base::RefCountedThreadSafe<VsyncDispatcher>
> 
> I'm a bit confused on the VsyncDispather memory management and ownership. 
> It's a singleton?  Is it getting released on shutdown?  Where is the

VsyncDispather is a singleton for each process.
It's a static refcount object and it will alive until the process exit.
The shutdown() only disables the dispatch function in VsyncDispather. The internal thread is still alive.
It prevent the crash if we use VsyncDispather(especially the GetMessageLoop() function) after we call shutdown() in gfxPlatform::Shutdown().

> reference counting coming into the picture if we have a singleton?  Are
> others supposed to be holding on to the singleton or always call
> GetInstance()?

Hwc will hold the VsyncDispather.
HwcComposer2D::GetInstance()->RegisterVsyncDispatcher(VsyncDispatcher*)

Other module will not hold VsyncDispather. They just use GetInstance().

Comment 43

[Peter Chang[:pchang]]

Comment on attachment 8470000 [details] [diff] [review]
[WIP] Part3: implement GonkVsyncDispatcher. v4

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

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +66,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "ShutDownVsyncEvent should at main thread");
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
> +      "ShutDownVsyncEvent should be called in chrome");
> +
> +  if (mInitVsync) {

return if mInitVsync is false

@@ +84,5 @@
> +GonkVsyncDispatcher::EnableVsyncEvent(bool aEnable)
> +{
> +  MOZ_ASSERT(IsInVsyncDispatcherThread(), "Call EnableVsyncEvent at wrong thread");
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default,
> +      "EnableVsyncEvent should be called in chrome");

No need to check GetProcessType because IsInVsyncDispatcherThread already did.

::: widget/gonk/GonkVsyncDispatcher.h
@@ +24,5 @@
> +  virtual void ShutDownVsyncEvent() MOZ_OVERRIDE;
> +  virtual void EnableVsyncEvent(bool aEnable) MOZ_OVERRIDE;
> +
> +  // vsync event generator.
> +  bool mInitVsync;

s/mInitVsync/mVsyncInited

Comment 44

[Peter Chang[:pchang]]

Comment on attachment 8469999 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v5

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

::: gfx/layers/ipc/VsyncEventChild.cpp
@@ +23,5 @@
> +  if (!OpenProcessHandle(aOtherProcess, &processHandle)) {
> +    return nullptr;
> +  }
> +
> +  VsyncEventChild* vsyncEventChild = nullptr;

No need for this line and you can combine it with the following 'new VsyncEventChild'.

::: widget/shared/VsyncDispatcher.cpp
@@ +88,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "VsyncDispatcher ShutDown() should in main thread.");
> +  MOZ_ASSERT(mInit, "VsyncDispatcher is not initialized.");
> +
> +  EnableVsyncDispatch(false);

No need for EnableVsyncDispatch, you can set mInit as false and also check mInit or mEnableDispatch for every function.
BTW, why don't you free mVsyncDispatcher during shutdown stage?

@@ +121,5 @@
> +
> +void
> +VsyncDispatcher::NotifyVsync(int64_t aTimestampUS)
> +{
> +  GetMessageLoop()->PostTask(FROM_HERE,

Check mEnableDispatch here.
Does this func is called by chrome? If so, please check process here.

@@ +135,5 @@
> +    return;
> +  }
> +
> +  static uint32_t frameNumber = 0;
> +

Please add comment to describe the usage for frameNumber.

@@ +146,5 @@
> +  MOZ_ASSERT(IsInVsyncDispatcherThread(), "Call DispatchVsyncEvent at wrong thread");
> +
> +  if (!mEnableDispatch || !mNeedVsyncEvent) {
> +    return;
> +  }

Please merge mEnableDispatch and mNeedVsyncEvent, I think they means the same thing.

@@ +164,5 @@
> +VsyncDispatcher::EnableVsyncDispatch(bool aEnable)
> +{
> +  GetMessageLoop()->PostTask(FROM_HERE,
> +                             NewRunnableMethod(this,
> +                             &VsyncDispatcher::EnableVsyncDispatch,

It should be VsyncDispatcher::EnableVsyncDispatchTask.

@@ +230,5 @@
> +}
> +
> +void
> +VsyncDispatcher::CheckVsyncObserverNumber()
> +{

Here you try to enable/disable vsyncDispatch, it is better to rename to EnableVsyncDispatchIfhasObserver.
And please also check mInit here.

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8469999 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v5

I would like to modify the VsyncDispatcher lifecycle to make it destroyed(including the internal thread) at Shutdown() call.

Comment 46

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8470000 [details] [diff] [review]
[WIP] Part3: implement GonkVsyncDispatcher. v4

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

::: widget/gonk/GonkVsyncDispatcher.cpp
@@ +42,5 @@
> +  mInitVsync = true;
> +
> +#if ANDROID_VERSION >= 17
> +  // Use hw event or software vsync event
> +  if (gfxPrefs::SilkHWVsyncEnabled()) {

Project name "silk" should not be used. IIRC, it is already commented on other silk related bug.

Comment 47

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8470000 [details] [diff] [review]
[WIP] Part3: implement GonkVsyncDispatcher. v4

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

Sorry, I cleared review flag incorrectly.

Comment 48

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8470000 [details] [diff] [review]
[WIP] Part3: implement GonkVsyncDispatcher. v4

I will fix the pref first.

Comment 49

[Bas Schouten (:bas.schouten)]

I will have a look at this by tomorrow.

Comment 50

[Bas Schouten (:bas.schouten)]

Comment on attachment 8469999 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v5

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

I'm still looking at the exact details of the implementation here. One question that pops to mind though, in the portions of the code that are not directly working with IPDL, is there a particular reason we've chosen to use Chromium threads rather than XPCOM threads? To the best of my knowledge in general we should still be using XPCOM thread in regular code (in this case VsyncDispatcher).

::: gfx/layers/ipc/VsyncEventChild.cpp
@@ +27,5 @@
> +  VsyncEventChild* vsyncEventChild = nullptr;
> +
> +  VsyncDispatcher::GetInstance()->StartUp();
> +  vsyncEventChild = new VsyncEventChild(aTransport);
> +  VsyncDispatcher::GetInstance()->SetVsyncEventChild(vsyncEventChild);

nit: vertical whitespace would be nice

Comment 51

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: silk preference. v3

update the pref name

Comment 52

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: vsync protocol and dispatch framework. v6

Change the vsync dispatcher life cycle(delete the vsync dispatcher at Shutdown()).

Comment 53

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: implement GonkVsyncDispatcher. v5

Base on bug 987527 attachment 8472132 [details] [diff] [review], add a vsync callback to hwc.
Fix gfxpref name.

Comment 54

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: silk preference. v4

Fix the pref name for comment 36.

Comment 55

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8472146 [details] [diff] [review]
Part2: vsync protocol and dispatch framework. v6

The design doc: https://wiki.mozilla.org/Project_Silk

1. split VsyncDispatcher into VsyncDispatcherHost and VsyncDispatcherChild.
2. change the VsyncDispatcher* life cycle. They will be deleted in shutdown().

---
Ben,
I don't change the PVsyncEvent protocol part.
Please check:
dom/ipc/ContentChild.cpp
dom/ipc/ContentChild.h
dom/ipc/ContentParent.cpp
dom/ipc/ContentParent.h
dom/ipc/PContent.ipdl
gfx/layers/ipc/PVsyncEvent.ipdl
gfx/layers/ipc/VsyncEventChild.cpp
gfx/layers/ipc/VsyncEventChild.h
gfx/layers/ipc/VsyncEventParent.cpp
gfx/layers/ipc/VsyncEventParent.h
gfx/layers/moz.build

Comment 56

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Bas Schouten (:bas.schouten) from comment #50)
> Comment on attachment 8469999 [details] [diff] [review]
> [WIP] Part2: vsync protocol and dispatch framework. v5
> 
> Review of attachment 8469999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm still looking at the exact details of the implementation here. One
> question that pops to mind though, in the portions of the code that are not
> directly working with IPDL, is there a particular reason we've chosen to use
> Chromium threads rather than XPCOM threads? To the best of my knowledge in
> general we should still be using XPCOM thread in regular code (in this case
> VsyncDispatcher).

The PVsyncEvent protocol will use the VsyncDispatcherHost's internal thread, so I choose Chromium thread here.

Comment 57

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: vsync protocol and dispatch framework. v7

1. hide VsyncDispatcherHost/Client implementation for user.
2. add GetVsyncRate in PVsyncEvent protocol.

Comment 58

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: implement GonkVsyncDispatcher. v6

1. add GetHWVsyncRate function.

Comment 59

[Michael Wu [:mwu]]

Comment on attachment 8474565 [details] [diff] [review]
Part3: implement GonkVsyncDispatcher. v6

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

::: widget/gonk/GonkVsyncDispatcher.h
@@ +20,5 @@
> +  GonkVsyncDispatcher();
> +  ~GonkVsyncDispatcher();
> +
> +  virtual void StartUpVsyncEvent() MOZ_OVERRIDE;
> +  virtual void ShutDownVsyncEvent() MOZ_OVERRIDE;

StartUp and ShutDown look very weird. Try using Startup and Shutdown.

::: widget/gonk/HwcComposer2D.cpp
@@ +213,5 @@
> +    device->getDisplayAttributes(device, 0, 0, HWC_ATTRIBUTES, hwcAttributeValues);
> +    if (hwcAttributeValues[0] > 0) {
> +      mVsyncRate = 1.0e9 / hwcAttributeValues[0] + 0.5;
> +    }
> +    else {

else on the same line as }, here and elsewhere.

@@ +251,5 @@
>  HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
>  {
> +    if (mVsyncCallback) {
> +        mVsyncCallback();
> +    }

I prefer the API to reversed here - HwcComposer grabs the VsyncDispatcher and notifies it directly.

::: widget/gonk/HwcComposer2D.h
@@ +103,4 @@
>      void Vsync(int aDisplay, int64_t aTimestamp);
> +
> +    // Vsync event rate per second.
> +    uint32_t GetHWVsycnRate() const;

Typo that you've apparently managed to copy and paste everywhere.

Comment 60

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Michael Wu [:mwu] from comment #59)
> Comment on attachment 8474565 [details] [diff] [review]
> Part3: implement GonkVsyncDispatcher. v6
> 
> Review of attachment 8474565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/GonkVsyncDispatcher.h
> @@ +20,5 @@
> > +  GonkVsyncDispatcher();
> > +  ~GonkVsyncDispatcher();
> > +
> > +  virtual void StartUpVsyncEvent() MOZ_OVERRIDE;
> > +  virtual void ShutDownVsyncEvent() MOZ_OVERRIDE;
> 
> StartUp and ShutDown look very weird. Try using Startup and Shutdown.
> 

checked.

> ::: widget/gonk/HwcComposer2D.cpp
> @@ +213,5 @@
> > +    device->getDisplayAttributes(device, 0, 0, HWC_ATTRIBUTES, hwcAttributeValues);
> > +    if (hwcAttributeValues[0] > 0) {
> > +      mVsyncRate = 1.0e9 / hwcAttributeValues[0] + 0.5;
> > +    }
> > +    else {
> 
> else on the same line as }, here and elsewhere.
>

checked.
 
> @@ +251,5 @@
> >  HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> >  {
> > +    if (mVsyncCallback) {
> > +        mVsyncCallback();
> > +    }
> 
> I prefer the API to reversed here - HwcComposer grabs the VsyncDispatcher
> and notifies it directly.
> 

If HwcComposer call VsyncDispatcherHost directly, it should include the VsyncDispatcherHost stuff.
The HwcComposer use only one "NotifyVsync()" function, but it include whole VsyncDispatcherHost.
Furthermore, HwcComposer might hold one referene count of VsyncDispatcher. We would like to control the 
VsyncDispatcher life cycle within VsyncDispatcher module.

> ::: widget/gonk/HwcComposer2D.h
> @@ +103,4 @@
> >      void Vsync(int aDisplay, int64_t aTimestamp);
> > +
> > +    // Vsync event rate per second.
> > +    uint32_t GetHWVsycnRate() const;
> 
> Typo that you've apparently managed to copy and paste everywhere.
Thanks, rename VsycnRate to VsyncRate.

Comment 61

[Michael Wu [:mwu]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #60)
> > @@ +251,5 @@
> > >  HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> > >  {
> > > +    if (mVsyncCallback) {
> > > +        mVsyncCallback();
> > > +    }
> > 
> > I prefer the API to reversed here - HwcComposer grabs the VsyncDispatcher
> > and notifies it directly.
> > 
> 
> If HwcComposer call VsyncDispatcherHost directly, it should include the
> VsyncDispatcherHost stuff.
> The HwcComposer use only one "NotifyVsync()" function, but it include whole
> VsyncDispatcherHost.
> Furthermore, HwcComposer might hold one referene count of VsyncDispatcher.
> We would like to control the 
> VsyncDispatcher life cycle within VsyncDispatcher module.
> 

That doesn't sound like a problem to me.

Comment 62

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8474562 [details] [diff] [review]
Part2: vsync protocol and dispatch framework. v7

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

This looks like a good start! I'm minusing for now because I'm having a very hard time trying to figure out which thread everything is supposed to run on. There's also a bunch of stuff that is unimplemented here, and I'd like to see the protocol not use sync messages on app startup. But it looks good overall!

::: dom/ipc/ContentParent.cpp
@@ +1929,5 @@
>              MOZ_ASSERT(opened);
> +
> +            if (gfxPrefs::FrameUniformityEnabled()) {
> +                DebugOnly<bool> vsyncOpened = PVsyncEvent::Open(this);
> +                MOZ_ASSERT(vsyncOpened);

This can be simplified as:

  MOZ_ALWAYS_TRUE(PVsyncEvent::Open(this));

::: gfx/layers/ipc/PVsyncEvent.ipdl
@@ +11,5 @@
> +
> +struct VsyncData
> +{
> +  // The vsync event timetamp in microsecond.
> +  int64_t timeStampUS;

Why is this signed?

@@ +13,5 @@
> +{
> +  // The vsync event timetamp in microsecond.
> +  int64_t timeStampUS;
> +  // The monotonic increased frame count for each vsync update.
> +  uint32_t frameNumber;

I think this should be a 64bit number otherwise you risk overflow.

@@ +32,5 @@
> +  async RegisterVsyncEvent();
> +  async UnregisterVsyncEvent();
> +
> +  // Get the vsync event rate
> +  sync GetVsyncRate() returns (uint32_t vsyncRate);

Let's try to figure out a way to not have this be synchronous.

::: gfx/layers/ipc/VsyncEventChild.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/VsyncDispatcherClient.h"
> +#include "mozilla/layers/VsyncEventChild.h"

Please make sure your class' header (VsyncEventChild.h) is always first, otherwise it may contain stuff that won't compile by itself.

@@ +32,5 @@
> +  vsyncEventChild->Open(aTransport, aOtherProcess, XRE_GetIOMessageLoop(), ChildSide);
> +
> +  // Get vsync rate from parent side.
> +  uint32_t vsyncRate = 0;
> +  vsyncEventChild->SendGetVsyncRate(&vsyncRate);

Doing a blocking sync call on the startup path is pretty lame and we've been trying to remove existing ones as fast as possible. Please see if you can't do this asynchronously somehow.

::: gfx/layers/ipc/VsyncEventChild.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MOZILLA_GFX_VSYNCEVENTCHILD_H

Nit: Your include guards are different here than in VsyncEventParent.h, I'd make them similar.

@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +class VsyncEventChild MOZ_FINAL : public PVsyncEventChild

Please make sure you run all the gfx/layers/ipc changes by someone like bjacob/nical since they have been doing a ton of work there.

@@ +19,5 @@
> +public:
> +  static PVsyncEventChild* Create(Transport* aTransport,
> +                                  ProcessId aOtherProcess);
> +
> +  VsyncEventChild(Transport* aTransport);

Ctor should be private.

::: gfx/layers/ipc/VsyncEventParent.cpp
@@ +28,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  ProcessHandle processHandle;
> +  if (!OpenProcessHandle(aOtherProcess, &processHandle)) {

You'll need to close this process handle eventually... ContentParent and BackgroundParent both do, Compositor probably leaks :(

@@ +35,5 @@
> +
> +  VsyncEventParent* vsyncParent = nullptr;
> +
> +  vsyncParent = new VsyncEventParent(aTransport);
> +  if (vsyncParent) {

'new' is infallible, no need to check for null here.

::: gfx/layers/ipc/VsyncEventParent.h
@@ +16,5 @@
> +{
> +public:
> +  static PVsyncEventParent* Create(Transport* aTransport, ProcessId aOtherProcess);
> +
> +  VsyncEventParent(Transport* aTransport);

Ctor should be private.

::: widget/shared/VsyncDispatcherClient.cpp
@@ +115,5 @@
> +VsyncDispatcherClient::EnableVsyncEvent(bool aEnable)
> +{
> +  if (mVsyncEventChild) {
> +    if (aEnable) {
> +      mVsyncEventChild->SendRegisterVsyncEvent();

You probably want to guard against registering or unregistering more than once?

::: widget/shared/VsyncDispatcherHelper.h
@@ +23,5 @@
> +public:
> +  template <typename DispatcherType, typename Type>
> +  static void Add(DispatcherType* aDispatcher, nsTArray<Type*>* aList, Type* aItem)
> +  {
> +    if (!aList->Contains(aItem)) {

Seems smarter to assert this. Otherwise you could call Add twice and then a single Remove call would remove it from the list.

@@ +37,5 @@
> +    typedef nsTArray<Type*> ArrayType;
> +    typename ArrayType::index_type index = aList->IndexOf(aItem);
> +
> +    if (index != ArrayType::NoIndex) {
> +      aList->RemoveElementAt(index);

Just call RemoveElement instead of IndexOf followed by RemoveElementAt.

@@ +74,5 @@
> +                              nsTArray<Type*>* aList,
> +                              Type* aItem)
> +  {
> +    // Deadlock.
> +    MOZ_ASSERT(!aDispatcher->IsInVsyncDispatcherHostThread());

I assume this means you expect to be on the main thread? Please don't block the main thread...

@@ +131,5 @@
> +    MonitorAutoLock lock(*aMonitor);
> +
> +    Add(aDispatcher, aList, aItem);
> +
> +    *aDone = true;

This will corrupt the stack or crash if your lock timed out...

::: widget/shared/VsyncDispatcherHost.cpp
@@ +29,5 @@
> +    // Because we use NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION,
> +    // the first call of GetInstance() should at main thread.
> +#ifdef MOZ_WIDGET_GONK
> +    mVsyncDispatcherHost = new GonkVsyncDispatcher();
> +    MOZ_RELEASE_ASSERT(mVsyncDispatcherHost, "Create GonkVD failed.");

new is infallible.

@@ +31,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    mVsyncDispatcherHost = new GonkVsyncDispatcher();
> +    MOZ_RELEASE_ASSERT(mVsyncDispatcherHost, "Create GonkVD failed.");
> +#endif
> +    //TODO: put other platform's VsyncDispatcher here

NS_WARNING here? MOZ_CRASH maybe?

@@ +55,5 @@
> +{
> +  MOZ_ASSERT(!mVsyncDispatchHostThread, "VDHost thread already existed.");
> +
> +  mVsyncDispatchHostThread = new base::Thread("Vsync dispatch thread");
> +  MOZ_RELEASE_ASSERT(mVsyncDispatchHostThread, "New VDHost thread failed.");

new is infallible

@@ +102,5 @@
> +  // Release the VsyncDispatcher singleton.
> +  mVsyncDispatcherHost = nullptr;
> +}
> +
> +VsyncDispatcherHost::VsyncDispatcherHost()

It's actually really hard to follow the code in this file because there are not enough thread assertions. Please try to go through and mark *every* function in this and other files with assertions about which thread they should be called on (IsInVsyncDispatcherHostThread, NS_IsMainThread, etc.).

@@ +132,5 @@
> +VsyncDispatcherHost::NotifyVsyncTask(int64_t aTimestampUS)
> +{
> +  // We propose a monotonic increased frame number here.
> +  // It helps us to identify the frame count for each vsync update.
> +  static uint32_t frameNumber = 0;

Shouldn't this be uint64_t instead? 32bit can wrap around...

::: widget/shared/VsyncDispatcherHost.h
@@ +36,5 @@
> +{
> +  friend class layers::VsyncEventParent;
> +  friend class ObserverListHelper;
> +
> +  // We would like to create and delete the VsyncEventChild at main thread.

Copy/paste error?

@@ +46,5 @@
> +  void StartUp();
> +  void ShutDown();
> +
> +  // All vsync observers should call sync unregister call before they
> +  // call destructor.

This comment doesn't belong here I don't think.

@@ +77,5 @@
> +
> +  bool IsInVsyncDispatcherHostThread();
> +
> +protected:
> +  uint32_t mVsyncRate;

You should keep all your members together, even if you need to make some private and some protected.

@@ +87,5 @@
> +  void CreateVsyncDispatchThread();
> +
> +  // Startup/shutdown the platform dependent vsync event generator.
> +  virtual void StartUpVsyncEvent() = 0;
> +  virtual void ShutDownVsyncEvent() = 0;

Where are the implementations of these functions?

@@ +121,5 @@
> +  // notification.
> +  void EnableVsyncNotificationIfhasObserver();
> +
> +private:
> +  static scoped_refptr<VsyncDispatcherHost> mVsyncDispatcherHost;

Statics should be 's'-prefixed. And you should use StaticRefPtr.

Comment 63

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8474565 [details] [diff] [review]
Part3: implement GonkVsyncDispatcher. v6

review+, if the discussion point with muw is addressed.

Comment 64

[u459114]

Jerry, base on your current design, I upload a new image onto wiki to reveal object relation now.

Comment 65

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8474562 [details] [diff] [review]
Part2: vsync protocol and dispatch framework. v7

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

I will revise the patch for these comment.
Thanks!

::: gfx/layers/ipc/PVsyncEvent.ipdl
@@ +11,5 @@
> +
> +struct VsyncData
> +{
> +  // The vsync event timetamp in microsecond.
> +  int64_t timeStampUS;

I use base::TimeTicks::HighResNow().ToInternalValue() to get time stamp.
It returns int64_t.

@@ +13,5 @@
> +{
> +  // The vsync event timetamp in microsecond.
> +  int64_t timeStampUS;
> +  // The monotonic increased frame count for each vsync update.
> +  uint32_t frameNumber;

At first, I would like to save the ipc message size.
But I think I can chage to use int64. The performance is almost the same.

@@ +32,5 @@
> +  async RegisterVsyncEvent();
> +  async UnregisterVsyncEvent();
> +
> +  // Get the vsync event rate
> +  sync GetVsyncRate() returns (uint32_t vsyncRate);

I will try to remove the sync call here. But this sync call is used only once. We use this function only at init step.

::: gfx/layers/ipc/VsyncEventChild.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/VsyncDispatcherClient.h"
> +#include "mozilla/layers/VsyncEventChild.h"

checked.

::: gfx/layers/ipc/VsyncEventChild.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MOZILLA_GFX_VSYNCEVENTCHILD_H

Thanks, sync all header file style

::: gfx/layers/ipc/VsyncEventParent.cpp
@@ +28,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  ProcessHandle processHandle;
> +  if (!OpenProcessHandle(aOtherProcess, &processHandle)) {

I will close the ProcessHandle at Dtor.
In this block, if OpenProcessHandle failed, I think I need to delete the Transport object here.

@@ +35,5 @@
> +
> +  VsyncEventParent* vsyncParent = nullptr;
> +
> +  vsyncParent = new VsyncEventParent(aTransport);
> +  if (vsyncParent) {

remove all check for new op. thx!

::: widget/shared/VsyncDispatcherClient.cpp
@@ +115,5 @@
> +VsyncDispatcherClient::EnableVsyncEvent(bool aEnable)
> +{
> +  if (mVsyncEventChild) {
> +    if (aEnable) {
> +      mVsyncEventChild->SendRegisterVsyncEvent();

Yes, we check at EnableVsyncNotificationIfhasObserver().
We will not register again if we had registered.

::: widget/shared/VsyncDispatcherHelper.h
@@ +23,5 @@
> +public:
> +  template <typename DispatcherType, typename Type>
> +  static void Add(DispatcherType* aDispatcher, nsTArray<Type*>* aList, Type* aItem)
> +  {
> +    if (!aList->Contains(aItem)) {

This is a good hint.
But in current usage, some module will call add more than once.

For refresh driver, after register, it will receive the vsync event only once.
If it needs vsync event, it need to register again.
So, in some situation, the refresh driver will call add twice or more.

@@ +37,5 @@
> +    typedef nsTArray<Type*> ArrayType;
> +    typename ArrayType::index_type index = aList->IndexOf(aItem);
> +
> +    if (index != ArrayType::NoIndex) {
> +      aList->RemoveElementAt(index);

Chage to:
if(aList->Contains(aItem)){
  aList->RemoveElement(aItem);
}

@@ +74,5 @@
> +                              nsTArray<Type*>* aList,
> +                              Type* aItem)
> +  {
> +    // Deadlock.
> +    MOZ_ASSERT(!aDispatcher->IsInVsyncDispatcherHostThread());

The sync call will only used at shutdown case.
Yes, we will block the main thread here. But we use it at shutdown.
Is it ok for shutdown case?

@@ +131,5 @@
> +    MonitorAutoLock lock(*aMonitor);
> +
> +    Add(aDispatcher, aList, aItem);
> +
> +    *aDone = true;

Thanks.
I make a big mistake here.

::: widget/shared/VsyncDispatcherHost.cpp
@@ +31,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    mVsyncDispatcherHost = new GonkVsyncDispatcher();
> +    MOZ_RELEASE_ASSERT(mVsyncDispatcherHost, "Create GonkVD failed.");
> +#endif
> +    //TODO: put other platform's VsyncDispatcher here

good hint here if we don't have the implementation now.

@@ +102,5 @@
> +  // Release the VsyncDispatcher singleton.
> +  mVsyncDispatcherHost = nullptr;
> +}
> +
> +VsyncDispatcherHost::VsyncDispatcherHost()

We have a wiki to show the desiogn.
https://wiki.mozilla.org/Project_Silk

I will add the thread checking for all function.
Thanks.

::: widget/shared/VsyncDispatcherHost.h
@@ +36,5 @@
> +{
> +  friend class layers::VsyncEventParent;
> +  friend class ObserverListHelper;
> +
> +  // We would like to create and delete the VsyncEventChild at main thread.

checked

@@ +77,5 @@
> +
> +  bool IsInVsyncDispatcherHostThread();
> +
> +protected:
> +  uint32_t mVsyncRate;

checked

@@ +87,5 @@
> +  void CreateVsyncDispatchThread();
> +
> +  // Startup/shutdown the platform dependent vsync event generator.
> +  virtual void StartUpVsyncEvent() = 0;
> +  virtual void ShutDownVsyncEvent() = 0;

GonkVsyncDispatcher.cpp in part3 patch.

@@ +121,5 @@
> +  // notification.
> +  void EnableVsyncNotificationIfhasObserver();
> +
> +private:
> +  static scoped_refptr<VsyncDispatcherHost> mVsyncDispatcherHost;

checked

Comment 66

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync protocol and dispatch framework. v8

Split VsyncDispatcher into several small module.

Comment 67

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: implement platform vsync timer. v1

Remove the GonkVsyncDispatcher.
Instead, use a small scope vsync platform timer.
VsyncDispatcher can be used for all platform now.
We just need to implement the specific platform vsync timer.

Comment 68

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8483483 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v8

This patch haven't move the input resampling from hwc to VD. I will update this part soon.

Comment 69

[u459114]

Comment on attachment 8483483 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v8

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

::: widget/shared/VsyncPlatformTimer.h
@@ +27,5 @@
> +// Please check:
> +//   widget/android/VsyncPlatformTimerImpl.h
> +//   widget/gonk/VsyncPlatformTimerImpl.h
> +//   widget/windows/VsyncPlatformTimerImpl.h
> +class VsyncPlatformTimer

Suggest rename to PlatformVsyncTimer

@@ +33,5 @@
> +public:
> +  virtual ~VsyncPlatformTimer() { }
> +
> +  // We should register the callback before we call startup
> +  virtual void SetObserver(VsyncTimerObserver* aObserver) = 0;

Don't we need paired RemoveObserver function?

@@ +38,5 @@
> +
> +  // Startup/shutdown the timer. We should only call startup/shutdown once.
> +  virtual void Startup() = 0;
> +  virtual void Shutdown() = 0;
> +

Why clients need to call these two functions? Can PlatfromVsyncTimer call these two functions, internally, while Enable been called?

@@ +43,5 @@
> +  // Enable/disable the vsync event.
> +  virtual void Enable(bool aEnable) = 0;
> +
> +  // Query the vsync timer rate per second. It should be called after startup.
> +  virtual uint32_t GetVsyncRate() = 0;

It should be called after startup << what's the return value if user doesn't obey this rule?

@@ +49,5 @@
> +
> +class VsyncPlatformTimerFactory
> +{
> +public:
> +  static VsyncPlatformTimer* GetTimer();

suggestion
1. Make VsyncPlatformTimer refbase 
2. rename GetTimer to Create 
3. Function prototype: static already_AddRef<VsyncPlatformTimer> Create();

Comment 70

[u459114]

Comment on attachment 8483483 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v8

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

::: widget/shared/VsyncDispatcher.h
@@ +62,5 @@
> +// VsyncDispatcher can dispatch vsync event to all registered observer.
> +class VsyncDispatcher
> +{
> +public:
> +  static VsyncDispatcher* GetInstance();

Keep using singleton pattern before we find out a better host object to own VsyncDispatcher object.
After that, we may change this function to 
static alread_AddRef<VsyncDispatcher> Create();

@@ +84,5 @@
> +  virtual ~VsyncDispatcher() { }
> +};
> +
> +// The VDClient for content process
> +class VsyncDispatcherClient : public VsyncDispatcher

I dont think VsyncDispatcherClient need to be inherited from VsyncDispatcher
Make it a pure abstract interface, remove destructor here.

At VsyncDispatcherClientImp
class VsyncDispatcherClientImp: public VsyncDispatcher, public VsyncDispatcherClient.

The same opinion for VsyncDispatcherHost

::: widget/shared/VsyncPlatformTimer.cpp
@@ +21,5 @@
> +  return new GonkVsyncTimer;
> +#endif
> +
> +  // Just crash if here is no matched timer.
> +  MOZ_CRASH("No vsync timer implementation.");

Return fallback timer instead - SW Timer

Comment 71

[u459114]

Comment on attachment 8483486 [details] [diff] [review]
Part3: implement platform vsync timer. v1

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

::: widget/gonk/GonkVsyncTimer.cpp
@@ +63,5 @@
> +  // Fallback to software vsync event
> +  if (!mUseHWVsync) {
> +    //TODO: init software vsync event here.
> +    LOGI("GonkVsyncTimer: use soft vsync");
> +  }

I see, you construct fallback chain here.
My suggestion is
Construct fallback chain in VyncDispatcherHostImp.
In VyncDispatcherHostImp:
  if (FAILED(mTimer->StartUp()) {
    mTimer = CreateSoftwareVsyncTimer();
  }

This change make logic here and, all platform vsync timers, pretty simple. 
mInited == true only if HwcComposer2D::GetInstance()->InitHwcEventCallback() return true.
And, you can remove mUseHWVsync member

::: widget/gonk/GonkVsyncTimer.h
@@ +11,5 @@
> +#include "VsyncPlatformTimer.h"
> +
> +namespace mozilla {
> +
> +class GonkVsyncTimer : public VsyncPlatformTimer

class GonkVsyncTimer MOZ_FINAL

Comment 72

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8472220 [details] [diff] [review]
Part1: silk preference. v4

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

::: gfx/thebes/gfxPrefs.h
@@ +195,5 @@
>  
>    DECL_GFX_PREF(Live, "gfx.draw-color-bars",                   CompositorDrawColorBars, bool, false);
>  
> +  // Do tick and compose aligned with vsync.
> +  DECL_GFX_PREF(Once, "gfx.frameuniformity",                   FrameUniformityEnabled, bool, false);

I think (for better or worse), a more consistent name would be "gfx.frameuniformity.enabled".

@@ +197,5 @@
>  
> +  // Do tick and compose aligned with vsync.
> +  DECL_GFX_PREF(Once, "gfx.frameuniformity",                   FrameUniformityEnabled, bool, false);
> +  // Generate vsync event by hardware
> +  DECL_GFX_PREF(Once, "gfx.frameuniformity.hw-vsync",          FrameUniformityHWVsyncEnabled, bool, false);

This one already landed in bug 987527.

Comment 73

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: silk preference. v5

update for comment 72.

Comment 74

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: vsync protocol and dispatch framework. v9

update for comment 69 to 71.

Comment 75

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: implement platform vsync timer. v2

Comment 76

[Peter Chang[:pchang]]

Comment on attachment 8484155 [details] [diff] [review]
[WIP] Part2: vsync protocol and dispatch framework. v9

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

::: gfx/thebes/gfxPlatform.cpp
@@ +362,5 @@
>  #ifdef DEBUG
>      mozilla::gl::GLContext::StaticInit();
>  #endif
>  
> +#ifdef MOZ_WIDGET_GONK

Please add comment here.

::: widget/shared/PlatformVsyncTimer.cpp
@@ +24,5 @@
> +#endif
> +
> +  if (timer) {
> +    if (!timer->Startup()) {
> +      // Startup timer failed.

//failed to start up timer

@@ +38,5 @@
> +PlatformVsyncTimerFactory::CreateSWTimer(VsyncTimerObserver* aObserver)
> +{
> +  PlatformVsyncTimer* timer = nullptr;
> +
> +  // Create the sw vsync timer here.

redundant comment.

@@ +61,5 @@
> +  if (!timer){
> +    timer = CreateSWTimer(aObserver);
> +  }
> +
> +  // Just assert if here is no matched timer.

//Assert here if no timer is available.

::: widget/shared/PlatformVsyncTimer.h
@@ +23,5 @@
> +  static PlatformVsyncTimer* CreateSWTimer(VsyncTimerObserver* aObserver);
> +};
> +
> +// This is the base class which will be notified for every vsync event.
> +class VsyncTimerObserver

Can we just call VsyncObserver?

@@ +38,5 @@
> +// Platform dependent vsync timer. It will call the VsyncTimerCallback when the
> +// vsync event comes.
> +// We need to call Enable(true) to start the timer at first.
> +// The implementation is at the platform specific folder.
> +// Please check:

// A cross-platform vsync timer that provides the VsyncTimerCallback
// to notify vsync events

@@ +42,5 @@
> +// Please check:
> +//   widget/android/*VsyncTimer.h
> +//   widget/gonk/*VsyncTimer.h
> +//   widget/windows/*VsyncTimer.h
> +class PlatformVsyncTimer

Can we just call PlatformVsync?

@@ +55,5 @@
> +  }
> +
> +  virtual ~PlatformVsyncTimer() { }
> +
> +  // Shutdown the timer. After shutdown, the Enable() function is invalid.

move 'Enable() function is invalid' to below.

@@ +58,5 @@
> +
> +  // Shutdown the timer. After shutdown, the Enable() function is invalid.
> +  virtual void Shutdown() = 0;
> +
> +  // Enable/disable the vsync event.

// Enable/disable the vsync timer. It becomes no functionality after shutdown.

@@ +62,5 @@
> +  // Enable/disable the vsync event.
> +  virtual void Enable(bool aEnable) = 0;
> +
> +  // Query the vsync timer rate per second. It will return 0 if the timer is
> +  // invalid.

Query the vsync timer rate per second. Return 0 if fails to get the vsync timer reate

@@ +67,5 @@
> +  virtual uint32_t GetVsyncRate() = 0;
> +
> +private:
> +  // Startup the timer. It will return false if we can't init the timer.
> +  virtual bool Startup() = 0;

// Return true if the timer was initialized

::: widget/shared/VsyncDispatcher.h
@@ +21,5 @@
> +
> +class VsyncDispatcherClient;
> +class VsyncDispatcherHost;
> +
> +// Every vsync event observer should inherit this base class.

You should describe the purpose of this class.

@@ +58,5 @@
> +  typedef nsTArray<VsyncObserver*> ObserverList;
> +  ObserverList mObserverListList;
> +};
> +
> +// VsyncDispatcher can dispatch vsync event to all registered observer.

// VsyncDispatcher is used to dispatch vsync events to the registered observers.

@@ +93,5 @@
> +
> +  // Set vsync event IPC child.
> +  virtual void SetVsyncEventChild(layers::VsyncEventChild* aVsyncEventChild) = 0;
> +
> +  // Update the vsync rate getting from VDHost.

Add comment why we need to change the vsnc reate

::: widget/shared/VsyncDispatcherClientImpl.cpp
@@ +55,5 @@
> +
> +void
> +RefreshDriverRegistryClient::Register(VsyncObserver* aVsyncObserver)
> +{
> +  MOZ_ASSERT(mVsyncDispatcher->IsInVsyncDispatcherThread());

why do we check thread here? Does it mean the add/remove is not thread-safe?

@@ +73,5 @@
> +
> +void
> +RefreshDriverRegistryClient::Dispatch(int64_t aTimestampUS, uint64_t aFrameNumber)
> +{
> +  MOZ_ASSERT(mVsyncDispatcher->IsInVsyncDispatcherThread());

tick the refresh driver will check the VD thread.

@@ +257,5 @@
> +{
> +  MOZ_ASSERT(mInited);
> +  MOZ_ASSERT(IsInVsyncDispatcherThread());
> +
> +  mVsyncRate = aVsyncRate;

I didn't why the purpose for SetVsyncRate. It just changed the mVsyncRate.

::: widget/shared/VsyncDispatcherHostImpl.cpp
@@ +17,5 @@
> +using namespace layers;
> +
> +StaticRefPtr<VsyncDispatcherHostImpl> VsyncDispatcherHostImpl::sVsyncDispatcherHost;
> +
> +class VsyncEventRegistryHost : public VsyncEventRegistry

why do we need the RegistryHost? Why we can't share functions with VsyncEventRegistery or RefreshDriverRegistryHost or VsyncEventRegistryHost?

@@ +100,5 @@
> +
> +void
> +VsyncEventRegistryHost::EnableVsyncNotificationIfhasObserver()
> +{
> +  MOZ_ASSERT(IsInVsyncDispatcherThread());

since you already check the VDthread inside VsyncDispatcher, I think you don't need to check again here.

@@ +195,5 @@
> +void
> +VsyncDispatcherHostImpl::Startup()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "VDHost StartUp() should in main thread.");
> +  MOZ_ASSERT(!mInited, "VDHost is already initialized.");

Why we need to assert the mInited? You can just return.

@@ +374,5 @@
> +  //TODO: notify chrome to dispatch input
> +}
> +
> +void
> +VsyncDispatcherHostImpl::Compose()

I think here you only dispatch to vsync to compositor not try to compose. Am I right?
If yes, it is not proper to call Compose()

::: widget/shared/moz.build
@@ +29,5 @@
> +  LOCAL_INCLUDES += [
> +      '/widget/gonk',
> +  ]
> +
> +# VsyncEvent ipc

unnecessary comment?

Comment 77

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: vsync protocol and dispatch framework. v10

add input registry

Comment 78

[Mason Chang [Inactive] [:mchang]]

Attachment: Composites Aligned with Vsync

Let's see if we can land a simple Vsync Framework that just notifies the compositor on vsync. We have a lot of good performance data in bug 1077651.

The general architecture is slightly different than what we have in the docs.

1) We notify the compositor on the android hardware vsync thread. We do this because the Compositor is latency sensitive and we already know there is a delay going from android hardware vsync thread -> VsyncDispatcher thread -> Compositor Thread. (bug 991420).
2) The compositor subscribes to the VsyncDispatcher and gets every vsync event. It doesn't have to keep registering to listen to vsync events. The compositor unsubscribes to vsync notifications when it receives 5 vsync events and doesn't have to composite for 5 vsync events in a row.

Comment 79

[Mason Chang [Inactive] [:mchang]]

Attachment: Enable Hardware Composer to Query Vsync Rate

Mostly the same as Part 3 patch previously posted. Enables the hardware composer to query the device for the vsync rate.

Comment 80

[Mason Chang [Inactive] [:mchang]]

Attachment: Simple Vsync Framework

Simple Vsync Framework that does everything on the android input thread. It notifies touch events and composites on vsync events. I think it would be nice to slim down the framework if we can. This version supports the touch events and composites, we just have to add the refresh driver.

Comment 81

[u459114]

Comment on attachment 8502136 [details] [diff] [review]
Composites Aligned with Vsync

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +421,5 @@
>  }
>  
>  void
>  CompositorParent::ActorDestroy(ActorDestroyReason why)
>  {

if (gfxPrefs::VsyncAlignedCompositor()) {

@@ +427,5 @@
> +  mScheduledVsyncComposite = false;
> +  mSkippedVsyncComposite = false;
> +
> +  VsyncDispatcher::GetInstance()->RemoveCompositorVsyncObserver(this);
> +

}

@@ +511,5 @@
>  CompositorParent::CancelCurrentCompositeTask()
>  {
> +  if (mScheduledVsyncComposite) {
> +    mScheduledVsyncComposite = false;
> +  }

if (gfxPrefs::VsyncAlignedCompositor()) {
  mScheduledVsyncComposite = false;
}

@@ +623,5 @@
> +CompositorParent::NotifyVsync(TimeStamp aVsyncTimestamp)
> +{
> +  // Should be the android hardware vsync thread
> +  MOZ_ASSERT(!IsInCompositorThread());
> +

MOZ_ASSERT(gfxPrefs::VsyncAlignedCompositor());

@@ +641,3 @@
>  void
> +CompositorParent::ScheduleVsyncAlignedComposition()
> +{

MOZ_ASSERT(gfxPrefs::VsyncAlignedCompositor());

@@ +655,5 @@
> +   * and hope we finish before the next vsync
> +   */
> +  if (mSkippedVsyncComposite) {
> +    TimeDuration duration = TimeStamp::Now() - mLastVsyncTimestamp;
> +    if (duration.ToMilliseconds() < 3.0 && mVsyncEvents == 0) {

why 3.0?

@@ +656,5 @@
> +   */
> +  if (mSkippedVsyncComposite) {
> +    TimeDuration duration = TimeStamp::Now() - mLastVsyncTimestamp;
> +    if (duration.ToMilliseconds() < 3.0 && mVsyncEvents == 0) {
> +      mVsyncEvents++;

How about move #648 here? Set mScheduledVsyncComposite as true right ahead of posting CompositeCallback

@@ +660,5 @@
> +      mVsyncEvents++;
> +      mCurrentCompositeTask = NewRunnableMethod(this,
> +                                  &CompositorParent::CompositeCallback,
> +                                  TimeStamp::Now());
> +      ScheduleTask(mCurrentCompositeTask, 0);

since you already schedule a composition here, why not just return and exit this function?

@@ +662,5 @@
> +                                  &CompositorParent::CompositeCallback,
> +                                  TimeStamp::Now());
> +      ScheduleTask(mCurrentCompositeTask, 0);
> +    }
> +  }

Mason, is there any benchmark tell us that we can really get benefit from this if statement(#657)?

@@ +678,1 @@
>  {

MOZ_ASSERT(!gfxPrefs::VsyncAlignedCompositor());

@@ +738,5 @@
> +
> +    if (mSkippedVsyncComposite) {
> +      mSkippedVsyncCount++;
> +      return;
> +    }

move #745 here
else {
  mSkippedVsyncCount = 0;
}

@@ +765,5 @@
>                    scheduleDelta.ToMilliseconds());
>    }
>  #endif
>  
> +  mScheduledVsyncComposite = false;

if (gfxPrefs::VsyncAlignedCompositor()) {
  mScheduledVsyncComposite = false;
}

@@ +941,5 @@
>      // values to avoid race conditions when calling GetAnimationTransform etc.
>      // (since the above SetShadowProperties will remove animation effects).
>      // However, we only do this update when a composite operation is already
>      // scheduled in order to better match the behavior under regular sampling
>      // conditions.

Try to access silk context only if silk is enable.

bool doComposite = gfxPrefs::VsyncAlignedCompositor() ?
  (mIsTesting && root && (mCurrentCompositeTask || mScheduledVsyncComposite)) :
  (mIsTesting && root && mCurrentCompositeTask);

if (doComposite) {

@@ +974,5 @@
>    mIsTesting = true;
>    mTestTime = aTime;
>  
>    // Update but only if we were already scheduled to animate
> +  if (mCompositionManager && (mCurrentCompositeTask || mScheduledVsyncComposite)) {

See previous one

Comment 82

[u459114]

Comment on attachment 8502138 [details] [diff] [review]
Simple Vsync Framework

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

Even if we only want to align composition with vsync dispatcher, I still suggest we put implementation in VsyncDispacherHostImp.
By the change here, all unit tests need to rewrite again after we bring refresh driver in.

Comment 83

[u459114]

Comment on attachment 8502137 [details] [diff] [review]
Enable Hardware Composer to Query Vsync Rate

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

I do not really familiar with HwcComposer2D internal behavior. Peter, please help to review this one

Comment 84

[u459114]

Test case:
https://github.com/CJKu/gecko-dev/blob/cj-silk-test/widget/shared/tests/gtest/TestVsyncDispatcher.cpp

Comment 85

[u459114]

Test target - VsyncDispatcherHostImpl

Jerry:
To test VsyncDispatcherHostImpl, I need to surround this object by mock vsync observer and platform timer.
1. PlatformVsyncTimerFactory::SetCustomizedCreator API request. [1]
With this API, I can replace platform timer by a mock one.
2. VsyncDispatcherHostImpl creation request [2]
I need to create one clear VsyncDispatcherHostImpl for each single test case, instead of reuse one single VsyncDispatcherHostImpl for all test cases


[1] https://github.com/CJKu/gecko-dev/blob/cj-silk-test/widget/shared/tests/gtest/TestVsyncDispatcher.cpp#L138
[2] https://github.com/CJKu/gecko-dev/blob/cj-silk-test/widget/shared/tests/gtest/TestVsyncDispatcher.cpp#L142

Comment 86

[Peter Chang[:pchang]]

Comment on attachment 8502137 [details] [diff] [review]
Enable Hardware Composer to Query Vsync Rate

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

::: widget/gonk/HwcComposer2D.cpp
@@ +227,5 @@
> +    const int QUERY_ATTRIBUTE_NUM = 1;
> +    const uint32_t HWC_ATTRIBUTES[QUERY_ATTRIBUTE_NUM+1] = {
> +        HWC_DISPLAY_VSYNC_PERIOD,
> +        HWC_DISPLAY_NO_ATTRIBUTE
> +    };

Suggest not to hard code the array size here, please use sizeof(HWC_ATTRIBUTES) /
sizeof(HWC_ATTRIBUTES)[0]

Comment 87

[Mason Chang [Inactive] [:mchang]]

(In reply to C.J. Ku[:cjku] from comment #82)
> Comment on attachment 8502138 [details] [diff] [review]
> Simple Vsync Framework
> 
> Review of attachment 8502138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Even if we only want to align composition with vsync dispatcher, I still
> suggest we put implementation in VsyncDispacherHostImp.
> By the change here, all unit tests need to rewrite again after we bring
> refresh driver in.

Hi CJ, where / what unit tests do we have at the moment? I know you're writing some on gtest, but just wondering, why gtest? If we're targeting b2g first and gtests aren't running on b2g, do we have plans to get gtest running soon? I'm ok rewriting the unit tests if we have to. Also, we should make sure the compositor and input work first and we need to have tests for those components first in my opinion. What do you think? Thanks!

I'll make the compositor changes as well. Thanks for the review!

Comment 88

[Mason Chang [Inactive] [:mchang]]

Attachment: Screenshot of Missed Composite

(In reply to C.J. Ku[:cjku] from comment #81)
> Comment on attachment 8502136 [details] [diff] [review]
> Composites Aligned with Vsync
> 
> @@ +655,5 @@
> > +   * and hope we finish before the next vsync
> > +   */
> > +  if (mSkippedVsyncComposite) {
> > +    TimeDuration duration = TimeStamp::Now() - mLastVsyncTimestamp;
> > +    if (duration.ToMilliseconds() < 3.0 && mVsyncEvents == 0) {
> 
> why 3.0?
> 
> @@ +656,5 @@
> > +   */
> > +  if (mSkippedVsyncComposite) {
> > +    TimeDuration duration = TimeStamp::Now() - mLastVsyncTimestamp;
> > +    if (duration.ToMilliseconds() < 3.0 && mVsyncEvents == 0) {
> > +      mVsyncEvents++;
> 
> How about move #648 here? Set mScheduledVsyncComposite as true right ahead
> of posting CompositeCallback
> 
> @@ +660,5 @@
> > +      mVsyncEvents++;
> > +      mCurrentCompositeTask = NewRunnableMethod(this,
> > +                                  &CompositorParent::CompositeCallback,
> > +                                  TimeStamp::Now());
> > +      ScheduleTask(mCurrentCompositeTask, 0);
> 
> since you already schedule a composition here, why not just return and exit
> this function?
> 
> @@ +662,5 @@
> > +                                  &CompositorParent::CompositeCallback,
> > +                                  TimeStamp::Now());
> > +      ScheduleTask(mCurrentCompositeTask, 0);
> > +    }
> > +  }
> 
> Mason, is there any benchmark tell us that we can really get benefit from
> this if statement(#657)?

I don't have any benchmarks, but I do see this in profiles a lot: https://bugzilla.mozilla.org/show_bug.cgi?id=1077651#c7 and the screenshot - Note that the vsync bar is wider than it needs to be for readability reasons. The vsync actually occurs at the start of the orange box, so the layer transaction finished a little after vsync. However, because the compositor already finished processing the vsync composite without a layer transaction, we don't composite. 

I don't really like hardcoding a specific 3 ms value either. But I do see that composites take between 3-5 ms usually when we're doing well. If we start to composite ~ 6-7 ms after vsync, it means we finish composting around 10-ms of the frame. At this point, there is a lock in the driver that takes 4-5 ms to grab, extending our composite times quite a bit. This occurs on master. See bug 1076166. All tests were done on a flame. So 3 ms seemed to be the sweet spot where we could still schedule a composite and finish before the next vsync somewhat reliably, composite most "around vsync layer transactions" and not hit the driver lock. If you can come up with something better than this hack, I'm more than happy to hear it!

Comment 89

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8502137 [details] [diff] [review]
Enable Hardware Composer to Query Vsync Rate

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

::: widget/gonk/HwcComposer2D.cpp
@@ +289,5 @@
>      }
>  }
> +
> +uint32_t HwcComposer2D::GetHWVsyncRate() const
> +{

add MOZ_ASSERT(mHasHWVsync)?

@@ +290,5 @@
>  }
> +
> +uint32_t HwcComposer2D::GetHWVsyncRate() const
> +{
> +    return mVsyncRate;

Set a default value for mVsyncRate.

Comment 90

[Mason Chang [Inactive] [:mchang]]

Attachment: Composites Aligned with Vsync

Composites that are aligned with vsync. Updated to include comments from CJ in comment 81. The biggest contention will be we hard code the number of vsyncs to listen to until we stop listening. The other is how to deal with layer transactions that come around vsync times. If we have a layer transaction that occurs right after vsync and we skipped a composite, we schedule a composite immediately. I'm not super happy with these solutions but registering to vsync every composite seems worse.

Comment 91

[Mason Chang [Inactive] [:mchang]]

Attachment: Simple Vsync Framework

Simple Vsync Framework. Uses one single timestamp everywhere now! Only mozilla::TimeStamp.

Comment 92

[Mason Chang [Inactive] [:mchang]]

(In reply to Mason Chang [:mchang] from comment #90)
> Created attachment 8504401 [details] [diff] [review]
> Composites Aligned with Vsync
> 
> Composites that are aligned with vsync. Updated to include comments from CJ
> in comment 81. The biggest contention will be we hard code the number of
> vsyncs to listen to until we stop listening. The other is how to deal with
> layer transactions that come around vsync times. If we have a layer
> transaction that occurs right after vsync and we skipped a composite, we
> schedule a composite immediately. I'm not super happy with these solutions
> but registering to vsync every composite seems worse.

Also, forgot to note that having the touch dispatch + composite race created very janky behavior. I modified the compositor to dispatch touch events AFTER we finish composites, which works out very well until we can get bug 930939 done.

Comment 93

[Mason Chang [Inactive] [:mchang]]

Attachment: HwcComposer can Query Vsync

Carrying r+ from peter. Not sure we need all the instrumentation to query Vsync rates yet. Where / what did we plan on using it for?

Comment 94

[Mason Chang [Inactive] [:mchang]]

Attachment: Composites Aligned with Vsync

Forgot to include the preference change.

Comment 95

[Avi Halachmi (:avih)]

Not sure if this should go here or on bug 987532 or elsewhere.

Does this system support a notion of "disabling vsync" such that consecutive frames could rendered as soon as possible?

In talos (our desktop performance testing framework), we use this mode extensively to measure throughput performance of various animations (scrolling, tab animations, etc), because without disabling vsync, many tests would just average at 16.7ms/frame, but when vsync is disabled we can measure reasonably well how fast can the pipeline really iterate.

This is similar to benchmarking 3D games/engines by disabling vsync and measuring the maximum frames throughput.

We call this "ASAP mode", we enter this mode by setting the pref layout.frame_rate to 0 (-1 is the default value for this pref which should result in proper vsync or 60hz if the frequency is unknown, and any other >0 value results in this value as the frequency).

ASAP mode currently works and is being used for our performance tests on Windows, Linux and OS X.

Comment 96

[Mason Chang [Inactive] [:mchang]]

(In reply to Avi Halachmi (:avih) from comment #95)
> Not sure if this should go here or on bug 987532 or elsewhere.
> 
> Does this system support a notion of "disabling vsync" such that consecutive
> frames could rendered as soon as possible?
> 
> In talos (our desktop performance testing framework), we use this mode
> extensively to measure throughput performance of various animations
> (scrolling, tab animations, etc), because without disabling vsync, many
> tests would just average at 16.7ms/frame, but when vsync is disabled we can
> measure reasonably well how fast can the pipeline really iterate.
> 
> This is similar to benchmarking 3D games/engines by disabling vsync and
> measuring the maximum frames throughput.
> 
> We call this "ASAP mode", we enter this mode by setting the pref
> layout.frame_rate to 0 (-1 is the default value for this pref which should
> result in proper vsync or 60hz if the frequency is unknown, and any other >0
> value results in this value as the frequency).
> 
> ASAP mode currently works and is being used for our performance tests on
> Windows, Linux and OS X.

Hey Avi. Yes, currently this is all behind a preference that will be off by default for a while. You can disable vsync and leave the ASAP mode enabled for talos performance tests.

Comment 97

[Avi Halachmi (:avih)]

I know that currently the new system is off by default and that ASAP mode works.

My question was if the new vsync system will support disabling vsync to get maximum rendering/iterations throughput, since we rely on this mode in many of our performance tests.

Comment 98

[Mason Chang [Inactive] [:mchang]]

(In reply to Avi Halachmi (:avih) from comment #97)
> I know that currently the new system is off by default and that ASAP mode
> works.
> 
> My question was if the new vsync system will support disabling vsync to get
> maximum rendering/iterations throughput, since we rely on this mode in many
> of our performance tests.

I assume that we can just disable the new system, which will also disable vsync.

Comment 99

[Avi Halachmi (:avih)]

(In reply to Mason Chang [:mchang] from comment #98)
> I assume that we can just disable the new system, which will also disable
> vsync.

It's not a trivial assumption IMO. Would appreciate if this issue is taken into account with further progress of this project, by validating that this assumption is kept true. Via a test maybe?

Comment 100

[Benoit Girard (:BenWa)]

Comment on attachment 8504428 [details] [diff] [review]
Composites Aligned with Vsync

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

Sorry if the comments are discoursing. It's a good patch but I think it's too complex for what we're implementing. I either want to see good supporting data for the heuristics or lets simplify it down quite a bit and land each heuristic in a follow with proper data.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +661,5 @@
> +   * and hope we finish before the next vsync
> +   */
> +  if (mSkippedVsyncComposite) {
> +    TimeDuration duration = TimeStamp::Now() - mLastVsyncTimestamp;
> +    if (duration.ToMilliseconds() < 3.0 && mVsyncEvents == 0) {

I see what you're trying to do and it's logical, but this type of magic number '3ms' is really fishy.

Do we have data that this is even desirable? I'd like to suggest that we land the simplest thing possible and then gather data on how to improve it. I'm suggesting that things schedule a composite should mark the compositor as dirty and when a vsync is notified that we composite. This isn't great for latency but it's great for simplicity, has no magic threshold and doesn't risk picking a threshold that will sometimes slip into the next interval.

Later we can investigate and validate our heuristics. If you already have supporting data for this then I'd be happy to look it over.

@@ +665,5 @@
> +    if (duration.ToMilliseconds() < 3.0 && mVsyncEvents == 0) {
> +      mVsyncEvents++;
> +      mCurrentCompositeTask = NewRunnableMethod(this,
> +                                  &CompositorParent::CompositeCallback,
> +                                  TimeStamp::Now());

I wouldn't like the idea of using TimeStamp::Now(). If we don't miss a frame we're going to show 2 frames that are 16ms apart but have their animation < 16 ms apart.

@@ +791,5 @@
>    }
>  #endif
>  
> +/*
> +  if (gfxprefs::VsyncAlignedComposite) {

this is commented out.

::: gfx/layers/ipc/CompositorParent.h
@@ +139,5 @@
>    virtual void GetAPZTestData(const LayerTransactionParent* aLayerTree,
>                                APZTestData* aOutData) MOZ_OVERRIDE;
>    virtual AsyncCompositionManager* GetCompositionManager(LayerTransactionParent* aLayerTree) MOZ_OVERRIDE { return mCompositionManager; }
>  
> +  virtual bool NotifyVsync(TimeStamp aVsyncTimestamp) MOZ_OVERRIDE;

I don't think this really belong in the public interface for CompositorParent. The vsync observer could probably live entirely in the implementation (CPP file).

But I guess this is fine.

@@ +165,5 @@
>     */
>    bool ScheduleResumeOnCompositorThread(int width, int height);
>  
>    virtual void ScheduleComposition();
> +  void ScheduleVsyncAlignedComposition();

Should these two not be private?

Vsync-ing should be an implementation detail of CompositorParent.

@@ +334,4 @@
>    bool mIsTesting;
> +
> +  // True if we've scheduled a composite aligned with vsync
> +  bool mScheduledVsyncComposite;

I don't like this variable. If we've scheduled a compsite it shouldn't need to matter if it's vsync or not. Also we should either have a vsync aligned compositor  with all composites either aligned or not. We should just have a non live preference that is read on startup. We can check the state of the preference instead and assume that all scheduled compositor match the preference. Eventually silk will be the default and this preference will be removed.

@@ +335,5 @@
> +
> +  // True if we've scheduled a composite aligned with vsync
> +  bool mScheduledVsyncComposite;
> +
> +  // True if this is observing vsyncs

When I first read this I assumed this should tracked if we we're initialized for vsync observing but really this is tracking if we're turned off the observer because there was no changes for the last x ticks.

This isn't great because we have duplicate state here that we need to keep synchronized. i.e. this bool + if this object is in the observer list.

@@ +339,5 @@
> +  // True if this is observing vsyncs
> +  bool mIsObservingVsync;
> +
> +  // True if we got a vsync event but didn't composite
> +  bool mSkippedVsyncComposite;

This needs to be better defined. Is this counting skipped composites because we we're too busy to do it in time, is it counting when we tried to composite but had no pending changes?

@@ +342,5 @@
> +  // True if we got a vsync event but didn't composite
> +  bool mSkippedVsyncComposite;
> +
> +  // Counter to check how many times we got a vsync event but didn't composite
> +  int32_t mSkippedVsyncCount;

You might want to clarify in the comment that this is used to unregister the vsync listener.

@@ +345,5 @@
> +  // Counter to check how many times we got a vsync event but didn't composite
> +  int32_t mSkippedVsyncCount;
> +
> +  // Counter for how many vsync composite events are on the message loop
> +  int32_t mVsyncEvents;

Do we ever want more than one queued? I belive right now with the compositor we have up to one only. We keep track of the task.

::: gfx/thebes/gfxPrefs.h
@@ +203,5 @@
>    DECL_GFX_PREF(Live, "gfx.draw-color-bars",                   CompositorDrawColorBars, bool, false);
>  
>    // Use vsync events generated by hardware
>    DECL_GFX_PREF(Once, "gfx.frameuniformity.hw-vsync",          FrameUniformityHWVsyncEnabled, bool, false);
> +  DECL_GFX_PREF(Once, "gfx.vsync.compositor",                  VsyncAlignedCompositor, bool, false);

IMO better preference names would be:
layers.vsync.enabled;true
gfx.vsync.enabled;true

A nit since compositor is more of a top level feature than vsync.

Comment 101

[Mason Chang [Inactive] [:mchang]]

Attachment: Composites Aligned with Vsync

From our conversation on irc. Created a new CompositorVsyncObserver that controls listening to vsync. If we get a vsync event without a schedule composite, we stop listening to vsync. Has none of the performance heuristics. Here's a profile: https://people.mozilla.org/~bgirard/cleopatra/#report=06b6dae75192b149769d598d5d62d86a9726e157

The weird thing is that we sometimes miss 2 frames, not sure why. Still have to investigate (might be another performance heuristic) unless you know off the top of your head. The other issue is that since we do some locking of the vsync observers in the VsyncFramework, I made sure we always register/unregister the CompositorVsyncObserver on the Compositor thread.

Comment 102

[Mason Chang [Inactive] [:mchang]]

Attachment: Hardware Composer Modifications to Notify VsyncFramework

Comment 103

[Mason Chang [Inactive] [:mchang]]

Attachment: Simple Vsync Framework

Simple Vsync Framework. Small nits. Overall architecture is HwcComposer->VsyncFramework->Compositor, all on the android input thread.

Comment 104

[Benoit Girard (:BenWa)]

Comment on attachment 8505141 [details] [diff] [review]
Composites Aligned with Vsync

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +216,5 @@
> +CompositorVsyncObserver::ScheduleComposite(bool aSchedule)
> +{
> +  MOZ_ASSERT(IsInCompositorThread());
> +  MonitorAutoLock lock(mScheduledCompositeMonitor);
> +  mScheduledComposite = aSchedule;

nit: This doesn't schedule something (post a message to a loop). IMO a better name is mNeedsRecomposite.

@@ +227,5 @@
> +bool
> +CompositorVsyncObserver::NotifyVsync(TimeStamp aVsyncTimestamp)
> +{
> +  // Called from the vsync dispatch thread
> +  MOZ_ASSERT(!IsInCompositorThread());

Nice comment about which thread should call this + assert.

@@ +232,5 @@
> +
> +  MonitorAutoLock lock(mScheduledCompositeMonitor);
> +  if (mScheduledComposite && mCompositorParent) {
> +    CompositorParent::CompositorLoop()->PostTask(FROM_HERE,
> +                            NewRunnableMethod(mCompositorParent.get(),

Here you take a pointer to mCompositorParent and put it at the back of the event loop. Is something keeping it alive? What if the refcount for mCompositorParent goes to zero while this event is still pending? You might need to hold on to this task and cancel it when mCompositor is destroyed or make sure that the lifetime is ok. Some examples in the graphics code:

http://mxr.mozilla.org/mozilla-central/ident?i=NewRunnableMethod&tree=mozilla-central&filter=gfx%2Flayers

I think we might get it wrong at some places but we need to fix them and something high frequency like the vsync callback needs to get this right.

@@ +233,5 @@
> +  MonitorAutoLock lock(mScheduledCompositeMonitor);
> +  if (mScheduledComposite && mCompositorParent) {
> +    CompositorParent::CompositorLoop()->PostTask(FROM_HERE,
> +                            NewRunnableMethod(mCompositorParent.get(),
> +                            &CompositorParent::CompositeCallback,

these two lines need to be indented.

@@ +239,5 @@
> +    mScheduledComposite = false;
> +    return true;
> +  }
> +
> +  UnobserveVsync(false);

IMO this would look better in the else block with a comment like:

// We're getting vsync notifications but we don't need to paint so unregister the vsync.

I know I said heuristics are bad and we can probably land this without it but unregistering vsync after a single frame might be unregistering is too fast. We can fix this in a follow up however. You might want to leave a note.

@@ +253,5 @@
> +/**
> + * Always make sure we observe/unobserve vsync messages from the compositor thread
> + * since the vsync dispatcher loop has its own locks before notifying us of
> + * vsync
> + */

It seems like you're code allows another thread to call this but will dispatch it to the right thread. If so then this comment belongs in the implementation since it's not a restriction placed on the caller.

@@ +276,5 @@
> +  // input thread is safe.
> +  if (!CompositorParent::IsInCompositorThread() && !isDestructing) {
> +    CompositorParent::CompositorLoop()->PostTask(FROM_HERE,
> +      NewRunnableMethod(this, &CompositorVsyncObserver::UnobserveVsync,
> +                              isDestructing));

this should align with the first argument (this), not the second.

@@ +597,5 @@
>  void
>  CompositorParent::CancelCurrentCompositeTask()
>  {
> +  if (gfxPrefs::VsyncAlignedCompositor()) {
> +    mCompositorVsyncObserver->ScheduleComposite(false);

This doesn't canceled any posted task like before.

@@ +757,5 @@
> +CompositorParent::CompositeCallback(TimeStamp aScheduleTime)
> +{
> +  if (gfxPrefs::VsyncAlignedCompositor()) {
> +    // Align OMTA to vsync time
> +    mLastCompose = aScheduleTime;

Note: We will probably want to clean this up later.

@@ +961,5 @@
>      // scheduled in order to better match the behavior under regular sampling
>      // conditions.
> +    bool composite = gfxPrefs::VsyncAlignedCompositor() ?
> +                    (mIsTesting && root && (mCurrentCompositeTask ||
> +                                      mCompositorVsyncObserver->DidScheduleComposite()))

indentation nit. Should be two space after ( on the previous line. ( is missing one space.

Actually this logic is too complex (ternary operator isn't needed). How about:
composite = mIsTesting && root &&
            (mCurrentCompositeTask ||
              (gfxPrefs::VsyncAlignedCompositor() && mCompositorVsyncObserver->DidScheduleComposite()))

Is composite a good name for this variable? We check mIsTesting so that's hint that the name isn't good.

::: gfx/layers/ipc/CompositorParent.h
@@ +88,5 @@
>  
>    friend class CompositorParent;
>  };
>  
> +class CompositorVsyncObserver MOZ_FINAL : public VsyncObserver

/**
 * Manages the vsync (de)registration and tracking on behalf of the compositor when it need to paint.
 * Turns vsync notifications into scheduled composites.
 */

@@ +94,5 @@
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(CompositorVsyncObserver)
> +
> +public:
> +  virtual bool NotifyVsync(TimeStamp aVsyncTimestamp) MOZ_OVERRIDE;
> +  CompositorVsyncObserver(CompositorParent* aCompositorParent);

nit: Put the constructor first.

@@ +102,5 @@
> +private:
> +  virtual ~CompositorVsyncObserver();
> +
> +  void ObserveVsync();
> +  void UnobserveVsync(bool isDestructing); // I wish unobserve was a real word!

This comment shouldn't land. IMO words like these are fine since it's just tearing down the other call.

@@ +106,5 @@
> +  void UnobserveVsync(bool isDestructing); // I wish unobserve was a real word!
> +
> +  mozilla::Monitor mScheduledCompositeMonitor;
> +  bool mScheduledComposite;
> +  bool mIsObservingVsync;

I wish we had a formal way to annotate that these variable MUST be accessed through the above monitor. Sadly we don't.

@@ +349,5 @@
>    CancelableTask *mCurrentCompositeTask;
>    TimeStamp mLastCompose;
>    TimeStamp mTestTime;
>    bool mIsTesting;
> +

whitespace change

::: gfx/thebes/gfxPrefs.h
@@ +202,5 @@
>  
>    DECL_GFX_PREF(Live, "gfx.draw-color-bars",                   CompositorDrawColorBars, bool, false);
>  
>    // Use vsync events generated by hardware
> +  DECL_GFX_PREF(Once, "gfx.frameuniformity.hw-vsync",          FrameUniformityHWVsyncEnabled, bool, true);

Is this ready to be turned on? I don't see you having addressed my feedback on preference name, if you think this is better please tell me why.

Comment 105

[Mason Chang [Inactive] [:mchang]]

Attachment: Composites Aligned With Vsync

Fixed most of the feedback from comment 104. I didn't want to keep track of a list of posted tasks, so I explicitly add/remove a ref when we get a vsync notification, along with a big warning comment. 

Also, a design decision I made was that we should not composite on non-vsync intervals with at least this patch. That means when we cancel a current task, we just stop listening to vsync instead. We also don't force composites and instead wait until the next vsync. I'm up for debate for this, but at least this keeps it consistent with the idea of, we only composite on vsyncs. I think we can fix those issues in a later patch as well.

Comment 106

[Benoit Girard (:BenWa)]

Comment on attachment 8506383 [details] [diff] [review]
Composites Aligned With Vsync

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

Almost there, I just need to be convinced that we ever want more than one posted composite task. I don't think we do because we risk recompositing the same frame.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +233,5 @@
> +  MOZ_ASSERT(!IsInCompositorThread());
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  MonitorAutoLock lock(mNeedsCompositeMonitor);
> +  if (mNeedsComposite && mCompositorParent) {

I'm not sure checking+resetting mNeedsComposite here is correct.

In fact I don't think you ever really want more then one composite in the queue at a time. If get LayerTransaction1+Vsync+LayerTransaction2+Vsync you're going to have two CompositeCallback at the back of the queue. The first one will include the changes from transaction 1+2 and the second CompositeCallback will be redundant.

@@ +241,5 @@
> +     * nsRefPtr for the CompositorParent and CompositorVsyncObserver, we have a
> +     * situation where this and the CompositorParent could be deleted while the
> +     * task is scheduled in the MessageLoop. Thus when the task finally
> +     * executes, the CompositorParent could be deleted. Because it is a chromium
> +     * thread, we can't use the nsRunnable class to keep a reference. Instead,

Yes but you can get a CancelableTask

@@ +491,5 @@
>  bool
>  CompositorParent::RecvFlushRendering()
>  {
> +  if (gfxPrefs::VsyncAlignedCompositor()) {
> +    mCompositorVsyncObserver->SetNeedsComposite(true);

You're changing the semantics here in a significal way. This is a sync message so the main thread is expecting, and waiting, that the composite is completed. But now you're only scheduling the vsync.

Are you sure this is ok? If it is then this message minus-well be async.

Comment 107

[Benoit Girard (:BenWa)]

(In reply to Mason Chang [:mchang] from comment #105)
> Also, a design decision I made was that we should not composite on non-vsync
> intervals with at least this patch.

For the place I pointed out I think we run the risk of regression so we shouldn't defer that if I'm not mistaken.

Comment 108

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [SilkFramework] Part1: PlatformTimer framework

PlatformVsyncTimer is the abstraction layer for all platform dependent vsync timer.
User can use PlatformVsyncTimerFactory::Create() to get the suitable for their platform.

Comment 109

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [SilkFramework] Part2: GonkVsyncTimer implement

The vsync timer implementation for gonk.
It get vsync event from hwc2D.

Comment 110

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [SilkFramework] Part3: VsyncDispatcher framework

The vsync dispatcher framework. It provides some event registery interface for
all vsync observer and dispatch to observer when a vsync event comes.

Comment 111

[Mason Chang [Inactive] [:mchang]]

Attachment: Vsync Aligned Compositor

From our discussion on irc, use a single CancelableTask*. I actually think, in a follow up patch, we should just overwrite the CompositorParent::mCurrentCompositeTask. I didn't want to do it now since I'd have to refactor a bunch to make sure it's all thread safe and I want to make sure we really want to stick with having at most 1 task posted.

Comment 112

[Mason Chang [Inactive] [:mchang]]

Attachment: Skeleton Vsync Framework

Skeleton Vsync Framework that does nothing. It's just a basic outline of a vsync framework so that we can land the compositor portion without breaking the build. I expect that the actual framework will still have to go through multiple reviews.

Comment 113

[Mason Chang [Inactive] [:mchang]]

Attachment: Skeleton Vsync Framework

Same as comment 112, just cleaned up a bit more. Also includes the preference renames done in the compositor patch. Builds and works on linux and b2g, will test on other platforms as well.

Comment 114

[Mason Chang [Inactive] [:mchang]]

Attachment: Vsync Aligned Compositor

Fixed a bug related to the compositor's posted task.

Comment 115

[Mason Chang [Inactive] [:mchang]]

Attachment: Alternate Vsync Aligned Compositor

I thought of another approach instead of what we were doing before. We post a task from the android vsync thread -> CompositorVsyncObserver. There we directly call the CompositorParent::CompositeCallback. In the CompositorVsyncObserver, we observe/unobserve vsync since we're on the compositor thread. It also lets me delete a bunch more of the locking since the android vsync thread doesn't read anything, it just posts a task unlike before.

Comment 116

[u459114]

Attachment: [SilkFramework] Part4: Silk Framework Unit test

Comment 117

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8506899 [details] [diff] [review]
[SilkFramework] Part3: VsyncDispatcher framework

We would like to implement the vsync-ailgn compostor and input dispatcher first.
So this patch remove a lot of code related to the ipc stuff.
This patch is similer to Mason's patch, but it pluses the PlatformVsyncTimer and VsyncEventRegistry abstract class.

1) The PlatformVsyncTimer hides the platform timer implememtation. Even though we only have b2g vsync timer currently, but we can extend to other platform easier.
  // create timer
  mTimer = PlatformVsyncTimerFactory::Create(this);

  // Start vsync timer.
  mTimer->Enable(true)

2) The VsyncEventRegistry registry is used for handling add/remove observer stuff. In this stage, we only have one observer(right now, we will implement the compositor first). But we will need to handle the IPC or input dispatcher in the future for b2g or for other platform.

  class VsyncDispatcher
  {
  public:
    virtual VsyncEventRegistry* GetInputDispatcherRegistry();
    virtual VsyncEventRegistry* GetCompositorRegistry();
    //virtual VsyncEventRegistry* GetVsyncEventParentRegistry(); //for ipc
  };

  //usage:
  VsyncDispatcher::GetInstance()->GetCompositorRegistry()->AddObserver(xxx);
  VsyncDispatcher::GetInstance()->GetCompositorRegistry()->RemoveObserver(xxx);
  //VsyncDispatcher::GetInstance()->GetVsyncEventParentRegistry()->AddObserver(xxx); // If we need IPC


We could just use the minimize framework code, or extend a little bit to get more flexibility but have more code and longer review process. What do you think?

Comment 118

[u459114]

Attachment: [SilkFramework] Part4: Silk Framework Unit test

Comment 119

[u459114]

Attachment: [SilkFramework] Part4: Silk Framework Unit test

Comment 120

[u459114]

Comment on attachment 8507672 [details] [diff] [review]
[SilkFramework] Part4: Silk Framework Unit test

Hi roc,
This is a basic testing suite for VsyncDispathcerHost.
I isolated VsyncDispathcerHostImp by MockTimer and MockVsyncObserver to verify the correctness of HostImp's behavior. f?

Comment 121

[u459114]

Comment on attachment 8505143 [details] [diff] [review]
Simple Vsync Framework

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

::: widget/shared/VsyncDispatcher.h
@@ +34,5 @@
> +  static VsyncDispatcher* GetInstance();
> +  void NotifyVsync(TimeStamp aVsyncTimestamp, nsecs_t aAndroidVsyncTime);
> +  void AddCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
> +  void RemoveCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
> +

I can's see big difference between your patch and Jerry's patch.
From API's level, the biggest difference is you change API of VsyncObserver registry/unregistry.
I don't have comments on this, please discuss with Jerry.

According to API change, you may need to modify in unit test as well(attachment 8507672 [details] [diff] [review]).
TEST_F(SilkHostTest, TimerEnabling): VsyncDispathcer should enable/disable VsyncTimer according to the number of vysnc observers.

Comment 122

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8507233 [details] [diff] [review]
Skeleton Vsync Framework

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

::: widget/shared/VsyncDispatcher.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef __mozilla_widget_VsyncDispatcher_h
> +#define __mozilla_widget_VsyncDispatcher_h

Technically there should be no leading underscores since those symbols are reserved by compiler.

@@ +26,5 @@
> +
> +public:
> +  static VsyncDispatcher* GetInstance();
> +  void AddCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
> +  void RemoveCompositorVsyncObserver(VsyncObserver* aVsyncObserver);

Document which threads can call these methods, and which thread the observer will be invoked on.

Comment 123

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8506899 [details] [diff] [review]
[SilkFramework] Part3: VsyncDispatcher framework

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

::: widget/shared/VsyncDispatcher.h
@@ +45,5 @@
> +  virtual void AddObserver(VsyncObserver* aVsyncObserver,
> +                           bool aAlwaysTrigger) = 0;
> +
> +  virtual void RemoveObserver(VsyncObserver* aVsyncObserver,
> +                              bool aAlwaysTrigger) = 0;

Use an enum instead of a bool to make callsites easier to read.

@@ +55,5 @@
> +  virtual ~VsyncEventRegistry() { }
> +};
> +
> +// VsyncDispatcher is used to dispatch vsync events to the registered observers.
> +class VsyncDispatcher

Why do we need this base class? Are we actually going to have more than one subclass of this class?

@@ +61,5 @@
> +public:
> +  static VsyncDispatcher* GetInstance();
> +
> +  virtual void Startup() = 0;
> +  virtual void Shutdown() = 0;

Document the thread these are called on

::: widget/shared/VsyncDispatcherHelper.h
@@ +25,5 @@
> +
> +// This class provide the simple registering implementation for vsync observer.
> +// We can set different thread policy to protect the observer list when we
> +// access. We use VsyncRegistryThreadSafePolicy by default. Please check
> +// VsyncRegistryThreadSafePolicy and VsyncRegistryNonThreadSafePolicy.

Why do we need both of these policies?

::: widget/shared/VsyncDispatcherHostImpl.h
@@ +14,5 @@
> +#include "VsyncDispatcher.h"
> +
> +namespace mozilla {
> +
> +// The host side vsync dispatcher implementation.

Needs more explanation of what this is and does.

@@ +18,5 @@
> +// The host side vsync dispatcher implementation.
> +class VsyncDispatcherHostImpl MOZ_FINAL : public VsyncDispatcher
> +                                        , public VsyncTimerObserver
> +{
> +  // We would like to create and delete the VsyncDispatcherHostImpl at main thread.

Explain why it needs to be destroyed on the main thread.

Comment 124

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8507672 [details] [diff] [review]
[SilkFramework] Part4: Silk Framework Unit test

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

I don't really know anything about gtests. Benoit Girard is probably a better reviewer for this.

Comment 125

[u459114]

Comment on attachment 8507672 [details] [diff] [review]
[SilkFramework] Part4: Silk Framework Unit test

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

hi benwa,
Please help to feedback this test cases, thanks.

This is a basic testing suite for VsyncDispatcher.
Isolating VsyncDispathcerHostImp by MockTimer and MockVsyncObserver to verify the correctness of VysynDispatcher's behavior.

Comment 126

[Benoit Girard (:BenWa)]

Comment on attachment 8507672 [details] [diff] [review]
[SilkFramework] Part4: Silk Framework Unit test

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

GTest usable looks good. Someone more familiar with silk should review the tests themsevles.

::: widget/shared/moz.build
@@ -21,4 @@
>  SOURCES += [
>      'PlatformVsyncTimer.cpp',
>      'VsyncDispatcher.cpp',
> -    'VsyncDispatcherClientImpl.cpp',

This removal doesn't make sense in the context of this patch

@@ +36,5 @@
>  
>  FINAL_LIBRARY = 'xul'
>  
> +if CONFIG['ENABLE_TESTS']:
> +    DIRS += ['tests/gtest']

I think putting the vsync implementation and tests in widget/shared is a mistake, it should probably go to widget/xpwidget. In fact I don't think widget/shared should exist at all and be moved to widget/xpwidget.

::: widget/shared/tests/gtest/TestVsyncDispatcher.cpp
@@ +18,5 @@
> +using ::testing::InSequence;
> +using ::testing::Return;
> +
> +// A manual timer for testing purepose.
> +// Send vsync notification while a test case explicit calls SendVsync

that explicitly calls*

@@ +25,5 @@
> +public:
> +  MockTimer(VsyncTimerObserver* aObserver)
> +    : PlatformVsyncTimer(aObserver)
> +  {
> +    /* Pass */

I think this comment is misleading. The constructor itself isn't a test case. Unless I'm misunderstanding something I think it would be better to not have a comment here.

::: widget/shared/tests/gtest/moz.build
@@ +7,5 @@
> +UNIFIED_SOURCES += [
> +    'TestVsyncDispatcher.cpp',
> +]
> +
> +include('/ipc/chromium/chromium-config.mozbuild')

I think this might not be needed. We're not compiling any IPDL sources here. I don't know?

Comment 127

[Benoit Girard (:BenWa)]

Comment on attachment 8507283 [details] [diff] [review]
Vsync Aligned Compositor

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

Nice! Let's make sure that the vsync tasks are safe and we're good to land.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +285,5 @@
> +   * vsync
> +   */
> +  if (!CompositorParent::IsInCompositorThread()) {
> +    CompositorParent::CompositorLoop()->PostTask(FROM_HERE,
> +      NewRunnableMethod(this, &CompositorVsyncObserver::ObserveVsync));

We don't hold a reference count when posting this task. Are you sure that this can't be deleted while this event is pending?

@@ +300,5 @@
> +  // This gets destroyed on the main thread, so thats also a safe place to
> +  // remove the compositor vsync observer. Any thread other than the android
> +  // input thread is safe.
> +  if (!CompositorParent::IsInCompositorThread() && !isDestructing) {
> +    CompositorParent::CompositorLoop()->PostTask(FROM_HERE,

same?

Comment 128

[Benoit Girard (:BenWa)]

Comment on attachment 8507300 [details] [diff] [review]
Alternate Vsync Aligned Compositor

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

Should this be an alternative patch? Because comparing the difference this patch doesn't include the new preferences in gfxPrefs.h

Had a quick look at the patch. They both feel very similar. I'll leave it up to your which one you want to land with the task issues fixed.

Comment 129

[Mason Chang [Inactive] [:mchang]]

Attachment: Skeleton Vsync Framework v2

Updated with feedback from comment 122. Also added assertions in the implementations to check that it's called on the right thread.

Comment 130

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8508208 [details] [diff] [review]
Skeleton Vsync Framework v2

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

Looks OK though there's really nothing here

::: widget/shared/VsyncDispatcher.cpp
@@ +16,5 @@
> +/*static*/ VsyncDispatcher*
> +VsyncDispatcher::GetInstance()
> +{
> +  if (!sVsyncDispatcher) {
> +    sVsyncDispatcher= new VsyncDispatcher();

Space before =

::: widget/shared/VsyncDispatcher.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef _mozilla_widget_VsyncDispatcher_h
> +#define _mozilla_widget_VsyncDispatcher_h

Actually you're not supposed to have any leading underscores

@@ +13,5 @@
> +{
> +public:
> +  // The method called when a vsync occurs. Return true if some work was done.
> +  // Vsync notifications will occur on the hardware vsync thread
> +  virtual bool NotifyVsync (TimeStamp aVsyncTimestamp) = 0;

No space before (

Comment 131

[Mason Chang [Inactive] [:mchang]]

Attachment: Skeleton Vsync Framework and Vsync Aligned Compositor. r=benwa, roc

Carrying r+, merged into a single patch. Successful try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fcf83788397c - Android 2.3 OPT Mochitest 6 seems to be perma-orange on mozilla-central.

b2g-inbound - https://hg.mozilla.org/integration/b2g-inbound/rev/c4b63beb3d76

Sheriffs - Please leave this open.

Comment 132

[Avi Halachmi (:avih)]

Since vsync is such a fundamental part of the quest for absolute smoothness, and tracking how smooth are we in practice sounds like a useful metric to have, would it be possible to add some telemetry for that?

Not sure what such metric might measure, but I can think of few measurements:

- Percentage of "handled on time" vsync events during a continuous vsync dispatch sequence.

- Average duration of "handled on time" vsync events in a continuous dispatch sequence (such that we're only looking at vsync which should have been handled but we didn't manage to do that on time). In other words - average duration between vsync glitches/missed-frames.

- Others?

Comment 133

[Mason Chang [Inactive] [:mchang]]

(In reply to Avi Halachmi (:avih) from comment #132)
> Since vsync is such a fundamental part of the quest for absolute smoothness,
> and tracking how smooth are we in practice sounds like a useful metric to
> have, would it be possible to add some telemetry for that?


Hi Avi,

You're right that it would be nice to have some better metrics especially when vsync is everywhere. In terms of adding telemetry data, that is in bug 1080160.

> Not sure what such metric might measure, but I can think of few measurements:
> 
> - Percentage of "handled on time" vsync events during a continuous vsync
> dispatch sequence.
> 
> - Average duration of "handled on time" vsync events in a continuous
> dispatch sequence (such that we're only looking at vsync which should have
> been handled but we didn't manage to do that on time). In other words -
> average duration between vsync glitches/missed-frames.

This is a good compositor measurement. Essentially we only care if the composite finished before the next vsync interval. We would like to use this measurement and we plan to do that.

> 
> - Others?

We had two other measurements in mind since Project Silk touches both touch input as well as the refresh driver.

1) Plot a perfect ideal scroll and compare it to our current scrolling behavior. This should give us an idea of smooth scrolling and tests the touch resampling. An in-depth look at what this means is here - http://www.masonchang.com/blog/2014/8/11/smooth-scrolling-frame-uniformity-touch-interpolation-and-touch-responsiveness

2) Animation smoothness / layer position with a RequestAnimationFrame animation - Once we have the refresh driver listening to vsync, we should have smoother animations, especially for requestAnimationFrame animations. We need a test case for this.

This way, we can test the compositor, input resampling, and refresh driver to ensure that we have measurable improved smoothness.

Comment 134

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4b63beb3d76

Comment 135

[Mason Chang [Inactive] [:mchang]]

Attachment: Simple Vsync Framework

A smaller and filled out Vsync Framework. It receives notifications from the HardwareComposer on vsync and notifies the Compositor and input touch resampler. A note about dispatching touch events - We will normally dispatch touch events AFTER a composite finishes in bug 1087048. If we dispatch touch events and composites at vsync, there is a race condition which causes much jankier behavior.

Comment 136

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8509955 [details] [diff] [review]
Simple Vsync Framework

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

::: widget/shared/VsyncDispatcher.h
@@ +44,5 @@
>  
>  public:
>    static VsyncDispatcher* GetInstance();
> +  // Called on the vsync thread when a hardware vsync occurs
> +  void NotifyVsync(TimeStamp aVsyncTimestamp, nsecs_t aAndroidVsyncTime);

What's the plan for making this cross-platform rather than just Android?

Comment 137

[Mason Chang [Inactive] [:mchang]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #136)
> Comment on attachment 8509955 [details] [diff] [review]
> Simple Vsync Framework
> 
> Review of attachment 8509955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/shared/VsyncDispatcher.h
> @@ +44,5 @@
> >  
> >  public:
> >    static VsyncDispatcher* GetInstance();
> > +  // Called on the vsync thread when a hardware vsync occurs
> > +  void NotifyVsync(TimeStamp aVsyncTimestamp, nsecs_t aAndroidVsyncTime);
> 
> What's the plan for making this cross-platform rather than just Android?

Once bug 1083530 lands, we will no longer need aAndroidVsynctime, just the mozilla::TimeStamp. Each platform will have to convert from their vsync event timestamp -> mozilla::TimeStamp and then the VsyncDispatcher should be platform agnostic.

Comment 138

[Mason Chang [Inactive] [:mchang]]

Successful try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0497b55010d7

In inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a72e359110a

Comment 139

[Mason Chang [Inactive] [:mchang]]

Attachment: Simple Vsync Framework

Carrying r+. Rebased on master.

Comment 140

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a72e359110a