XR framebuffer creation panics when setting up texture

[jdm]

When running the three.js webvr rollercoaster example in immersive, I get GL errors when we try to bind the texture that we've just created as part of creating an XRWebGLLayer. My theory is that the webgl commands end up being interleaved with other GL processing from webrender in such a way that the state we rely upon is messed up. I have a patch to move these operations to run in the webgl thread in an atomic unit as part of a single WebGLCommand, and this makes the GL errors go away.

[asajeffrey]

Hmm, that shouldn't be happening. There shouldn't be any difference between issuing the WebGL commands directly in the webgl thread, compared to issuing them in the script thread and sending them to the webgl thread for execution.

[asajeffrey]

IRC chat https://mozilla.logbot.info/servo/20190828#c16574986

[jdm]

There is a difference when the webgl thread is shared with the webrender thread - any synchronous GL API (like CreateTexture) will cause the current set of messages to be processed, allowing arbitrary WR GL calls to execute before the next set of GL messages gets processed.

[jdm]

The real underlying bug may be that there is some GL code in WR or webxr that modifies the state and doesn't restore what was there before. It's hard to tell.

[asajeffrey]

Ah, that makes sense. WebGL maintains a lot of state that assumes it's the only owner of the GL context. This is a deeper problem than just XR initialization, any user WebGL code could hit the same problem.

[jdm]

Indeed :/

[jdm]

Patch that needs some cleaning up:

