I can't repro. We only request new callbacks from onVSync, and we would fail DCHECKs if we were doubling up on rAF callbacks as we only store one callback and clear it when called back. What you're describing should be impossible.

It is not doubling on rAF callbacks, it is doubling on VrShellGl::OnVSync callbacks. You may get the second call to it, when you call vsync_helper_.RequestVSync in VrShellGl::OnVSync. Seems like it is a matter of timing, so you could be just lucky not seeing it. Or, could be some other logic what prevents from double rAFs, however, it is still possible that you may receive double (or even tripple) OnVSyncs with Choreographer.

So, here is the sequence of calls at VSync event:
AndroidVSyncHelper.doFrame -> AndroidVSyncHelper::OnVSync -> VrShellGl::OnVSync -> AndroidVSyncHelper.requestVSync -> (instantly, since the previous callback is not removed yet and timing is still good for its call) ->  AndroidVSyncHelper.doFrame -> AndroidVSyncHelper::OnVSync -> VrShellGl::OnVSync.

Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

If what you're saying is correct, then your proposed fix doesn't fix the problem, as when you go to cancel the old callback there is none because it's already fired.

What you're describing sounds like it might be a framework bug, that I'm not seeing. Calling AndroidVSyncHelper.requestVSync within AndroidVSyncHelper.doFrame is supposed to request for the next frame.

Yes, it is a known "feature" of Android Choreographer. We see it a lot and my way is a correct way to avoid the issue. But it is your call for sure.

BTW, the same technique is used in standard Android SDK widget (android-25/android/widget/TextView.java): they also remove the handler inside doFrame.

Looking at the android source, https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/Choreographer.java

the callback is added to mCallbackQueues during postCallbackDelayedInternal, then we synchronously call scheduleFrameLocked if the delay time is <=0.  

scheduleFrameLocked will call scheduleVsyncLocked if we are on the looper thread, or post a message to run later, and we'll call doScheduleVsync.

It looks like we don't actually process the callbacks until the next vsync from the display - android_view_DisplayEventReceiver.cpp for example.

For example, doCallbacks in Choreographer appears to be called only when a vsync occurs, and it takes a snapshot of all callbacks that are due at the point it is run, so callbacks that add new callbacks won't have the new callbacks run during the same call of doCallbacks.

In short, I don't really see the platform bug, or how our implementation would call the vsync callback multiple times.

Potentially there could be bugs when we resume or start the vsync loop.

artyom, where you seeing real issues/symptoms of this, or is this a theoretical problem you are concerned with?

I have to admit, I've never seen it with Daydream code running (simply because I don't run Daydream code). However, I definitely saw double and even triple VSyncs when I tried to use AndroidVSyncHelper with Oculus code, plus, we know internally about this "feature" of the Choreographer. Maybe it depends on NDK/SDK/Android versions, I don't know.

The most likely explanation to me is that your code was unintentionally requesting VSyncs multiple times, and ended up creating multiple VSync loops.

However, if you do encounter a device that reproduces this bad Choreographer behaviour, please re-open this issue and let us know. We have DCHECKS in place that should detect this kind of bug and haven't seen it on any devices yet, so I would be surprised if any (even cardboard) VR-supporting devices have this bug.