Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add XRFrame.predictedDisplayTime #1217

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Aug 18, 2021

Hi everyone,

Following up on #1211 and the discussion in the latest meeting, here's a proposal/draft for the XRFrame.predictedDisplayTime property.

I adapted the main loop rendering example to demonstrate the use of predictedDisplayTime. While the explicit time parameter here is redundant, it shows a clear difference to the window rAF.

This spec currently expects the implementor to understand how the display time could be predicted rather than providing an algorithm. I'm uncertain if it is okay to assume that the underlying implementation/SDK has this value available (as discussed, it is for OpenXR). Could it be useful to add a suggestion for what to set the value to if it is not?

Best,
Jonathan


Preview | Diff

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, though I'd want the other editors to approve too

@Manishearth
Copy link
Contributor

@Squareys are you affiliated with any immersive-web WG members? Otherwise we can't merge substantiative PRs (but we could potentially recreate this)

@Squareys
Copy link
Contributor Author

@Manishearth I am not affiliated with any of the WG member companies.

Feel free to copy/reproduce any changes you see fit. Let me know, if I can pronounce the changes under any permissive license to allow you to trivially copy--even without attribution, if needed.

@Manishearth
Copy link
Contributor

@himorin is there a way to proceed here or should one of the editors reproduce this change using our own words

@Squareys
Copy link
Contributor Author

@Manishearth @himorin Any news on this? :)

@himorin
Copy link
Member

himorin commented Aug 27, 2021

Sorry, I've missed this notification.
For external contributor agreement, one need to have W3C account with github account connected.
If you don't have W3C account, please create your. If you haven't connect your github account to your W3C account yet, please connect from your account page.

@@ -1085,6 +1087,14 @@ Each {{XRFrame}} has an <dfn for="XRFrame">active</dfn> boolean which is initial

The <dfn attribute for="XRFrame">session</dfn> attribute returns the {{XRSession}} that produced the {{XRFrame}}.

The <dfn attribute for="XRFrame">predictedDisplayTime</dfn> attribute returns the {{DOMHighResTimeStamp}} that represents the point in time at which a rendered frame submitted for this {{XRFrame}} is expected to show up on the devices display.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a bit clearer:
that represents the point in time at which the [=XR Compositor=] expects this {{XRFrame}} to be displayed on the [=XR Device=].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

index.bs Outdated
@@ -990,6 +990,7 @@ When an {{XRSession}} |session| receives updated [=viewer=] state for timestamp
1. Let |now| be the [=current high resolution time=].
1. Let |frame| be |session|'s [=XRSession/animation frame=].
1. Set |frame|'s [=XRFrame/time=] to |frameTime|.
1. Set |frame|'s [predictedDisplayTime] to when the callback's result is expected to show up on the devices display.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:
to the timestampe when the [=XR Compositor=] is expected to display content drawn during this [=XR animation frame=]

index.bs Outdated
<div class=note>
The [=XRFrame/predictedDisplayTime=] is intended to allow rendering an animated XR scene in the state that it should be in when the frame is displayed rather than when the {{XRSession/requestAnimationFrame()}} callback was scheduled or when it was executed.

The [=XRFrame/predictedDisplayTime=] is not intended be used to infer how much time the application has for rendering, as the [=XRFrame/predictedDisplayTime=] is later than when the frame needs to have been submitted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:
as the [=XR Compositor=] typically has to do extra processing after the frame is submitted. If the experience assumes that it can process up to [=XRFrame/predictedDisplayTime=], it will miss every single frame and will not be able to make full framerate.

Copy link
Contributor Author

@Squareys Squareys Sep 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(bikeshedding here) I think the concepts of "missing frames" or "making framerate" aren't otherwise described in the spec so far, I'll try to avoid them as so:

it will miss every single frame and will not be able to make full framerate.

"the [=XR Compositor=] will not be able to make use of the submitted frames, and the application would not hit (make?) target framerate."

Alternatively, I could add definitions for "target framerate", which seems to also be missing (at least in the main spec), "dropping a frame" and "missing a frame"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a note and not normative, it's ok to use new concepts.

@Squareys
Copy link
Contributor Author

Squareys commented Sep 1, 2021

@himorin I created an account and linked it to github, but revalidating still shows:
image

@cabanier Thanks for the review! Will address the feedback by end of the week.

index.bs Outdated
@@ -1075,6 +1076,7 @@ An {{XRFrame}} represents a snapshot of the state of all of the tracked objects
<pre class="idl">
[SecureContext, Exposed=Window] interface XRFrame {
[SameObject] readonly attribute XRSession session;
DOMHighResTimeStamp predictedDisplayTime;
Copy link
Member

@cabanier cabanier Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add readonly attribute

@cabanier
Copy link
Member

What should the behavior be for inline sessions?

@cabanier
Copy link
Member

Also, what should be the "start" time? Should it match the document session start like the one that is passed to requestAnimationFrame?
/agenda discuss review comments on XRFrame.predictedDisplayTime

@probot-label probot-label bot added the agenda Request discussion in the next telecon/FTF label Sep 13, 2021
@Manishearth
Copy link
Contributor

@himorin updates on the external contributor agreement?

Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

@himorin @Manishearth I received and signed the external contributors agreement and the automatic checks are happy now!

@cabanier

Also, what should be the "start" time? Should it match the document session start like the one that is passed to requestAnimationFrame?

The only thing important for applications is the monotonically increasing nature of the timestamp, it might not be necessary provide any further guarantees. While it might be confusing if the numbers aren't comparable to performance.now() or the rAF timestamp, it may otoh help avoid developers inferring anything from the diff that they shouldn't.

In Wonderland Engine we snapshot the "start time" every time larger hitches/update time deltas would occur, e.g. after session focus event and session start event, to reset the large delta time that would cause physics instability for example--and also if a switch in "clock" occurs, namely performance.now() and XRFrame.predictedDisplayTime.

What should the behavior be for inline sessions?

I would assume that any requestAnimationFrame has a guesstimate of when it would like to display the frame, but missing a frame there would probably just lead to it displaying on the next vsync? There's been discussions about vsync on desktop browsers for a long time (w3c/html#785 as referenced by #1211), might be a motif even for non-XR web games to use WebXR, heh.

I've updated the PR to address your comments @cabanier !

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, should also get approval from another editor.

@cabanier
Copy link
Member

What should the behavior be for inline sessions?

I would assume that any requestAnimationFrame has a guesstimate of when it would like to display the frame, but missing a frame there would probably just lead to it displaying on the next vsync? There's been discussions about vsync on desktop browsers for a long time (w3c/html#785 as referenced by #1211), might be a motif even for non-XR web games to use WebXR, heh.

That will introduce a security issue because now a page can get very accurate timing on how long it will take to display itself.
There have been many panics in the past where a similar ability let a page leak private information.

I think you should make the parameter optional and define that it is not populated for inline session.
Alternatively, you could set it to the same timestamp as time in the RequestAnimationCallback

@Yonet Yonet removed the agenda Request discussion in the next telecon/FTF label Oct 5, 2021
@cabanier cabanier merged commit cc6b625 into immersive-web:main Oct 5, 2021
github-actions bot added a commit that referenced this pull request Oct 5, 2021
SHA: cc6b625
Reason: push, by @cabanier

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants