Skip to content

Commit

Permalink
Auto merge of #58 - paulrouget:shutdown, r=Manishearth
Browse files Browse the repository at this point in the history
More shutdown event processing

This fixes 2 different crashes at shutdown.

The initial approach of checking if session was running, then call RAF would fail as we would call `is_running()` before processing the event.

I got rid of the `is_running()` mechanic and instead made `wait_for_animation_frame` return an Option. Assuming that returning None mean the session has ended.

Fix #47
  • Loading branch information
bors-servo authored Sep 24, 2019
2 parents e13697c + 7d667e0 commit d4f585c
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 67 deletions.
6 changes: 1 addition & 5 deletions webxr-api/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,9 @@ pub trait Device: 'static {
Size2D::new(viewport.max_x(), viewport.max_y())
}

/// This method checks if the session has exited since last
/// wait_for_animation_frame call.
fn is_running(&self) -> bool;

/// This method should block waiting for the next frame,
/// and return the information for it.
fn wait_for_animation_frame(&mut self) -> Frame;
fn wait_for_animation_frame(&mut self) -> Option<Frame>;

/// This method should render a GL texture to the device.
/// While this method is being called, the device has unique access
Expand Down
14 changes: 8 additions & 6 deletions webxr-api/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,14 @@ impl<D: Device> SessionThread<D> {
}
SessionMsg::RequestAnimationFrame(dest) => {
let timestamp = self.timestamp;
if self.device.is_running() {
let frame = self.device.wait_for_animation_frame();
let _ = dest.send((timestamp, frame));
} else {
return false;
}
match self.device.wait_for_animation_frame() {
Some(frame) => {
let _ = dest.send((timestamp, frame));
}
None => {
return false;
}
};
}
SessionMsg::UpdateClipPlanes(near, far) => self.device.update_clip_planes(near, far),
SessionMsg::RenderAnimationFrame => {
Expand Down
13 changes: 3 additions & 10 deletions webxr/glwindow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ pub struct GlWindowDevice {
read_fbo: GLuint,
events: EventBuffer,
clip_planes: ClipPlanes,
is_running: bool,
}

impl Device for GlWindowDevice {
Expand All @@ -104,7 +103,7 @@ impl Device for GlWindowDevice {
Views::Stereo(left, right)
}

fn wait_for_animation_frame(&mut self) -> Frame {
fn wait_for_animation_frame(&mut self) -> Option<Frame> {
self.window.swap_buffers();
let translation = Vector3D::new(0.0, 0.0, -5.0);
let transform = RigidTransform3D::from_translation(translation);
Expand All @@ -113,11 +112,11 @@ impl Device for GlWindowDevice {
} else {
vec![]
};
Frame {
Some(Frame {
transform,
inputs: vec![],
events,
}
})
}

fn render_animation_frame(
Expand Down Expand Up @@ -178,12 +177,7 @@ impl Device for GlWindowDevice {
self.events.upgrade(dest)
}

fn is_running(&self) -> bool {
self.is_running
}

fn quit(&mut self) {
self.is_running = false;
self.events.callback(Event::SessionEnd);
}

Expand All @@ -210,7 +204,6 @@ impl GlWindowDevice {
read_fbo,
events: Default::default(),
clip_planes: Default::default(),
is_running: true,
})
}

Expand Down
14 changes: 3 additions & 11 deletions webxr/googlevr/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ pub(crate) struct GoogleVRDevice {
depth: bool,
clip_planes: ClipPlanes,
input: Option<GoogleVRController>,
is_running: bool,

#[cfg(target_os = "android")]
java_class: ndk::jclass,
Expand Down Expand Up @@ -99,7 +98,6 @@ impl GoogleVRDevice {
depth: false,
clip_planes: Default::default(),
input: None,
is_running: true,

ctx: ctx.get(),
controller_ctx: controller_ctx.get(),
Expand Down Expand Up @@ -141,7 +139,6 @@ impl GoogleVRDevice {
depth: false,
clip_planes: Default::default(),
input: None,
is_running: true,

ctx: ctx.get(),
controller_ctx: controller_ctx.get(),
Expand Down Expand Up @@ -532,7 +529,7 @@ impl Device for GoogleVRDevice {
}
}

fn wait_for_animation_frame(&mut self) -> Frame {
fn wait_for_animation_frame(&mut self) -> Option<Frame> {
unsafe {
self.acquire_frame();
}
Expand All @@ -542,11 +539,11 @@ impl Device for GoogleVRDevice {
vec![]
};
// Predict head matrix
Frame {
Some(Frame {
transform: self.fetch_head_matrix(),
inputs: self.input_state(),
events,
}
})
}

fn render_animation_frame(
Expand Down Expand Up @@ -582,14 +579,9 @@ impl Device for GoogleVRDevice {
self.events.upgrade(dest);
}

fn is_running(&self) -> bool {
self.is_running
}

fn quit(&mut self) {
self.stop_present();
self.events.callback(Event::SessionEnd);
self.is_running = false;
}

fn set_quitter(&mut self, _: Quitter) {
Expand Down
20 changes: 4 additions & 16 deletions webxr/headless/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ struct InputInfo {
struct HeadlessDevice {
gl: Rc<dyn Gl>,
data: Arc<Mutex<HeadlessDeviceData>>,
is_running: bool,
}

struct HeadlessDeviceData {
Expand Down Expand Up @@ -120,13 +119,7 @@ impl Discovery for HeadlessDiscovery {
}
let gl = self.gl.clone();
let data = self.data.clone();
xr.run_on_main_thread(move || {
Ok(HeadlessDevice {
gl,
data,
is_running: true,
})
})
xr.run_on_main_thread(move || Ok(HeadlessDevice { gl, data }))
}

fn supports_session(&self, mode: SessionMode) -> bool {
Expand All @@ -143,7 +136,7 @@ impl Device for HeadlessDevice {
self.data.lock().unwrap().views.clone()
}

fn wait_for_animation_frame(&mut self) -> Frame {
fn wait_for_animation_frame(&mut self) -> Option<Frame> {
let mut data = self.data.lock().unwrap();
let transform = data.viewer_origin;
let inputs = data
Expand All @@ -162,11 +155,11 @@ impl Device for HeadlessDevice {
} else {
vec![]
};
Frame {
Some(Frame {
transform,
inputs,
events,
}
})
}

fn render_animation_frame(&mut self, _: GLuint, _: Size2D<i32>, sync: Option<GLsync>) {
Expand All @@ -184,12 +177,7 @@ impl Device for HeadlessDevice {
self.data.lock().unwrap().events.upgrade(dest)
}

fn is_running(&self) -> bool {
self.is_running
}

fn quit(&mut self) {
self.is_running = false;
self.data.lock().unwrap().events.callback(Event::SessionEnd);
}

Expand Down
7 changes: 0 additions & 7 deletions webxr/magicleap/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ pub struct MagicLeapDevice {
frame_handle: MLHandle,
cameras: MLGraphicsVirtualCameraInfoArray,
view_update_needed: bool,
is_running: bool,
}

impl MagicLeapDiscovery {
Expand Down Expand Up @@ -165,7 +164,6 @@ impl MagicLeapDevice {
frame_handle,
cameras,
view_update_needed: false,
is_running: true,
};

// Rather annoyingly, in order for the views to be available, we have to
Expand Down Expand Up @@ -444,12 +442,7 @@ impl Device for MagicLeapDevice {
// TODO: handle events
}

fn is_running(&self) -> bool {
self.is_running
}

fn quit(&mut self) {
self.is_running = false;
// TODO: handle quit
}

Expand Down
28 changes: 16 additions & 12 deletions webxr/openxr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ struct OpenXrDevice {
resource: ComPtr<dxgi::IDXGIResource>,
device_context: ComPtr<d3d11::ID3D11DeviceContext>,
device: ComPtr<d3d11::ID3D11Device>,
is_running: bool,
}

impl OpenXrDevice {
Expand Down Expand Up @@ -261,11 +260,10 @@ impl OpenXrDevice {
resource,
device_context,
device,
is_running: true,
})
}

fn handle_openxr_events(&mut self) {
fn handle_openxr_events(&mut self) -> bool {
use openxr::Event::*;
loop {
let mut buffer = openxr::EventDataBuffer::new();
Expand All @@ -275,12 +273,18 @@ impl OpenXrDevice {
openxr::SessionState::STOPPING => {
self.events.callback(Event::SessionEnd);
self.session.end().unwrap();
self.is_running = false;
return false;
}
openxr::SessionState::EXITING | openxr::SessionState::LOSS_PENDING => {
break;
}
_ => {
// FIXME: Handle other states
}
},
Some(InstanceLossPending(_)) => {
break;
}
Some(_) => {
// FIXME: Handle other events
}
Expand All @@ -290,6 +294,7 @@ impl OpenXrDevice {
}
}
}
true
}
}

Expand Down Expand Up @@ -336,12 +341,11 @@ impl Device for OpenXrDevice {
Views::Stereo(left_view, right_view)
}

fn is_running(&self) -> bool {
self.is_running
}

fn wait_for_animation_frame(&mut self) -> Frame {
self.handle_openxr_events();
fn wait_for_animation_frame(&mut self) -> Option<Frame> {
if !self.handle_openxr_events() {
// Session is not running anymore.
return None;
}
self.frame_state = self.frame_waiter.wait().expect("error waiting for frame");
// XXXManishearth should we check frame_state.should_render?
let (_view_flags, views) = self
Expand All @@ -361,11 +365,11 @@ impl Device for OpenXrDevice {
} else {
vec![]
};
Frame {
Some(Frame {
transform,
inputs: vec![],
events,
}
})
}

fn render_animation_frame(
Expand Down

0 comments on commit d4f585c

Please sign in to comment.