Remove GL->d3d blit in HoloLens immersive mode #25855

jdm · 2020-02-27T21:49:50Z

Depends on:

These changes add two extra APIs for embedders to use when registering a WebXR device - one to allow running any closure as a task in the webgl thread, and one to register an arbitrary surface provider for a particular webxr session. When an openxr session is started, it can then obtain the webgl thread's d3d device from that thread's surfman device and ensure that openxr uses it.

Surface providers are traits that have their methods invoked by the webgl thread as part of the the normal swapchain operations. This allows the openxr surface provider to return surfaces that wrap the underlying openxr textures, which are valid in the webgl thread and can be used as the target of an opaque framebuffer.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #25735
These changes do not require tests because there are no windows immersive mode tests

highfive · 2020-02-27T21:49:57Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webglrenderingcontext.rs, components/script/dom/webglframebuffer.rs, components/script/dom/xrsession.rs
@KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/webglframebuffer.rs, components/script/dom/xrsession.rs

highfive · 2020-02-27T21:49:59Z

Warning

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

jdm · 2020-02-28T00:12:45Z

The good: in my testing of https://threejs.org/examples/webxr_vr_paint.html and https://threejs.org/examples/webxr_vr_ballshooter.html (and other content from https://servo.org/hl-home/), these changes significantly improve the framerate in my Windows MR VR headset.

The bad: every single babylon demo on that page starts off looking great (yay!) then triggers some kind of D3D error that makes it disconnect the underlying device.

asajeffrey

Some minor comments, mosly LGTM. My main concern about this is that it overlaps quite a bit with surfmanup, so we'll need to think about how to stage them.

asajeffrey · 2020-02-28T22:23:11Z

Cargo.toml

+#webxr = { path = "../webxr/webxr" }
+#webxr-api = { path = "../webxr/webxr-api" }
+webxr = { git = "https://github.com/jdm/webxr", branch = "invert" }
+webxr-api = { git = "https://github.com/jdm/webxr", branch = "invert" }


Get rid of these once the respective PRs merge.

asajeffrey · 2020-02-28T22:23:11Z

components/canvas/webgl_mode/inprocess.rs

