Closed Bug 1306493 Opened 8 years ago Closed 7 years ago

[webvr] Implement Mochitest: Multiple calls to VRDisplay.GetFrameData must return the same values within a frame

Categories

(Core :: WebVR, defect, P3)

Product:
Core
Component:
WebVR
Platform:
x86_64
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kip, Assigned: daoshengmu)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [gfx-noted][webvr])

Attachments

(6 files)

Bug 1306493 - Part 1: Fix bug of getting frame data from VRPuppet;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1306493 - Part 2: Make vrMockDisplay to be the global var in VRSimulationDriver;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1306493 - Part 3: Add VRDisplay getFrameData test for WebVR;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1306493 - Part 4: Make timestamp to be a monotonically increasing value in VRMockDisplay;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1306493 - Part 5: Make we only have one VRMockDisplay in tests;
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1306493 - Part 6: disable require gesture when running VR tests;
59 bytes, text/x-review-board-request
kip
: review+
Details 
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.
Blocks: 1229479
Priority: -- → P3
Whiteboard: [gfx-noted]
OS: Unspecified → Windows
Hardware: Unspecified → x86_64
Whiteboard: [gfx-noted] → [gfx-noted][webr]
Whiteboard: [gfx-noted][webr] → [gfx-noted][webvr]
Component: Graphics → WebVR
Assignee: nobody → dmu
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.
See Also: → 1346680
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.
See Also: → 1346927
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
Attachment #8846474 - Flags: review?(kgilbert) → review+
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!
Attachment #8846475 - Flags: review?(kgilbert) → review+
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!
Attachment #8846476 - Flags: review?(kgilbert) → review+
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
Attachment #8847525 - Flags: review?(kgilbert) → review+
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.
Attachment #8847524 - Flags: review?(kgilbert) → review+
(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!
Attachment #8847891 - Flags: review?(kgilbert) → review+
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
Depends on: 1358010
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: