WebVR API Implementation #14618

MortimerGoro · 2016-12-16T17:59:33Z

WebVR API Implementation with HTC Vive support on Windows. The current implementations only enables the WebVR support on Windows. In other platforms the API is available on JavaScript but navigator.vr.getDisplays() returns an empty array. This will change when we add support for more VR providers and platforms ;)

Info about the architecture:
https://blog.mozvr.com/webvr-servo-architecture-and-latency-optimizations/

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

Proprietary openvr.dll must be copied next to servo.exe in order to test on HTC Vive (https://github.com/ValveSoftware/openvr/tree/master/bin/win64) I have added some of the official WebVR samples for testing. Switch on your headset and run:

mach run tests/html/webvr/room-scale.html

This change is

highfive · 2016-12-16T17:59:39Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/lib.rs, components/constellation/pipeline.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs
@KiChjang: components/script/dom/webidls/VRDisplayCapabilities.webidl, components/script/dom/navigator.rs, components/script/dom/vrdisplay.rs, components/script/dom/webidls/VRFrameData.webidl, components/script/dom/mod.rs, components/script/dom/vr.rs, components/script/dom/vrdisplaycapabilities.rs, components/script/dom/vrstageparameters.rs, components/script/dom/vrdisplayevent.rs, components/script/dom/window.rs, components/script/dom/webidls/VREyeParameters.webidl, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/lib.rs, components/script/dom/webidls/Window.webidl, components/script/dom/vreyeparameters.rs, components/script/dom/webidls/VRStageParameters.webidl, components/script/dom/vrframedata.rs, components/script/dom/macros.rs, components/script/dom/vrpose.rs, components/script/dom/webidls/Navigator.webidl, components/script/dom/webidls/VR.webidl, components/script/dom/webidls/VRPose.webidl, components/script/Cargo.toml, components/script/dom/webidls/VRDisplay.webidl, components/script/dom/webidls/VRDisplayEvent.webidl, components/script/script_thread.rs, components/script/script_runtime.rs, components/script/dom/vrfieldofview.rs, components/script/dom/webidls/VRFieldOfView.webidl, components/script/dom/bindings/conversions.rs, components/script/dom/webidls/VRLayer.webidl, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml
@fitzgen: components/script/dom/webidls/VRDisplayCapabilities.webidl, components/script/dom/navigator.rs, components/script/dom/vrdisplay.rs, components/script/dom/webidls/VRFrameData.webidl, components/script/dom/mod.rs, components/script/dom/vr.rs, components/script/dom/vrdisplaycapabilities.rs, components/script/dom/vrstageparameters.rs, components/script/dom/vrdisplayevent.rs, components/script/dom/window.rs, components/script/dom/webidls/VREyeParameters.webidl, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/lib.rs, components/script/dom/webidls/Window.webidl, components/script/dom/vreyeparameters.rs, components/script/dom/webidls/VRStageParameters.webidl, components/script/dom/vrframedata.rs, components/script/dom/macros.rs, components/script/dom/vrpose.rs, components/script/dom/webidls/Navigator.webidl, components/script/dom/webidls/VR.webidl, components/script/dom/webidls/VRPose.webidl, components/profile/time.rs, components/script/Cargo.toml, components/script/dom/webidls/VRDisplay.webidl, components/profile_traits/time.rs, components/profile_traits/time.rs, components/script/dom/webidls/VRDisplayEvent.webidl, components/script/script_thread.rs, components/script/script_runtime.rs, components/script/dom/vrfieldofview.rs, components/script/dom/webidls/VRFieldOfView.webidl, components/script/dom/bindings/conversions.rs, components/script/dom/webidls/VRLayer.webidl, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml
@emilio: components/canvas/webgl_paint_thread.rs

highfive · 2016-12-16T17:59:40Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

emilio

Just some mostly-superficial comments, since @larsbergstrom has kindly offered to take over the review.

I guess adding automatic tests for this is not truly feasible? Is there a plan to test this stuff?

emilio · 2016-12-16T18:23:03Z

components/script/script_thread.rs

@@ -2197,6 +2207,13 @@ impl ScriptThread {
        }
    }

