Closed Bug 1237689 Opened 4 years ago Closed 4 years ago

[webvr] Enable Oculus Motion-To-Photon latency testing within Oculus HUD

Categories

(Core :: Graphics, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: kip, Assigned: kip)

References

()

Details

(Whiteboard: [webvr])

Attachments

(1 file, 1 obsolete file)

In order for the Oculus hardware latency tester to display accurate latency statistics in the Oculus HUD, we need to track sequential frame indexes into the ovr_GetTrackingState call and to the corresponding call to ovr_SubmitFrame.

The "Rendering on Different Threads" section of the Oculus SDK document linked describes this in further details.
Blocks: 1229485
Blocks: 1237691
Blocks: 1237693
Whiteboard: [webvr]
WIP - This patch functions, but before landing:
- Cleanup where commented with "FINDME" keyword
- Need to separate pose prediction implementation, put it behind a pref, and apply separately to Bug 1237691.
- Generate and pass sequential frame indexes into the ovr_GetTrackingState call and the corresponding call to ovr_SubmitFrame

Review commit: https://reviewboard.mozilla.org/r/35029/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35029/
Comment on attachment 8717643 [details] [diff] [review]
Enable Oculus hardware latency tester. Generate and pass sequential frame indexes into the ovr_GetTrackingState call and the corresponding call to ovr_SubmitFrame

Updated patch now in mozreview
Attachment #8717643 - Attachment is obsolete: true
Attachment #8719611 - Flags: review?(vladimir)
Comment on attachment 8719611 [details]
MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35029/diff/1-2/
I have updated the patch to correct build issues identified by the try run.  This is a small (two character) change.
Attachment #8719611 - Flags: review?(vladimir) → review?(dmu)
I have assigned reviews for Bug 237689 and Bug 1237691 to Daosheng.  Please needinfo me if you have any questions or if you won't have time to review, thanks!
Comment on attachment 8719611 [details]
MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester

https://reviewboard.mozilla.org/r/35029/#review31933

This looks good for adding frame indexes to ovr_SubmitFrame. I just concern about if we should separate vr usage and no-vr usage for ContainerLayer, TimedTexture, and TimedTextureClient.

::: gfx/layers/Layers.h:2234
(Diff revision 2)
> +  int32_t mVRFrameIndex;

IMHO, if we can make a new ContainerLayer class for VR that probably can be named VRContainerLayer to remove specific vr attributes for other no-vr ContainerLayer usage.

::: gfx/layers/composite/CompositableHost.h:197
(Diff revision 2)
> +    int32_t mVRFrameIndex;

Probably, making a VRTimedTexture that inherits TimedTexture would be better?

::: gfx/layers/composite/ImageHost.h:122
(Diff revision 2)
> +    int32_t mVRFrameIndex;

Making a new VRTimedImage that inherits TimedImage would be better?

::: gfx/layers/ipc/CompositableForwarder.h:123
(Diff revision 2)
> +    int32_t mVRFrameIndex;

Same. I am considering to make a VRTimedTextureClient to separate VR and no-VR type.

::: gfx/layers/ipc/LayersMessages.ipdlh:400
(Diff revision 2)
> +  int32_t VRFrameID;

Same. I think we can consider to make a VRTimedTexture
Attachment #8719611 - Flags: review?(dmu)
Comment on attachment 8719611 [details]
MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35029/diff/2-3/
Attachment #8719611 - Flags: review?(dmu)
(In reply to :kip (Kearwood Gilbert) from comment #8)
> Comment on attachment 8719611 [details]
> MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/35029/diff/2-3/

I have updated the patch with non-vr-specific naming of the frame indexes (now called inputFrameID), to reflect the future use of this in non-vr input latency measurement.

A comment (marked with XXX) has been added to indicate a reference to VRManagerChild which will be moved to more VR specific code with the implementation of the upcoming WebVR 1.0 API.
https://reviewboard.mozilla.org/r/35029/#review31933

> IMHO, if we can make a new ContainerLayer class for VR that probably can be named VRContainerLayer to remove specific vr attributes for other no-vr ContainerLayer usage.

This is a good suggestion; however, it would greatly increase the complexity as classes such as ContainerLayerComposite derive from ContainerLayer, resulting in even more VR specific classes.

One of our goals is to track latency of inputs that are not specifically related to VR.  Would it be acceptable to rename mVRFrameIndex to something non-vr specific, anticipating that it will be used outside of VR?

> Probably, making a VRTimedTexture that inherits TimedTexture would be better?

As per my earlier comment, would it be feasible to rename mVRFrameIndex to a non-vr-specific name?

> Making a new VRTimedImage that inherits TimedImage would be better?

Same as above -- Would it be feasible to rename mVRFrameIndex to a non-vr-specific name?

> Same. I am considering to make a VRTimedTextureClient to separate VR and no-VR type.

More of the same, renaming mVRFrameIndex to something non-vr-specific.  Additionally, there is complexity to having separate VR and non-vr classes, as it is possible to enter and exit VR mode, requiring re-instantiation of the classes to change their types.

> Same. I think we can consider to make a VRTimedTexture

Same here
https://reviewboard.mozilla.org/r/35029/#review31933

> This is a good suggestion; however, it would greatly increase the complexity as classes such as ContainerLayerComposite derive from ContainerLayer, resulting in even more VR specific classes.
> 
> One of our goals is to track latency of inputs that are not specifically related to VR.  Would it be acceptable to rename mVRFrameIndex to something non-vr specific, anticipating that it will be used outside of VR?

I have updated the patch with non-vr-specific naming of the frame indexes (now called inputFrameID), to reflect the future use of this in non-vr input latency measurement.

A comment (marked with XXX) has been added to indicate a reference to VRManagerChild which will be moved to more VR specific code with the implementation of the upcoming WebVR 1.0 API.
Comment on attachment 8719611 [details]
MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester

https://reviewboard.mozilla.org/r/35029/#review32035

It looks good to me

::: gfx/layers/client/CanvasClient.cpp:490
(Diff revision 3)
> +  t->mInputFrameID = VRManagerChild::Get()->GetInputFrameID();

Nice!
Attachment #8719611 - Flags: review?(dmu)
Comment on attachment 8719611 [details]
MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester

https://reviewboard.mozilla.org/r/35029/#review32039
Attachment #8719611 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d1b7c2464c2d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.