When multiple calls to getFrameData are called within a single VR frame, the values returned for the first call that frame should be returned in subsequent calls even if the predicted pose has changed since the start of the frame. The VR frame is ended by successful calls to VRDisplay.exitPresent or VRDisplay.submitFrame.

To make this mochitest effective, the Puppet VR device must be able to simulate continuously changing pose prediction updates within a single frame.

Although VRMockDisplay::update uses async IPC channel, I have checked we still can the right frame data because we call GetSensorState() once a frame. If there is a new frame data coming, it should be gotten at the next frame not the current frame. If we can support shared memory, replacing VRMockDisplay::update aync with shared memory would be a better way to go though. VRMockDisplay::Update timestamp 1489382663361641.000000 // set new frame data VRDisplayPuppet::SetSensorState timestamp 1489382663361641.000000 VRDisplayPuppet::GetSensorState timestamp 1489382663361641.000000 VRMockDisplay::Update timestamp 1489382663490379.000000 // set new frame data VRDisplayPuppet::SetSensorState timestamp 1489382663490379.000000

It detects memory leaks on VRLayer. After Bug 1346680 is resolved, it can start the review process.

After applying the patch from Bug 1346680, try looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e658254fce9370ef73656d76e308a5e8fef57381

FYI - I will be switching over to a shared memory method of sending pose data in Bug 1346927. This test will be useful in ensuring that we do it right.

Comment on attachment 8846474 [details] Bug 1306493 - Part 1: Fix bug of getting frame data from VRPuppet; https://reviewboard.mozilla.org/r/119516/#review122254 r=me with the sizeof() arguments changed to the members rather than the type names for safety Replacing PR_Now() with a monotonically increasing value could be done in another bug. Looking great, thanks! ::: dom/vr/VRServiceTest.cpp:81 (Diff revision 2) > const Nullable<Float32Array>& aOrientation, > const Nullable<Float32Array>& aAngularVelocity, > const Nullable<Float32Array>& aAngularAcceleration) > { > + mSensorState.Clear(); > + mSensorState.timestamp = PR_Now(); PR_Now() doesn't return a monotonically increasing value. This won't matter for the tests we have landed so far, but should be adjusted. This could be done in another patch. ::: gfx/vr/gfxVRPuppet.cpp:123 (Diff revision 2) > > void > VRDisplayPuppet::SetDisplayInfo(const VRDisplayInfo& aDisplayInfo) > { > - mDisplayInfo = aDisplayInfo; > + // We are only interested in the eye info of the display info. > + memcpy(&mDisplayInfo.mEyeResolution, &aDisplayInfo.mEyeResolution, I think the = operator for mEyeResolution should be just as fast (or faster) than the memcpy here, and would be less brittle to type changes. ::: gfx/vr/gfxVRPuppet.cpp:125 (Diff revision 2) > VRDisplayPuppet::SetDisplayInfo(const VRDisplayInfo& aDisplayInfo) > { > - mDisplayInfo = aDisplayInfo; > + // We are only interested in the eye info of the display info. > + memcpy(&mDisplayInfo.mEyeResolution, &aDisplayInfo.mEyeResolution, > + sizeof(IntSize)); > + memcpy(&mDisplayInfo.mEyeFOV, &aDisplayInfo.mEyeFOV, sizeof(mDisplayInfo.mEyeFOV) would be safer than sizeof(VRFieldOfView) in case we change the type and don't update this code. ::: gfx/vr/gfxVRPuppet.cpp:127 (Diff revision 2) > - mDisplayInfo = aDisplayInfo; > + // We are only interested in the eye info of the display info. > + memcpy(&mDisplayInfo.mEyeResolution, &aDisplayInfo.mEyeResolution, > + sizeof(IntSize)); > + memcpy(&mDisplayInfo.mEyeFOV, &aDisplayInfo.mEyeFOV, > + sizeof(VRFieldOfView) * VRDisplayInfo::NumEyes); > + memcpy(&mDisplayInfo.mEyeTranslation, &aDisplayInfo.mEyeTranslation, sizeof(Point3D) => sizeof(mDisplayInfo.mEyeTranslation) for safety ::: gfx/vr/gfxVRPuppet.cpp:163 (Diff revision 2) > } > > void > VRDisplayPuppet::SetSensorState(const VRHMDSensorState& aSensorState) > { > - mSensorState = aSensorState; > + memcpy(&mSensorState, &aSensorState, sizeof(VRHMDSensorState)); sizeof(VRHMDSensorState) => sizeof(mSensorState) for safety

Comment on attachment 8846474 [details] Bug 1306493 - Part 1: Fix bug of getting frame data from VRPuppet; https://reviewboard.mozilla.org/r/119516/#review122254 > sizeof(mDisplayInfo.mEyeFOV) would be safer than sizeof(VRFieldOfView) in case we change the type and don't update this code. As this is an array, it would actually be: sizeof(mDisplayInfo.mEyeFov[0]) > sizeof(Point3D) => sizeof(mDisplayInfo.mEyeTranslation) for safety As this is an array, it would actually be: sizeof(mDisplayInfo.mEyeTranslation[0])

Comment on attachment 8846475 [details] Bug 1306493 - Part 2: Make vrMockDisplay to be the global var in VRSimulationDriver; https://reviewboard.mozilla.org/r/119518/#review122262 LGTM, thanks!

Comment on attachment 8846476 [details] Bug 1306493 - Part 3: Add VRDisplay getFrameData test for WebVR; https://reviewboard.mozilla.org/r/119520/#review122264 This looks great! Thanks!

Comment on attachment 8847525 [details] Bug 1306493 - Part 5: Make we only have one VRMockDisplay in tests; https://reviewboard.mozilla.org/r/120512/#review122624 LGTM

Comment on attachment 8847524 [details] Bug 1306493 - Part 4: Make timestamp to be a monotonically increasing value in VRMockDisplay; https://reviewboard.mozilla.org/r/120510/#review122700 Please change the call from PR_Now() to mozilla::TimeStamp::Now() for a true monotonic timestamp. r=me with that change. ::: dom/vr/VRServiceTest.cpp:82 (Diff revision 1) > const Nullable<Float32Array>& aOrientation, > const Nullable<Float32Array>& aAngularVelocity, > const Nullable<Float32Array>& aAngularAcceleration) > { > mSensorState.Clear(); > - mSensorState.timestamp = PR_Now(); > + mSensorState.timestamp = PR_Now() * 0.000001 - mTimestamp; PR_Now() will jump backwards when the user changes their system clock while the browser is running. For a monotonic timestamp you could call mozilla::TimeStamp::Now() instead of PR_Now(). Also note that it should not matter what the origin of this timestamp is, as long as the value changes does not reverse direction.

(In reply to Daosheng Mu[:daoshengmu] from comment #23) > Created attachment 8847891 [details] > Bug 1306493 - Part 6: disable require gesture when running VR tests; > > Review commit: https://reviewboard.mozilla.org/r/120818/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/120818/ If we don't disable it, it will return promise reject at https://dxr.mozilla.org/mozilla-central/rev/ff04d410e74b69acfab17ef7e73e7397602d5a68/dom/vr/VRDisplay.cpp#473.

Comment on attachment 8847891 [details] Bug 1306493 - Part 6: disable require gesture when running VR tests; https://reviewboard.mozilla.org/r/120818/#review123166 lgtm, thanks!

Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ce8f1cde004f Part 1: Fix bug of getting frame data from VRPuppet; r=kip https://hg.mozilla.org/integration/autoland/rev/c529a6fdcf2b Part 2: Make vrMockDisplay to be the global var in VRSimulationDriver; r=kip https://hg.mozilla.org/integration/autoland/rev/fe03299ff1eb Part 3: Add VRDisplay getFrameData test for WebVR; r=kip https://hg.mozilla.org/integration/autoland/rev/1f6c8b941701 Part 4: Make timestamp to be a monotonically increasing value in VRMockDisplay; r=kip https://hg.mozilla.org/integration/autoland/rev/b34efbb33ad6 Part 5: Make we only have one VRMockDisplay in tests; r=kip https://hg.mozilla.org/integration/autoland/rev/ad24ad035ca8 Part 6: disable require gesture when running VR tests; r=kip

https://hg.mozilla.org/mozilla-central/rev/ce8f1cde004f https://hg.mozilla.org/mozilla-central/rev/c529a6fdcf2b https://hg.mozilla.org/mozilla-central/rev/fe03299ff1eb https://hg.mozilla.org/mozilla-central/rev/1f6c8b941701 https://hg.mozilla.org/mozilla-central/rev/b34efbb33ad6 https://hg.mozilla.org/mozilla-central/rev/ad24ad035ca8