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

openxr: how to properly end session? #47

Closed
paulrouget opened this issue Aug 30, 2019 · 11 comments · Fixed by #58
Closed

openxr: how to properly end session? #47

paulrouget opened this issue Aug 30, 2019 · 11 comments · Fixed by #58

Comments

@paulrouget
Copy link
Contributor

Ending the session with openxr is a 2 steps process:

  1. call session.request_exit()
  2. on openxr events SessionStateChanged(Stopping), call session.end()

What would be the right way of doing this? request_exit should be called in device.quit(), but I'm not sure when/where to pull the openxr events. For now I do it in wait_for_animation_frame. Is there a better way?

@jdm
Copy link
Member

jdm commented Aug 30, 2019

The fact that https://www.khronos.org/registry/OpenXR/specs/1.0/html/xrspec.html#events mentions that events must be polled regularly and we don't do that is a bit worrying.

@paulrouget
Copy link
Contributor Author

Would it be bad to do that in wait_for_animation_frame?

@paulrouget
Copy link
Contributor Author

One of the problem with wait_for_animation_frame is that it won't be called after Quit, but we still need to pull events afterward.

bors-servo pushed a commit that referenced this issue Sep 1, 2019
request openxr shutdown on quit

This will end the openxr session (back to 2D view in the case of HoloLens).

It's still missing the call to `session.end()` (see #47).

Fix #48
@paulrouget
Copy link
Contributor Author

@Manishearth we need to loop through the openxr events. Where/How do you recommend we do that?

@gterzian
Copy link
Member

gterzian commented Sep 3, 2019

My five cents:

In theory events from the device are forwarded to script, once set_event_dest has been set, for example at: https://github.com/servo/servo/blob/f21d11606987a4febcb54431c5c4577ab018dec7/components/script/dom/xrsession.rs#L152

Then you could handle the exit event from script, at https://github.com/servo/servo/blob/f21d11606987a4febcb54431c5c4577ab018dec7/components/script/dom/xrsession.rs#L155

and send a message back to the embedder/main-thread to run the final session.end step.

In practice it's not clear to me how events go from the device to

events: EventBuffer,

Since I don't see any polling or handling of events, it seems to only be used at

self.events.callback(Event::SessionEnd);

it might be that this functionality requires a new hook into a new Device API like device.poll_events as part of session run_one_frame(or let's call the "WebXr loop"), which could see in the case of openxr the device use the apparently non-blocking xrPollEvent to poll events and forward them to the buffer via self.events.callback(from inside the Device).

@gterzian
Copy link
Member

gterzian commented Sep 3, 2019

The interesting thing is that I think you'd want a event coming in on xrPollEvent to actually wake-up the main-thread, since just adding a poll_events hook to run_one_frame is probably not enough, since it relies on something else waking up the main-thread.

You might have to integrate xrPollEvent somehow with the embedder event-loop, like is done with UI events, and wake-up the main-thread when one comes in.

@gterzian
Copy link
Member

gterzian commented Sep 3, 2019

Would it be bad to do that in wait_for_animation_frame?

In Chromium they poll events inside wait_for_animation_frame.

https://github.com/chromium/chromium/blob/f96c948c7311452b4452d0fb4fb2dacc595d0d73/device/vr/openxr/openxr_api_wrapper.cc#L414

One of the problem with wait_for_animation_frame is that it won't be called after Quit, but we still need to pull events afterward.

I can't quite make out what their Quit workflow is. They do catch the XR_SESSION_STATE_STOPPING session state as part of this OpenXrApiWrapper::ProcessEvents(), and as far as I can tell it's only called at "begin frame", the equivalent of "wait for animation frame".

@Manishearth
Copy link
Member

I think we should poll in rAF but perhaps have a wakeup heartbeat?

@gterzian
Copy link
Member

gterzian commented Sep 3, 2019

One of the problem with wait_for_animation_frame is that it won't be called after Quit, but we still need to pull events afterward.

I think the solution is to not actually stop the loop when quit is called at

fn quit(&mut self) {

And move the self.events.callback(Event::SessionEnd); to after calling xrEndSession.

In that case you wouldn't need a heartbeat since you'd continue running animation frames until the runtime itself moved into the XR_SESSION_STATE_STOPPING state.

If the application wishes to exit a session that is running but not in the XR_SESSION_STATE_STOPPING state, the application should call xrRequestExitSession. This requests that the runtime transition to the XR_SESSION_STATE_STOPPING state, so that the application can call xrEndSession, after which the session will transition through XR_SESSION_STATE_IDLE to the XR_SESSION_STATE_EXITING state and quit the XR experience seamlessly. https://www.khronos.org/registry/OpenXR/specs/1.0/html/xrspec.html#xrEndSession

I read that as saying you can't just stop your render loop when the application calls quit, instead you actually have to wait for the session to transition into the stopping state.

@jdm
Copy link
Member

jdm commented Sep 10, 2019

@Manishearth Are there any other examples of heartbeats with webxr code?

@Manishearth
Copy link
Member

Nope. For now just polling in rAF would be fine, it would probably make more sense to have any kind of manual event loop pumping once we have #30. Not pumping the event loop with a heartbeat event only affects sessions without rAF, which are quite rare.

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 a pull request may close this issue.

4 participants