1098701 - [Silk] Gtest for project Silk

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Boris Chiou [:boris]]

We have to implement the google test for our new silk framework. This makes it easy to find out the problems in our silk code.

Comment 1

[Boris Chiou [:boris]]

Attachment: Silk Original Framework Unit test by CJ

This patch is from Bug 1048667: Implement Vsync dispatch framework. CJ implemented this test for our original framework. Silk framework is different now, so we should update it. Put this patch here for our reference.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Gtests for Silk

Current gtest framework that uses a software vsync timer to imitate vsync. Currently only has two tests:

1) Can enable/disable vsync.
2) Can create a CompositorVsyncDispatcher and dispatch vsync events to VsyncObservers.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Gtests for Silk v2

Updated to poll for vsync notifications in the TestVsyncObserver.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Gtests for silk v3

Updated to implement an option to disable thread assertions like APZC. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#828

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Gtests for Silk v4

Gtests for silk. Rebased on master, bug fixes from disabling vsync.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Allow querying the Display to see if a CompositorVsyncDispatcher exists.

Useful for gtests to ensure that we can add/remove a CompositorVsyncDispatcher.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Allow Disabling Thread Assertions

Since gtest runs on the main thread, allow disabling thread assertions in the CompositorVsyncDispatcher. Much like APZ gtests - https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.h?from=AsyncPanZoomController.h&case=true#1048

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Create TestVsyncObserver and gtest Skeleton

Create a TestVsyncObserver. It polls for 50 ms on the main thread to ensure a vsync notification occurs. This will be ok since the Vsync always occurs off the main thread.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Actual gtests

The actual gtests. Two tests included for now:

1) Ensure that we can enable/disable vsync
2) Ensure that the CompositorVsyncDispatcher gets vsync notifications.

I will add RefreshTimerVsyncDispatcher gtests in a follow up bug once we get closer to enabling the refresh driver by default.

Comment 10

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

Comment on attachment 8551928 [details] [diff] [review]
Part 3: Create TestVsyncObserver and gtest Skeleton

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

