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

Note somewhere that you should be always be queueing up the next rAF within rAF #877

Closed
Manishearth opened this issue Oct 15, 2019 · 7 comments
Labels
fixed by pending PR A PR that is in review will resolve this issue. help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Milestone

Comments

@Manishearth
Copy link
Contributor

Currently, we run an XR animation frame regardless of whether or not you've triggered rAF.

When an XRSession session receives updated viewer state from the XR device, it runs an XR animation frame with a timestamp now and an XRFrame frame, which MUST run the following steps regardless of if the list of animation frame callbacks is empty or not:

We also clear framebuffers before each "XR animation frame":

The buffers attached to an opaque framebuffer MUST be cleared to the values in the table below when first created, or prior to the processing of each XR animation frame. This is identical to the behavior of the WebGL context’s default framebuffer.

Both of these things are good decisions.

However, put together, this means that an application that does not queue up the next rAF within the current rAF callback will experience flashes of black.

This is kind of different from HTML rAF, in HTML you can choose to not queue up a rAF for a bit and that just makes the scene static until you do, whether your application is moving elements around or canvas stuff. This is less jarring.

It seems to me that the only time you should not queue up a rAF within your rAF callback is when you don't intend to render anything -- e.g. when your visibility state becomes hidden.

It may be worth noting this non normatively somewhere in the spec, for authors (and doc writers) to pick up.

@toji
Copy link
Member

toji commented Oct 15, 2019

Hm... I don't agree with this, though you bring up a good point about the flash of black. That's unintended.

It's a valid technique in some cases to render a single frame (like a splash screen logo) and then skip several frames while other work is happening, letting the device's reprojection system (if it has one) handle any tracking. I don't think we want to prevent that from working here, and so "forcing" developers to keep a full rAF loop scheduled seems problematic.

I'd rather that we find some way to express that the compositor should only update it's imagery if the layer framebuffer is modified during the course of the rAF callbacks (not including the initial clear). This is, as far as I recall, what Chrome is doing anyway.

@Manishearth
Copy link
Contributor Author

cc @asajeffrey

I suspect that might be pretty tricky to specify? Not sure if webgl gives us the terms necessary to spec that.

I'm okay with us specifying that -- it is a nicer solution -- but I'd check in with implementations first.

@Manishearth
Copy link
Contributor Author

Either way, I think we should definitely explicitly spec this if we're going this way, or add the note.

@toji
Copy link
Member

toji commented Oct 15, 2019

The WebGL spec already states this pretty clearly:

WebGL presents its drawing buffer to the HTML page compositor immediately before a compositing operation, but only if at least one of the following has occurred since the previous compositing operation:

  • Context creation
  • Canvas resize
  • clear, drawArrays, or drawElements has been called while the drawing buffer is the currently bound framebuffer

I'm sure we could use some variant of that.

@Manishearth
Copy link
Contributor Author

Ah, that seems reasonable

@Manishearth
Copy link
Contributor Author

I discussed this a bit with @asajeffrey and a secondary constraint popped up: we should be doing this only at the end of processing an animation frame. I.e if for some reason an rAF callback loops infinitely, doing a bunch of webgl stuff, it should not be reflected on the device until the callback is over.

Mostly the distinction will surface in cases where the page code is written terribly, but this is both cleaner for implementations and leads to slightly more logical behavior.

This also means that this issue can be fixed with a single "if draw (etc) has been called, render to the device" line at the end of the "process an xr frame" steps

bors-servo pushed a commit to servo/servo that referenced this issue Oct 17, 2019
Always request new XR frames

Currently we only request XR frames when there is a rAF callback queued up.

This is incorrect, see https://immersive-web.github.io/webxr/#xr-animation-frame

With this change we always request updated frame state regardless of whether or not the user has queued up callbacks.

In addition to being spec-correct, this means that we can reliably listen for events in the OpenXR backend in the rAF loop itself, instead of having to set up a second spinning loop for when the rAF is inactive. This lets us implement visibility states for openxr without having to worry about the user never receiving the wakeup call because the rAF loop isn't pumping.

See also: immersive-web/webxr#877

r? @asajeffrey
@Manishearth Manishearth added the help wanted This is a good issue for anyone to pick up and work on filing a PR for. label Oct 21, 2019
@Manishearth Manishearth added this to the November 2019 milestone Oct 21, 2019
@Manishearth
Copy link
Contributor Author

We discussed this a bit and want to make these changes: we should be updating things on the compositor only if the layer framebuffer is modified, and only after the end of a rAF call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed by pending PR A PR that is in review will resolve this issue. help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Projects
None yet
Development

No branches or pull requests

2 participants