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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[:kip (Kearwood Gilbert)]

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.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Enable Oculus hardware latency tester. Generate and pass sequential frame indexes into the ovr_GetTrackingState call and the corresponding call to ovr_SubmitFrame

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.

Comment 2

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester

- 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 3

[:kip (Kearwood Gilbert)]

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

Comment 4

[:kip (Kearwood Gilbert)]

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/

Comment 5

[:kip (Kearwood Gilbert)]

I have updated the patch to correct build issues identified by the try run.  This is a small (two character) change.

Comment 6

[:kip (Kearwood Gilbert)]

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 7

[Daosheng Mu[:daoshengmu]]

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

Comment 8

[:kip (Kearwood Gilbert)]

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/

Comment 9

[:kip (Kearwood Gilbert)]

(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.

Comment 10

[:kip (Kearwood Gilbert)]

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

Comment 11

[:kip (Kearwood Gilbert)]

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 12

[Daosheng Mu[:daoshengmu]]

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!

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8719611 [details]
MozReview Request: Bug 1237689 - Enable Oculus hardware latency tester

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b7c2464c2d

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1b7c2464c2d