+    fn handle_webvr_event(&self, pipeline_id: PipelineId, event: WebVREventMsg) {
+        if let Some(window) = self.documents.borrow().find_window(pipeline_id) {


Use a different variable for this, see #14589.

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+        sender
+    }
+
+    #[allow(unsafe_code)]


there doesn't seem to be unsafe code here?

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+
+    fn poll_events(&mut self, sender: IpcSender<bool>) {
+        let events = self.service.poll_events();
+        if events.len() > 0 {


if !events.is_empty()

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+    }
+
+    fn schedule_poll_events(&mut self) {
+        if self.service.is_initialized() && !self.polling_events {


nit: Probably nicer to read as:

if !self.service.is_initialized() || self.polling_events { return; } // ...

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+                        // WebVR Thread asked to unschedule this thread
+                        break;
+                    }
+                    thread::sleep(time::Duration::from_millis(500));


This seems quite arbitrary. I understand why it's this way, but probably worth moving the 500 to a constant.

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+// This also avoids "JS DDoS" attacks: A Second JSContext doing a lot of calls.
+// while the main one is presenting and demands both high framerate and low latency.
+
+pub struct WebVRCompositor(*mut VRDevice);


huh, this is going to need a better comment explaining how it's used and why it's safe and not racy.

Also, would be helpful to know when this VRDevice can be null or not, and how does is this VRDevice freed, since I don't see any Drop impl.

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+                        (*compositor.0).sync_poses();
+                        (*compositor.0).synced_frame_data(near, far).to_bytes()
+                    };
+                    let _result = sender.send(Ok(pose));


The patterns let _ = sender.send(..) or drop(sender.send()) seem more used across the codebase.

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+impl WebVRCompositorHandler {
+    #[allow(unsafe_code)]
+    fn create_compositor(&mut self, device_id: webrender_traits::VRCompositorId) {
+        if let Some(ref sender) = self.webvr_thread_sender {


Probably worth:

let sender = match self.webvr_thread_sender { Some(ref s) => s, None => return, };

emilio · 2016-12-16T18:23:03Z

components/vr/webvr_thread.rs

+        }
+    }
+
+    pub fn set_webvr_thread_sender(&mut self, sender: IpcSender<WebVRMsg>) {


Worth pointing here with a comment that this is done only a per-platform basis on initialization.

asajeffrey · 2016-12-16T20:11:41Z

components/constellation/pipeline.rs

@@ -119,6 +120,8 @@ pub struct InitialPipelineState {
    pub webrender_api_sender: webrender_traits::RenderApiSender,
    /// Whether this pipeline is considered private.
    pub is_private: bool,
+    /// A channel to the webvr thread.
+    pub webvr_thread: Option<IpcSender<WebVRMsg>>,


When is this used?

It's used to create the ScriptThreadFactory. When a script thread is created it gets a cloned handle to the webvr_thread sender. This sender is used by DOMObjects to comunicate with the WebVRThread and send messages like: getDisplays, requestPresent, etc.

MortimerGoro · 2016-12-16T22:47:26Z

@emilio WRT automated testing it could be done with mocked devices, but it requires more work: like setting up predefined pose animations and comparing images submitted to the display. I have an initial implementation of a MockVRDevice and @dmarcos was working on a mock openvr_api.dll. @dmarcos is there any plan for official WebVR conformance tests? Maybe you have talked about that in the latest W3C workshop/meetings.

dmarcos · 2016-12-16T23:33:30Z

@MortimerGoro @emilio No official plan for conformance tests. There's a face to face meeting scheduled in January that will touch the subject (among other things). I think in the short we can use your MockVRDevice. In the mid term and for cross browser testing will be better to implement a mock OpenVR driver (instead of the full OpenVR API implementation I had started). In the long term we will probably move to the driver standard that is supposed to come from the khronos vr effort.

Ms2ger · 2016-12-17T15:01:42Z

Seems like this should all be behind a pref.

MortimerGoro · 2016-12-17T18:30:39Z

@Ms2ger Sounds good, I'll add a new pref to https://github.com/servo/servo/blob/master/components/config/opts.rs that decides if the WebVR API is enabled.

jdm · 2016-12-17T19:15:24Z

@MortimerGoro It belongs in resources/prefs.json and annotated in the WebIDL files using [Pref=] instead of a command-line option.

MortimerGoro · 2016-12-17T20:02:28Z

@jdm Thanks for the info, never had played with that json before ;)

nox

Started looking at it a bit.

nox · 2016-12-18T17:20:12Z

components/script/dom/bindings/conversions.rs

@@ -463,6 +466,9 @@ pub unsafe trait ArrayBufferViewContents: Clone {
    /// Check if the JS ArrayBufferView type is compatible with the implementor of the
    /// trait
    fn is_type_compatible(ty: Type) -> bool;
+
+    /// Creates a typed array
+    fn new(cx: *mut JSContext, num: u32) -> *mut JSObject;


This should be unsafe.

nox · 2016-12-18T17:20:12Z

components/script/dom/bindings/conversions.rs

@@ -595,3 +633,25 @@ pub unsafe fn is_array_like(cx: *mut JSContext, value: HandleValue) -> bool {
    assert!(JS_IsArrayObject(cx, value, &mut result));
    result
 }
+
+/// Creates a typed JS array from a Rust slice
+pub fn slice_to_array_buffer_view<T>(cx: *mut JSContext, data: &[T]) -> *mut JSObject


This should be unsafe.

nox · 2016-12-18T17:20:12Z

components/script/dom/navigator.rs

+
+impl Navigator {
+     pub fn handle_webvr_event(&self, event: WebVREventMsg) {
+         if let Some(vr) = self.vr.get() {


How can this be called with self.vr being None?

nox · 2016-12-18T17:20:12Z

components/script/dom/vr.rs

+            return promise;
+        }
+
+        if let Some(wevbr_sender) = self.webvr_thread() {


Spelling of webvr_sender is wrong, and it is weird to call the variable sender when the method says thread.

nox · 2016-12-18T17:20:12Z

components/script/dom/vrdisplaycapabilities.rs

+    capabilities: DOMRefCell<WebVRDisplayCapabilities>
+}
+
+// Wrappers required to include WebVR structs in a DOM struct


nox · 2016-12-18T17:20:12Z

components/script/dom/vrdisplayevent.rs

+
+impl VRDisplayEvent {
+    fn new_inherited(display: &VRDisplay,
+                     reason: &Option<VRDisplayEventReason>)


Why a ref? Pass Option<VRDisplayEventReason>.

nox · 2016-12-18T17:20:12Z

components/script/dom/vrdisplayevent.rs

+                          display: &VRDisplay,
+                          event: &webvr::VRDisplayEvent)
+                          -> Root<VRDisplayEvent> {
+        let (name, reason) = match event {


Nit: match *event.

nox · 2016-12-18T17:20:12Z

components/script/dom/vreyeparameters.rs

+    fov: JS<VRFieldOfView>,
+}
+
+// Wrappers required to include WebVR structs in a DOM struct


I added the wrappers because Rust didn't allow to implement no_jsmanaged_fields macro or JSTraceable for structs defined in a different crate. I have done a test with the latest rustc compiler used in Servo and it seems to work now. I'll get rid of them now ;)

nox · 2016-12-18T17:20:12Z

components/script/dom/vrdisplayevent.rs

+        let (name, reason) = match event {
+            &webvr::VRDisplayEvent::Connect(_) => ("displayconnect", None),
+            &webvr::VRDisplayEvent::Disconnect(_) => ("displaydisconnect", None),
+            &webvr::VRDisplayEvent::Activate(_, ref reason) => ("activate", Some(reason)),


Why ref? Seems to me like VRDisplayEventReason should be Clone and Copy.

nox · 2016-12-18T17:20:12Z

components/script/dom/vreyeparameters.rs

+
+impl VREyeParameters {
+    #[allow(unrooted_must_root)]
+    fn new_inherited(parameters: &webvr::VREyeParameters, global: &GlobalScope) -> VREyeParameters {


Why a reference?

cvan

just feedback from having worked on the spec and helped Kip with Gecko implementation + bugs in the past. I'm not familiar with Rust, so take my webdev ergonomics POV with a grain of salt.

I'd like to iron out the event issues, but all means if the reviewers here okay this PR, feel free to proceed.

truly great work - this is really inspiring to see.

cvan · 2016-12-19T13:58:52Z

components/constellation/constellation.rs

+                self.webvr_thread = Some(webvr_thread)
+            }
+            FromCompositorMsg::WebVREvent(pipeline_ids, event) => {
+                debug!("constellation got webvr event message");


is the message part necessary?

Also, if we go this spell-checking mode, you may want to capitalize WebVR :P

cvan · 2016-12-19T13:58:52Z

components/constellation/constellation.rs

@@ -1181,6 +1195,13 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
            }
        }

+        if let Some(chan) = self.webvr_thread.as_ref() {
+            debug!("Exiting WebVR thread.");


should this be logged outside the if check (à la above)?

I included it inside the if because WebVR can be disabled and it didn't make sense to show that message in that case. I checked and the same pattern is used in ""Exiting devtools" too ;)

cvan · 2016-12-19T13:58:52Z

components/profile/time.rs

@@ -151,6 +151,7 @@ impl Formattable for ProfilerCategory {
            ProfilerCategory::ScriptServiceWorkerEvent => "Script Service Worker Event",
            ProfilerCategory::ScriptEnterFullscreen => "Script Enter Fullscreen",
            ProfilerCategory::ScriptExitFullscreen => "Script Exit Fullscreen",
+            ProfilerCategory::ScriptWebVREvent => "Script Web VR Event",


one word: WebVR

cvan · 2016-12-19T13:58:52Z

components/script/dom/macros.rs

@@ -495,6 +495,13 @@ macro_rules! window_event_handlers(
        event_handler!(unhandledrejection, GetOnunhandledrejection,
                       SetOnunhandledrejection);
        event_handler!(unload, GetOnunload, SetOnunload);
+        event_handler!(vrdisplayconnect, GetOnvrdisplayconnect, SetOnvrdisplayconnect);


I couldn't find one, but is there an issue no track to move these events from window to the VRDisplay interface?

(See immersive-web/webxr#152.)

but then I see you're implementing navigator.vr.getDisplays instead of navigator.getVRDisplays(). these events should be emitted from the VRDisplay instances not off of window.

Yes, I'll remove these ones. I started implementing 1.1 API but moved to 1.2. These events are already moved to VRDisplay instances (in the implementation and samples)

cvan · 2016-12-19T13:58:52Z

components/script/dom/vr.rs

+        self.displays.borrow()
+                     .iter()
+                     .find(|d| d.get_display_id() == display_id)
+                     .map(|d| Root::from_ref(&**d))


intentionally no semicolons here?

Yes, because it's a implicit return

cvan · 2016-12-19T13:58:52Z

components/vr_traits/Cargo.toml

+serde_derive = "0.8"
+rust-webvr = {version = "0.1", features = ["serde-serialization"] }
+
+


feel free to remove these two superfluous newlines, but keep a single one at EOF

cvan · 2016-12-19T13:58:52Z

components/vr_traits/Cargo.toml

+msg = {path = "../msg"}
+serde = "0.8"
+serde_derive = "0.8"
+rust-webvr = {version = "0.1", features = ["serde-serialization"] }


can remove the space before the closing curly brace at the end: }

cvan · 2016-12-19T13:58:52Z

components/vr_traits/webvr_traits.rs

+
+pub type WebVRResult<T> = Result<T, String>;
+
+// Messages from Script Thread to WebVR thread.


Script Thread -> Script thread

cvan · 2016-12-19T13:58:52Z

components/script/dom/vr.rs

+
+        if let Some(wevbr_sender) = self.webvr_thread() {
+            let (sender, receiver) = ipc::channel().unwrap();
+            wevbr_sender.send(WebVRMsg::GetVRDisplays(sender)).unwrap();


also: GetDisplays

cvan · 2016-12-19T13:58:52Z

components/vr_traits/webvr_traits.rs

+    RegisterContext(PipelineId),
+    UnregisterContext(PipelineId),
+    PollEvents(IpcSender<bool>),
+    GetVRDisplays(IpcSender<WebVRResult<Vec<VRDisplayData>>>),


same here: GetDisplays

emilio

I took a closer look, here are a few comments :)

emilio · 2016-12-19T15:02:35Z

components/canvas/canvas_paint_thread.rs

@@ -209,6 +209,7 @@ impl<'a> CanvasPaintThread<'a> {
                        }
                    }
                    CanvasMsg::WebGL(_) => panic!("Wrong message sent to Canvas2D thread"),
+                    CanvasMsg::WebVR(_) => panic!("Wrong message sent to Canvas2D thread"),


It'd be nice to differentiate these error messages now.

emilio · 2016-12-19T15:02:35Z

components/canvas/webgl_paint_thread.rs

+            WebGLPaintTaskData::WebRender(ref api, id) => {
+                api.send_vr_compositor_command(id, message);
+            }
+            WebGLPaintTaskData::Readback(_, _, _) => {


nit: You can use WebGLPaintTaskData::Readback(..) if you want.

emilio · 2016-12-19T15:02:35Z

components/constellation/constellation.rs

@@ -872,6 +879,13 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
            FromCompositorMsg::LogEntry(top_level_frame_id, thread_name, entry) => {
                self.handle_log_entry(top_level_frame_id, thread_name, entry);
            }
+            FromCompositorMsg::SetWebVRThread(webvr_thread) => {
+                self.webvr_thread = Some(webvr_thread)


Probably is worth sanity-checking with something like: assert!(self.webvr_thread.is_none())

emilio · 2016-12-19T15:02:35Z

components/constellation/constellation.rs

+                self.webvr_thread = Some(webvr_thread)
+            }
+            FromCompositorMsg::WebVREvent(pipeline_ids, event) => {
+                debug!("constellation got webvr event message");


Also, if we go this spell-checking mode, you may want to capitalize WebVR :P

emilio · 2016-12-19T15:02:35Z

components/script/dom/navigator.rs

+    fn Vr(&self) -> Root<VR> {
+        self.vr.or_init(|| VR::new(&self.global()))
+    }
+


nit: Extra newline.

emilio · 2016-12-19T15:02:35Z

components/script/dom/vrpose.rs

+
+    pub fn new(global: &GlobalScope, pose: &webvr::VRPose) -> Root<VRPose> {
+        let root = reflect_dom_object(box VRPose::new_inherited(),
+                           global,


nit: Indentation

emilio · 2016-12-19T15:02:35Z

components/script/dom/webidls/VRLayer.webidl

+
+// https://w3c.github.io/webvr/#interface-vrlayer
+
+//typedef (HTMLCanvasElement or OffscreenCanvas) VRSource;


Thanks for filling that :)

It's probably fine to add typedef HTMLCanvasElement VRSource for now, but I don't think it's worth it.

emilio · 2016-12-19T15:02:35Z

components/vr/Cargo.toml

+default_features = false
+features = ["serde_derive"]
+
+


Yeah, please delete these trailing newlines.

emilio · 2016-12-19T15:02:35Z

components/vr/webvr_thread.rs

+use vr_traits::webvr::*;
+use webrender_traits;
+
+// Defines the polling interval time in ms for VR Events such as VRDevice connected, disconnected, etc.


Also, I think you can use doc comments for these.

emilio · 2016-12-19T15:02:35Z

components/vr/webvr_thread.rs

+//    * WebVRCompositorHandler is stopped automatically when a JS tab is closed or the whole browser is closed.
+//    * WebVRThread and it's VRDevices are destroyed after all tabs are dropped and the browser is about to exit.
+//      WebVRThread is closed using the Exit message.
+


Doc comment above, then remove this newline :)

MortimerGoro · 2016-12-20T17:27:51Z

@nox @jdm @emilio @larsbergstrom @cvan @Ms2ger Thanks for the feedback. I have updated the PR:

Everything is behind the dom.webvr.enabled preference now
Used expect instead of if let Some() in places where the webvr_thread is guaranteed to be enabled
Renamed VRDevice to VRDisplay
Fixed nits
There are two trivial TODOs (getLayers and activeDisplays functions) because of a issue with the python WebIDL code autogeneration tool: it crashes when using readonly attribute sequence<T> pattern. I'll fill a issue about this.

larsbergstrom · 2016-12-21T17:04:41Z

components/canvas/canvas_paint_thread.rs

@@ -208,7 +208,8 @@ impl<'a> CanvasPaintThread<'a> {
                            }
                        }
                    }
-                    CanvasMsg::WebGL(_) => panic!("Wrong message sent to Canvas2D thread"),
+                    CanvasMsg::WebGL(_) => panic!("Wrong WebGLmessage sent to Canvas2D thread"),


nit: space between WebGL and message

larsbergstrom · 2016-12-21T17:04:41Z

components/script/dom/vrpose.rs

+    #[allow(unsafe_code)]
+    // https://w3c.github.io/webvr/#dom-vrpose-linearacceleration
+    unsafe fn GetLinearAcceleration(&self, _cx: *mut JSContext) -> Option<NonZero<*mut JSObject>> {
+        heap_to_option(&self.linear_vel)


Should this be &self.linear_acc?

larsbergstrom · 2016-12-21T17:04:41Z

components/script/script_thread.rs

@@ -473,6 +475,8 @@ pub struct ScriptThread {
    content_process_shutdown_chan: IpcSender<()>,

    promise_job_queue: PromiseJobQueue,
+    /// A handle to the webvr thread, if available


nit: blank line before this comment (they are roughly separated by "area")

larsbergstrom · 2016-12-21T17:04:41Z

components/script_traits/lib.rs

@@ -284,7 +289,8 @@ impl fmt::Debug for ConstellationControlMsg {
            DispatchStorageEvent(..) => "DispatchStorageEvent",
            FramedContentChanged(..) => "FramedContentChanged",
            ReportCSSError(..) => "ReportCSSError",
-            Reload(..) => "Reload"
+            Reload(..) => "Reload",
+            WebVREvent(..) => "WebVREvent"


nit: we usually keep the trailing , in the final line of a match. It helps both with consistency and if you add more entries to the end, as the diff is one line shorter :-)

larsbergstrom · 2016-12-21T17:04:41Z

resources/prefs.json

@@ -7,6 +7,7 @@
  "dom.serviceworker.timeout_seconds": 60,
  "dom.testable_crash.enabled": false,
  "dom.testbinding.enabled": false,
+  "dom.webvr.enabled": true,


This should be false by default

larsbergstrom · 2016-12-21T17:04:41Z