Closed
Bug 1237689
Opened 8 years ago
Closed 8 years ago
[webvr] Enable Oculus Motion-To-Photon latency testing within Oculus HUD
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
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.
Updated•8 years ago
|
Whiteboard: [webvr]
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
- 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/
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8719611 -
Flags: review?(vladimir)
Assignee | ||
Comment 4•8 years ago
|
||
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/
Assignee | ||
Comment 5•8 years ago
|
||
I have updated the patch to correct build issues identified by the try run. This is a small (two character) change.
Assignee | ||
Updated•8 years ago
|
Attachment #8719611 -
Flags: review?(vladimir) → review?(dmu)
Assignee | ||
Comment 6•8 years ago
|
||
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•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
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
Assignee | ||
Comment 11•8 years ago
|
||
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•8 years ago
|
||
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 13•8 years ago
|
||
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+
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d1b7c2464c2d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•