diff --git a/components/canvas/webgl_thread.rs b/components/canvas/webgl_thread.rs
index 7844c762cfbb..9fe8e57d680d 100644
--- a/components/canvas/webgl_thread.rs
+++ b/components/canvas/webgl_thread.rs
@@ -887,6 +887,60 @@ impl WebGLImpl {
         _backtrace: WebGLCommandBacktrace,
     ) {
         match command {
+            WebGLCommand::CreateXRWebGLLayer(ref resolution, ref sender) => {
+                let gl = ctx.gl();
+
+                let mut value = [0];
+                unsafe {
+                    gl.get_integer_v(gl::TEXTURE_BINDING_2D, &mut value);
+                }
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                let old_texture = value[0] as gl::GLuint;
+                unsafe {
+                    gl.get_integer_v(gl::FRAMEBUFFER_BINDING, &mut value);
+                }
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                let old_framebuffer = value[0] as gl::GLuint;
+                
+                let texture = gl.gen_textures(1)[0];
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                gl.bind_texture(gl::TEXTURE_2D, texture);
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                gl.tex_image_2d(
+                    gl::TEXTURE_2D,
+                    0,
+                    gl::RGBA as gl::GLint,
+                    resolution.width,
+                    resolution.height,
+                    0,
+                    gl::RGBA,
+                    gl::UNSIGNED_BYTE,
+                    None,
+                );
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+
+                let framebuffer = gl.gen_framebuffers(1)[0];
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                gl.bind_framebuffer(gl::FRAMEBUFFER, framebuffer);
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                gl.framebuffer_texture_2d(
+                    gl::FRAMEBUFFER,
+                    gl::COLOR_ATTACHMENT0,
+                    gl::TEXTURE_2D,
+                    texture,
+                    0,
+                );
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                
+                gl.bind_texture(gl::TEXTURE_2D, old_texture);
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                gl.bind_framebuffer(gl::FRAMEBUFFER, old_framebuffer);
+				assert_eq!(gl.get_error(), gl::NO_ERROR);
+                unsafe {
+                    let _ = sender.send(Ok((WebGLFramebufferId::new(framebuffer), WebGLTextureId::new(texture))));
+                }
+            }
             WebGLCommand::GetContextAttributes(ref sender) => sender
                 .send(map_attrs_to_script_attrs(*ctx.borrow_attributes()))
                 .unwrap(),
diff --git a/components/canvas_traits/webgl.rs b/components/canvas_traits/webgl.rs
index a037b5288747..e725b6d6703e 100644
--- a/components/canvas_traits/webgl.rs
+++ b/components/canvas_traits/webgl.rs
@@ -3,6 +3,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 use euclid::default::{Rect, Size2D};
+use euclid::Size2D as TypedSize2D;
 use gleam::gl;
 use gleam::gl::Gl;
 use ipc_channel::ipc::{IpcBytesReceiver, IpcBytesSender, IpcSharedMemory};
@@ -213,6 +214,9 @@ impl<T> Deref for TruncatedDebug<T> {
 /// WebGL Commands for a specific WebGLContext
 #[derive(Debug, Deserialize, Serialize)]
 pub enum WebGLCommand {
+    CreateXRWebGLLayer(TypedSize2D<i32, webxr_api::Viewport>, WebGLSender<Result<(WebGLFramebufferId, WebGLTextureId), ()>>),
     GetContextAttributes(WebGLSender<GLContextAttributes>),
     ActiveTexture(u32),
     BlendColor(f32, f32, f32, f32),
diff --git a/components/script/dom/webglframebuffer.rs b/components/script/dom/webglframebuffer.rs
index 114ede38b01b..faf9c720c856 100644
--- a/components/script/dom/webglframebuffer.rs
+++ b/components/script/dom/webglframebuffer.rs
@@ -13,9 +13,11 @@ use crate::dom::webglobject::WebGLObject;
 use crate::dom::webglrenderbuffer::WebGLRenderbuffer;
 use crate::dom::webglrenderingcontext::WebGLRenderingContext;
 use crate::dom::webgltexture::WebGLTexture;
+use crate::dom::webgl_validations::types::TexImageTarget;
 use canvas_traits::webgl::{webgl_channel, WebGLError, WebGLResult};
-use canvas_traits::webgl::{WebGLCommand, WebGLFramebufferBindingRequest, WebGLFramebufferId};
+use canvas_traits::webgl::{TexFormat, TexDataType, WebGLCommand, WebGLFramebufferBindingRequest, WebGLFramebufferId, WebGLTextureId};
 use dom_struct::dom_struct;
+use euclid::default::Size2D;
 use std::cell::Cell;
 
 pub enum CompleteForRendering {
@@ -118,6 +120,29 @@ impl WebGLFramebuffer {
             WebGLFramebufferBinding::Wrap,
         )
     }
+    
+    pub(crate) fn new_with_color_attachment(
+        context: &WebGLRenderingContext,
+        id: WebGLFramebufferId,
+        texture: WebGLTextureId,
+        size: Size2D<u32>,
+    ) -> DomRoot<Self> {
+        let fb = reflect_dom_object(
+            Box::new(WebGLFramebuffer::new_inherited(context, id)),
+            &*context.global(),
+            WebGLFramebufferBinding::Wrap,
+        );
+        let texture = WebGLTexture::new(context, texture);
+        texture.init_as_if_bound(constants::TEXTURE_2D).unwrap();
+        texture.initialize(TexImageTarget::Texture2D, size.width, size.height, 1, TexFormat::RGBA, 0, Some(TexDataType::UnsignedByte)).unwrap();
+        *fb.color.borrow_mut() = Some(WebGLFramebufferAttachment::Texture {
+            texture: Dom::from_ref(&*texture),
+            level: 0,
+        });
+        //fb.status.set(constants::FRAMEBUFFER_COMPLETE);
+        fb.update_status();
+        fb
+    }
 }
 
 impl WebGLFramebuffer {
diff --git a/components/script/dom/webgltexture.rs b/components/script/dom/webgltexture.rs
index a7e548dcdefc..b4a2d7b8a9e9 100644
--- a/components/script/dom/webgltexture.rs
+++ b/components/script/dom/webgltexture.rs
@@ -88,6 +88,17 @@ impl WebGLTexture {
     pub fn id(&self) -> WebGLTextureId {
         self.id
     }
+    
+    pub(crate) fn init_as_if_bound(&self, target: u32) -> WebGLResult<()> {
+        let face_count = match target {
+            constants::TEXTURE_2D => 1,
+            constants::TEXTURE_CUBE_MAP => 6,
+            _ => return Err(WebGLError::InvalidEnum),
+        };
+        self.face_count.set(face_count);
+        self.target.set(Some(target));
+        Ok(())
+    }
 
     // NB: Only valid texture targets come here
     pub fn bind(&self, target: u32) -> WebGLResult<()> {
@@ -101,13 +112,7 @@ impl WebGLTexture {
             }
         } else {
             // This is the first time binding
-            let face_count = match target {
-                constants::TEXTURE_2D => 1,
-                constants::TEXTURE_CUBE_MAP => 6,
-                _ => return Err(WebGLError::InvalidEnum),
-            };
-            self.face_count.set(face_count);
-            self.target.set(Some(target));
+            self.init_as_if_bound(target)?;
         }
 
         self.upcast::<WebGLObject>()
diff --git a/components/script/dom/xrwebgllayer.rs b/components/script/dom/xrwebgllayer.rs
index 713546e1129c..84a37e03a854 100644
--- a/components/script/dom/xrwebgllayer.rs
+++ b/components/script/dom/xrwebgllayer.rs
@@ -2,6 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
+use canvas_traits::webgl::{WebGLCommand, TexFormat, TexDataType};
+use canvas_traits::webgl::webgl_channel;
 use crate::dom::bindings::codegen::Bindings::XRViewBinding::{XREye, XRViewMethods};
 use crate::dom::bindings::codegen::Bindings::XRWebGLLayerBinding;
 use crate::dom::bindings::codegen::Bindings::XRWebGLLayerBinding::XRWebGLLayerInit;
@@ -21,6 +23,7 @@ use crate::dom::xrsession::XRSession;
 use crate::dom::xrview::XRView;
 use crate::dom::xrviewport::XRViewport;
 use dom_struct::dom_struct;
+use euclid::default::Size2D;
 use js::rust::CustomAutoRooter;
 use std::convert::TryInto;
 use webxr_api::Views;
@@ -90,20 +93,30 @@ impl XRWebGLLayer {
         // XXXManishearth step 4: check XR compat flag for immersive sessions
 
         let cx = global.get_cx();
-        let old_fbo = context.bound_framebuffer();
+        /*let old_fbo = context.bound_framebuffer();
         let old_texture = context
             .textures()
-            .active_texture_for_image_target(TexImageTarget::Texture2D);
+            .active_texture_for_image_target(TexImageTarget::Texture2D);*/
+            
+        let resolution = session.with_session(|s| s.recommended_framebuffer_resolution());
+        let (sender, receiver) = webgl_channel().unwrap();;
+        context.send_command(WebGLCommand::CreateXRWebGLLayer(resolution, sender));
+        let (framebuffer, texture) = receiver.recv().unwrap().map_err(|()| Error::Operation)?;
+        let framebuffer = WebGLFramebuffer::new_with_color_attachment(
+            context,
+            framebuffer,
+            texture,
+            Size2D::new(resolution.width as u32, resolution.height as u32)
+        );
 
         // Step 8.2. "Initialize layer’s framebuffer to a new opaque framebuffer created with context."
-        let framebuffer = context.CreateFramebuffer().ok_or(Error::Operation)?;
+        /*let framebuffer = context.CreateFramebuffer().ok_or(Error::Operation)?;
 
         // Step 8.3. "Allocate and initialize resources compatible with session’s XR device,
         // including GPU accessible memory buffers, as required to support the compositing of layer."
 
         // Create a new texture with size given by the session's recommended resolution
         let texture = context.CreateTexture().ok_or(Error::Operation)?;
-        let resolution = session.with_session(|s| s.recommended_framebuffer_resolution());
         let mut pixels = CustomAutoRooter::new(None);
         context.BindTexture(constants::TEXTURE_2D, Some(&texture));
         let sc = context.TexImage2D(
@@ -134,7 +147,7 @@ impl XRWebGLLayer {
 
         // Step 8.4: "If layer’s resources were unable to be created for any reason,
         // throw an OperationError and abort these steps."
-        sc.or(Err(Error::Operation))?;
+        sc.or(Err(Error::Operation))?;*/
 
         // Step 9. "Return layer."
         Ok(XRWebGLLayer::new(

[Manishearth]

The servo patches don't apply, there is no StartXRFrame in webgl_thread.rs

Is there a second commit missing here?

[jdm]

@Manishearth I attempted to remove any references to StartXRFrame hours ago. Are you applying the right patch? StartXRFrame and EndXRFrame did not actually do anything, so they can be ignored.

[Manishearth]

Ah, i was using the patches from the issue.

[Manishearth]

Applying the above patch gives me a malformed patch issue, on linux

patch: **** malformed patch at line 65: diff --git a/components/canvas_traits/webgl.rs b/components/canvas_traits/webgl.rs

The @@ offset numbers are all wrong, was this patch hand edited? I'll hand-edit it back and see what i get

[Manishearth]

Managed to apply it

[asajeffrey]

I'm still concerned about using this approach. There's a lot of state that's kept in sync between webgl and gl, so doing anything where we execute GL commands directly rather than going via WebGL seems like a recipe for getting out of sync.

[jdm]

My hypothesis about why the panic occurs was wrong - the bound texture remains bound when we get a GL error executing the glTexImage2D - I'm pretty sure it's because we're hardcoding gl::RGBA, whereas the code to decide what format to use for draw buffer in rust-offscreen-rendering-context is more complex and uses different values for GLES than GL.

[jdm]

I verified that immersive mode works fine with the existing code on HoloLens if the texture is initialized with constants::RGB and the framebuffer attachment complete checks are updated to accept that value. I'm writing a patch that should allow this to work on all platforms.