::: gfx/tests/gtest/TestVsync.cpp
@@ +45,5 @@
> +    TimeStamp start = TimeStamp::Now();
> +    for (;;)
> +    {
> +      TimeDuration duration = TimeStamp::Now() - start;
> +      if (duration.ToMilliseconds() > kVsyncTimeoutMS) {

Instead of busy-looping, use MonitorAutoLock::Wait and ::Notify

Comment 11

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

All of these patches need commit messages.

Comment 12

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

Comment on attachment 8551929 [details] [diff] [review]
Part 4: Actual gtests

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

Combine this patch with part 3; not much point keeping them separate here.

::: gfx/tests/gtest/TestVsync.cpp
@@ +95,5 @@
> +  globalDisplay.DisableVsync();
> +  ASSERT_TRUE(!globalDisplay.IsVsyncEnabled());
> +
> +  globalDisplay.EnableVsync();
> +  ASSERT_FALSE(globalDisplay.IsVsyncEnabled());

This should be ASSERT_TRUE. If anything the other two asserts in this function could be converted to ASSERT_FALSE.

@@ +119,5 @@
> +  ASSERT_FALSE(globalDisplay.ContainsCompositorVsyncDispatcher(vsyncDispatcher));
> +
> +  nsRefPtr<TestVsyncObserver> testVsyncObserver = new TestVsyncObserver();
> +  vsyncDispatcher->SetCompositorVsyncObserver(testVsyncObserver);
> +  ASSERT_TRUE(globalDisplay.ContainsCompositorVsyncDispatcher(vsyncDispatcher));

I don't think you need to assert this; the fact that the testVsyncObserver gets a vsync notification is proof enough that this is true. And removing this assert means you can get rid of part 1. I would prefer that because it exposes a messy hook in production code just for test purposes.

Comment 13

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

Comment on attachment 8551925 [details] [diff] [review]
Part 1: Allow querying the Display to see if a CompositorVsyncDispatcher exists.

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

See comment on part 4, we can do without this.

Comment 14

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

Comment on attachment 8551927 [details] [diff] [review]
Part 2: Allow Disabling Thread Assertions

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

::: widget/VsyncDispatcher.cpp
@@ +15,5 @@
>  #endif
>  
>  namespace mozilla {
> +static bool sThreadAssertionsEnabled = true;
> +void CompositorVsyncDispatcher::SetThreadAssertionsEnabled(bool aEnable)

nit: add a blank line before this function

@@ +58,5 @@
> +  if (!sThreadAssertionsEnabled) {
> +    return;
> +  }
> +
> +  MOZ_ASSERT(layers::CompositorParent::IsInCompositorThread());

I'd rather you used Compositor::AssertOnCompositorThread() here, and renamed this function to CompositorVsyncDispatcher::AssertOnCompositorThread

@@ +90,5 @@
>    bool observeVsync = aVsyncObserver != nullptr;
> +  if (NS_IsMainThread()) {
> +    // Should only occur under test conditions
> +    MOZ_ASSERT(!sThreadAssertionsEnabled);
> +    ObserveVsync(observeVsync);

I'm not a fan of this. It seems like the more tests you add the more this kind of code is going to be littered everywhere. I suggest talking to BenWa about this, he's been working on being able to write proper compositor gtests and you might be able to use his work to have less hacks here.

::: widget/VsyncDispatcher.h
@@ +46,5 @@
>    void SetCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
>    void Shutdown();
>  
> +  // Useful for gtests which only run on one thread. This will disable
> +  // thread assertions.

"This can be used to enable or disable thread assertions. This is useful for gtests because usually things run in only one thread in that environment"

@@ +53,2 @@
>  private:
> +  void AssertInCompositorThread();;

s/;;/;/

Comment 15

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

So I think I would prefer an approach where instead of doing

  if (NS_IsMainThread()) {
    // Should only occur under test conditions
    Func();
  } else {
    NS_DispatchToMainThread(..Func..)
  }

you leave the original code as-is (i.e. NS_DispatchToMainThread) and then in the gtest follow it up with calling NS_GetMainThread [1] and then run |while (thread->processNextEvent(false));| to process the pending dispatch-to-main-thread events. Give that a shot and see if it works.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp?rev=b71ccef9c674#113
[2] http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsIThread.idl?rev=7e51a10dee23#67

Comment 16

[Mason Chang [Inactive] [:mchang]]

Attachment: Part1: Gtest Framework and Tests

Updated with feedback from comments 10-13, 15.

1) Use notify/wait in the lock instead of polling
2) Flush notifications on main thread.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Allow Disabling Thread Assertions

Updated with feedback from comment 14. 

> I'm not a fan of this. It seems like the more tests you add the more this 
> kind of code is going to be littered everywhere. I suggest talking to BenWa 
> about this, he's been working on being able to write proper compositor gtests 
> and you might be able to use his work to have less hacks here.

I talked with BenWa about this. Currently, his work is to spin up the actual Compositor and their backends, not the CompositorParent/Child pair which is where the CompositorVsyncObserver is. We both agreed it'd be nice to have gtests for both vsync in isolate and large internal tests.

We can do large internal gtests in a follow up bug, which will require some work to mock up both the CompositorParent/Child pair. We can keep this bug to have gtests for small vsync verification unit tests.

Comment 18

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

Comment on attachment 8552125 [details] [diff] [review]
Part1: Gtest Framework and Tests

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

Just to be clear, this test won't actually get exercised on B2G at all, right? It will only start getting exercised once we turn on the HardwareVsyncEnabled pref on OS X (or other desktop platforms)?

::: gfx/tests/gtest/TestVsync.cpp
@@ +43,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    MonitorAutoLock lock(mVsyncMonitor);
> +    PRIntervalTime timeout = PR_MillisecondsToInterval(kVsyncTimeoutMS);
> +    lock.Wait(timeout);

Early return before the Wait call if mDidGetVsyncNotification is already true.

@@ +46,5 @@
> +    PRIntervalTime timeout = PR_MillisecondsToInterval(kVsyncTimeoutMS);
> +    lock.Wait(timeout);
> +  }
> +
> +  bool mDidGetVsyncNotification;

Make this private and expose a getter function. Also add a comment that it is protected by mVsyncMonitor

@@ +85,5 @@
> +  rv = NS_OK;
> +  bool processed = true;
> +  while (processed && NS_SUCCEEDED(rv)) {
> +    rv = mainThread->ProcessNextEvent(false, &processed);
> +  }

you could also ASSERT_TRUE(NS_SUCCEEDED(rv)) at the bottom of the loop and take it out of the loop condition if you want. this is fine too.

Comment 19

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Gtest framework and tests

Updated with feedback from comment 18.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> Comment on attachment 8552125 [details] [diff] [review]
> Part1: Gtest Framework and Tests
> 
> Review of attachment 8552125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just to be clear, this test won't actually get exercised on B2G at all,
> right? It will only start getting exercised once we turn on the
> HardwareVsyncEnabled pref on OS X (or other desktop platforms)?

Unfortunately :(. Still helps locally with testing though since I force enable the prefs on desktop.

Comment 20

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/121f665885d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1649a66f18e9

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

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/121f665885d5
https://hg.mozilla.org/mozilla-central/rev/1649a66f18e9

Comment 22

[Yuan Pengfei]

Comment on attachment 8552126 [details] [diff] [review]
Part 2: Allow Disabling Thread Assertions

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

::: widget/VsyncDispatcher.cpp
@@ +62,5 @@
> +  if (!sThreadAssertionsEnabled) {
> +    return;
> +  }
> +
> +  Compositor::AssertOnCompositorThread();

layers::Compositor::AssertOnCompositorThread();