@@ -27,8 +28,10 @@ use webxr_api::SwapChainId as WebXRSwapChainId;
 pub struct WebGLComm {
    pub webgl_threads: WebGLThreads,
    pub webxr_swap_chains: SwapChains<WebXRSwapChainId>,
+    pub webxr_surface_providers: SurfaceProviders,


I'd prefer an API that merged these two back together, but that can wait for now.

The swap chains and surface providers, you mean?

Yes, I suspect that would be tidier, but it's not an urgent change.

asajeffrey · 2020-02-28T22:23:11Z

components/canvas/webgl_mode/inprocess.rs

    pub image_handler: Box<dyn WebrenderExternalImageApi>,
    pub output_handler: Option<Box<dyn webrender_api::OutputImageHandler>>,
+    pub webgl_executor: WebGlExecutor,


Scary API for running unknown code! I'm confused where this is used.

This facilitates this code which extracts the d3d11 device pointer from the surfman device on the webgl thread.

OK, so as commented there if we could do this the other way round, and get the WebGL thread to use the OpenXR device, we could avoid this?

The webgl thread is created at servo startup. The openxr device is created when a page requests an XR session.

And I don't think we can't switch to using a different device later, because the page will have already created textures/framebuffers/etc. that won't be valid on a different device.

Oh that's annoying! IIRC the spec does allow the GL context to be invalidated when the XR session starts, presumably for exactly this reason.

jdm · 2020-02-28T22:46:30Z

Manish tried this on a HL2 and the numbers were not as impressive as my VR testing suggested. Alan theorizes that the change from one d3d texture per eye to one d3d texture for both eyes ends up taking a slow path on HL2 but not on the VR device. It should be possible to investigate that theory by applying the same change to master and running it on both HL2 and the Samsung Odyssey+.

jdm · 2020-03-02T19:02:23Z

I applied the following diff to webxr in an otherwise-unmodified master build, using https://servo.org/hl-home/bbjs/#JA1ND3#164 as the testcase:

diff --git a/webxr/openxr/mod.rs b/webxr/openxr/mod.rs
index 54accd7..0d8df03 100644
--- a/webxr/openxr/mod.rs
+++ b/webxr/openxr/mod.rs
@@ -143,9 +143,9 @@ struct OpenXrDevice {
     left_swapchain: Swapchain<D3D11>,
     left_image: u32,
     left_images: Vec<<D3D11 as Graphics>::SwapchainImage>,
-    right_swapchain: Swapchain<D3D11>,
+    /*right_swapchain: Swapchain<D3D11>,
     right_image: u32,
-    right_images: Vec<<D3D11 as Graphics>::SwapchainImage>,
+    right_images: Vec<<D3D11 as Graphics>::SwapchainImage>,*/
     surfman: (SurfmanDevice, SurfmanContext),
 
     // input
@@ -284,8 +284,7 @@ impl OpenXrDevice {
             usage_flags: SwapchainUsageFlags::COLOR_ATTACHMENT | SwapchainUsageFlags::SAMPLED,
             format,
             sample_count: 1,
-            // XXXManishearth what if the recommended widths are different?
-            width: left_view_configuration.recommended_image_rect_width,
+            width: left_view_configuration.recommended_image_rect_width + right_view_configuration.recommended_image_rect_width,
             height: left_view_configuration.recommended_image_rect_height,
             face_count: 1,
             array_size: 1,
@@ -298,12 +297,12 @@ impl OpenXrDevice {
         let left_images = left_swapchain
             .enumerate_images()
             .map_err(|e| Error::BackendSpecific(format!("{:?}", e)))?;
-        let right_swapchain = session
+        /*let right_swapchain = session
             .create_swapchain(&swapchain_create_info)
             .map_err(|e| Error::BackendSpecific(format!("{:?}", e)))?;
         let right_images = right_swapchain
             .enumerate_images()
-            .map_err(|e| Error::BackendSpecific(format!("{:?}", e)))?;
+            .map_err(|e| Error::BackendSpecific(format!("{:?}", e)))?;*/
 
         // input
 
@@ -339,11 +338,11 @@ impl OpenXrDevice {
             blend_mode,
             view_configurations,
             left_swapchain,
-            right_swapchain,
+            //right_swapchain,
             left_images,
-            right_images,
+            //right_images,
             left_image: 0,
-            right_image: 0,
+            //right_image: 0,
             surfman: surfman.extract(),
 
             action_set,
@@ -533,19 +532,19 @@ impl DeviceAPI<Surface> for OpenXrDevice {
         self.left_swapchain
             .wait_image(openxr::Duration::INFINITE)
             .unwrap();
-        self.right_image = self.right_swapchain.acquire_image().unwrap();
+        /*self.right_image = self.right_swapchain.acquire_image().unwrap();
         self.right_swapchain
             .wait_image(openxr::Duration::INFINITE)
-            .unwrap();
+            .unwrap();*/
 
         let left_image = self.left_images[self.left_image as usize];
-        let right_image = self.right_images[self.right_image as usize];
+        //let right_image = self.right_images[self.right_image as usize];
 
         let left_surface = unsafe {
             device
                 .create_surface_from_texture(
                     &context,
-                    &Size2D::new(size.width / 2, size.height),
+                    &Size2D::new(size.width /*/ 2*/, size.height),
                     left_image,
                 )
                 .expect("couldn't create left surface")
@@ -555,7 +554,7 @@ impl DeviceAPI<Surface> for OpenXrDevice {
             .expect("couldn't create left surface texture");
         let left_texture_id = left_surface_texture.gl_texture();
 
-        let right_surface = unsafe {
+        /*let right_surface = unsafe {
             device
                 .create_surface_from_texture(
                     &context,
@@ -567,7 +566,7 @@ impl DeviceAPI<Surface> for OpenXrDevice {
         let right_surface_texture = device
             .create_surface_texture(context, right_surface)
             .expect("couldn't create right surface texture");
-        let right_texture_id = right_surface_texture.gl_texture();
+        let right_texture_id = right_surface_texture.gl_texture();*/
 
         self.gl
             .bind_framebuffer(gl::DRAW_FRAMEBUFFER, self.write_fbo);
@@ -586,11 +585,11 @@ impl DeviceAPI<Surface> for OpenXrDevice {
         self.gl.blit_framebuffer(
             0,
             0,
-            size.width / 2,
+            size.width /*/ 2*/,
             size.height,
             0,
             size.height,
-            size.width / 2,
+            size.width /*/ 2*/,
             0,
             gl::COLOR_BUFFER_BIT,
             gl::NEAREST,
@@ -598,7 +597,7 @@ impl DeviceAPI<Surface> for OpenXrDevice {
         debug_assert_eq!(self.gl.get_error(), gl::NO_ERROR);
 
         // Bind the right eye's texture to the draw framebuffer.
-        self.gl.framebuffer_texture_2d(
+        /*self.gl.framebuffer_texture_2d(
             gl::DRAW_FRAMEBUFFER,
             gl::COLOR_ATTACHMENT0,
             device.surface_gl_texture_target(),
@@ -621,13 +620,13 @@ impl DeviceAPI<Surface> for OpenXrDevice {
         );
         debug_assert_eq!(self.gl.get_error(), gl::NO_ERROR);
 
-        self.gl.flush();
+        self.gl.flush();*/
 
         // Restore old GL bindings.
         self.gl.bind_framebuffer(gl::FRAMEBUFFER, old_framebuffer);
 
         self.left_swapchain.release_image().unwrap();
-        self.right_swapchain.release_image().unwrap();
+        //self.right_swapchain.release_image().unwrap();
         self.frame_stream
             .end(
                 self.frame_state.predicted_display_time,
@@ -654,10 +653,10 @@ impl DeviceAPI<Surface> for OpenXrDevice {
                             .fov(self.openxr_views[1].fov)
                             .sub_image(
                                 openxr::SwapchainSubImage::new()
-                                    .swapchain(&self.right_swapchain)
+                                    .swapchain(&self.left_swapchain)
                                     .image_array_index(0)
                                     .image_rect(openxr::Rect2Di {
-                                        offset: openxr::Offset2Di { x: 0, y: 0 },
+                                        offset: openxr::Offset2Di { x: self.left_extent.width, y: 0 },
                                         extent: self.right_extent,
                                     }),
                             ),
@@ -673,10 +672,10 @@ impl DeviceAPI<Surface> for OpenXrDevice {
             .unwrap();
         device.destroy_surface(context, left_surface).unwrap();
 
-        let right_surface = device
+        /*let right_surface = device
             .destroy_surface_texture(context, right_surface_texture)
             .unwrap();
-        device.destroy_surface(context, right_surface).unwrap();
+        device.destroy_surface(context, right_surface).unwrap();*/
 
         surface
     }

There was not a clear improvement in visual quality in the VR headset, and comparing against a build that avoided the GL->D3D blit there was a noticeable visual quality improvement in the latter. I have not yet had a chance to try this change in a HL2.

jdm · 2020-03-03T21:27:17Z

I can reproduce the results on HL2 where the performance difference is negligible. Profiling the paint demo immediately turns up:

webrender is taking up more than half of the the time that the main thread is running; we really need to finish off #25837 and this should disappear.
most of the time in the webgl thread is now spent under SwapChain::swap_buffers, and most of that is in Device::bind_surface_to_context where I suspect we end up taking the glFinish codepath (because the surfaces that wrap textures don't have keyed mutexes)
- I recall asking about keyed mutexes and the textures in the past, and we may be able to make use of fences instead

jdm · 2020-03-03T22:23:07Z

With #25837 applied, the paint demo FPS starts at 35 and eventually consistently reaches 50. It's not totally clear why there's a ramp up period; it would be worth looking at a profile that spans the full time period and comparing the stacks in the first 5-10 seconds versus the last 5-10 seconds.

jdm · 2020-03-03T22:27:08Z

With https://github.com/pcwalton/surfman/blob/ae14cc9afd140bc73e89f3fe8c080f729f09ccd2/surfman/src/platform/windows/angle/context.rs#L272 changed to use Flush instead, the FPS never drops below 45 at any point and usually sits at 50 \o/

asajeffrey · 2020-03-03T22:30:48Z

The relevant spec discussion is https://github.com/immersive-web/webxr/issues/968

asajeffrey · 2020-03-03T22:40:18Z

It's not obvious to me why Finish() or Flush() are needed there, I can imagine needing them for unbind (to make sure that any pending drawing on the surface has happened) but not bind.

jdm · 2020-03-03T22:45:10Z

The low framerate babylon demos only went up by 1 or 2 FPS with the finish->flush change, and profiles show the vast majority of time being spent in the script thread, unsurprisingly.

jdm · 2020-03-04T23:25:14Z

@Manishearth This branch should be full ready for testing.

bors-servo · 2020-03-05T16:07:05Z

☔ The latest upstream changes (presumably #25863) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2020-03-06T15:10:50Z

@bors-servo r=asajeffrey,manishearth

bors-servo · 2020-03-06T15:10:51Z

📌 Commit 5a9ae36 has been approved by asajeffrey,manishearth

bors-servo · 2020-03-09T19:50:21Z

💔 Test failed - status-taskcluster

jdm · 2020-03-09T20:00:10Z

@bors-servo retry

#25934

jdm · 2020-03-09T20:32:53Z

@bors-servo r-
Too many timeouts on mac webgl tests for comfort.


        Integrate swapchain surface provider changes into webgl and webxr imp…

…lementations.

jdm · 2020-03-09T21:51:19Z

@bors-servo r=Manishearth,asajeffrey

bors-servo · 2020-03-09T21:51:20Z

📌 Commit fbcf2bb has been approved by Manishearth,asajeffrey

jdm · 2020-03-09T21:51:38Z

@bors-servo try=wpt

bors-servo · 2020-03-09T21:51:44Z

⌛ Trying commit fbcf2bb with merge 5dbea4c...


        Auto merge of #25855 - jdm:surface-inversion, r=<try>

Remove GL->d3d blit in HoloLens immersive mode Depends on: * servo/surfman#151 * asajeffrey/surfman-chains#7 * servo/webxr#133 These changes add two extra APIs for embedders to use when registering a WebXR device - one to allow running any closure as a task in the webgl thread, and one to register an arbitrary surface provider for a particular webxr session. When an openxr session is started, it can then obtain the webgl thread's d3d device from that thread's surfman device and ensure that openxr uses it. Surface providers are traits that have their methods invoked by the webgl thread as part of the the normal swapchain operations. This allows the openxr surface provider to return surfaces that wrap the underlying openxr textures, which are valid in the webgl thread and can be used as the target of an opaque framebuffer. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25735 - [x] These changes do not require tests because there are no windows immersive mode tests

bors-servo · 2020-03-09T22:36:42Z

☀️ Test successful - status-taskcluster
State: approved=Manishearth,asajeffrey try=True

bors-servo · 2020-03-09T22:36:47Z

⌛ Testing commit fbcf2bb with merge 2d30e27...


        Auto merge of #25855 - jdm:surface-inversion, r=Manishearth,asajeffrey

Remove GL->d3d blit in HoloLens immersive mode Depends on: * servo/surfman#151 * asajeffrey/surfman-chains#7 * servo/webxr#133 These changes add two extra APIs for embedders to use when registering a WebXR device - one to allow running any closure as a task in the webgl thread, and one to register an arbitrary surface provider for a particular webxr session. When an openxr session is started, it can then obtain the webgl thread's d3d device from that thread's surfman device and ensure that openxr uses it. Surface providers are traits that have their methods invoked by the webgl thread as part of the the normal swapchain operations. This allows the openxr surface provider to return surfaces that wrap the underlying openxr textures, which are valid in the webgl thread and can be used as the target of an opaque framebuffer. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25735 - [x] These changes do not require tests because there are no windows immersive mode tests

Manishearth · 2020-03-09T23:12:35Z

@bors-servo try-

i feel like we fixed the sticky try bug, but either way...

jdm · 2020-03-09T23:58:18Z

@bors-servo retry

bors-servo · 2020-03-09T23:58:23Z

⌛ Testing commit fbcf2bb with merge b4d7ec1...

bors-servo · 2020-03-10T01:08:11Z

☀️ Test successful - status-taskcluster
Approved by: Manishearth,asajeffrey
Pushing b4d7ec1 to master...

highfive assigned asajeffrey Feb 27, 2020

highfive added the S-awaiting-review label Feb 27, 2020

jdm force-pushed the jdm:surface-inversion branch 3 times, most recently from 7eccb8c to 087866a Feb 27, 2020

jdm added the S-blocked-on-external label Feb 27, 2020

asajeffrey reviewed Feb 28, 2020

View changes

This was referenced Mar 3, 2020

Immersive mode doesn't reach 60fps #25425

Closed

three.js dragging demo is really slow in immersive mode #25897

Open

Gradient base in lorenz demo hurts FPS #25901

Closed

jdm force-pushed the jdm:surface-inversion branch from 087866a to f75db4d Mar 4, 2020

highfive added the S-needs-rebase label Mar 5, 2020

jdm force-pushed the jdm:surface-inversion branch 2 times, most recently from bccc1d5 to 5a9ae36 Mar 5, 2020

highfive removed the S-awaiting-review label Mar 6, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 9, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 9, 2020

Integrate swapchain surface provider changes into webgl and webxr imp…

Loading status checks…

fbcf2bb

…lementations.

jdm force-pushed the jdm:surface-inversion branch from 162db37 to fbcf2bb Mar 9, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Mar 9, 2020

highfive assigned Manishearth and unassigned asajeffrey Mar 9, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 9, 2020

bors-servo merged commit b4d7ec1 into servo:master Mar 10, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 10, 2020

servo / servo

Join GitHub today

Remove GL->d3d blit in HoloLens immersive mode #25855

Remove GL->d3d blit in HoloLens immersive mode #25855

Conversation

jdm commented Feb 27, 2020 • edited

highfive commented Feb 27, 2020

highfive commented Feb 27, 2020

jdm commented Feb 28, 2020 • edited

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 28, 2020

jdm commented Mar 2, 2020 • edited

jdm commented Mar 3, 2020 • edited

jdm commented Mar 3, 2020 • edited

jdm commented Mar 3, 2020

asajeffrey commented Mar 3, 2020

asajeffrey commented Mar 3, 2020

jdm commented Mar 3, 2020

jdm commented Mar 4, 2020

bors-servo commented Mar 5, 2020

jdm commented Mar 6, 2020

bors-servo commented Mar 6, 2020

bors-servo commented Mar 9, 2020

jdm commented Mar 9, 2020

jdm commented Mar 9, 2020

jdm commented Mar 9, 2020

bors-servo commented Mar 9, 2020

jdm commented Mar 9, 2020

bors-servo commented Mar 9, 2020

bors-servo commented Mar 9, 2020

bors-servo commented Mar 9, 2020

Manishearth commented Mar 9, 2020

jdm commented Mar 9, 2020

bors-servo commented Mar 9, 2020

bors-servo commented Mar 10, 2020

jdm commented Feb 27, 2020 •

edited

jdm commented Feb 28, 2020 •

edited

jdm commented Mar 2, 2020 •

edited

jdm commented Mar 3, 2020 •

edited

jdm commented Mar 3, 2020 •

edited