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

Depth issues fixes for post-atomic XR layer creation #24141

Closed
jdm opened this issue Sep 4, 2019 · 3 comments
Closed

Depth issues fixes for post-atomic XR layer creation #24141

jdm opened this issue Sep 4, 2019 · 3 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Sep 4, 2019

Patch for Servo based on #24083 which needs cleaning up:

diff --git a/components/canvas/gl_context.rs b/components/canvas/gl_context.rs
index e1e45f40a60d..4c3849409e44 100644
--- a/components/canvas/gl_context.rs
+++ b/components/canvas/gl_context.rs
@@ -4,7 +4,7 @@
 
 use super::webgl_thread::{GLState, WebGLImpl};
 use canvas_traits::webgl::{
-    GLContextAttributes, GLLimits, WebGLCommand, WebGLCommandBacktrace, WebGLVersion,
+    GLContextAttributes, GLFormats, GLLimits, TexFormat, TexDataType, WebGLCommand, WebGLCommandBacktrace, WebGLVersion,
 };
 use euclid::default::Size2D;
 use gleam::gl;
diff --git a/components/canvas/webgl_thread.rs b/components/canvas/webgl_thread.rs
index 9fe8e57d680d..e531f716f7a8 100644
--- a/components/canvas/webgl_thread.rs
+++ b/components/canvas/webgl_thread.rs
@@ -12,7 +12,7 @@ use gleam::gl;
 use half::f16;
 use ipc_channel::ipc::{self, OpaqueIpcMessage};
 use ipc_channel::router::ROUTER;
-use offscreen_gl_context::{DrawBuffer, GLContext, NativeGLContextMethods};
+use offscreen_gl_context::{ColorAttachmentType, DrawBuffer, GLContext, NativeGLContextMethods, RenderbufferAttachments};
 use pixels::{self, PixelFormat};
 use std::borrow::Cow;
 use std::cell::Cell;
@@ -39,6 +39,7 @@ pub struct GLState {
     stencil_clear_value: i32,
     depth_write_mask: bool,
     depth_clear_value: f64,
+    draw_buffer: Option<DrawBuffer>,
 }
 
 impl Default for GLState {
@@ -50,6 +51,7 @@ impl Default for GLState {
             stencil_clear_value: 0,
             depth_write_mask: true,
             depth_clear_value: 1.,
+            draw_buffer: None,
         }
     }
 }
@@ -865,6 +867,7 @@ struct WebGLContextInfo {
     gl_sync: Option<gl::GLsync>,
     /// The status of this context with respect to external consumers.
     render_state: ContextRenderState,
+    
 }
 
 /// Data about the linked DOM<->WebGLTexture elements.
@@ -888,10 +891,44 @@ impl WebGLImpl {
     ) {
         match command {
-            WebGLCommand::CreateXRWebGLLayer(ref resolution, ref sender) => {
-                let gl = ctx.gl();
+            WebGLCommand::CreateXRWebGLLayer(ref resolution, ref sender, ref alpha, ref depth, ref stencil, ref antialias) => {
+                //let gl = ctx.gl();
+                let raw_formats = ctx.borrow_formats();
+                let formats = GLFormats {
+                    texture_internal: TexFormat::from_gl_constant(raw_formats.texture_internal).expect("unknown format"),
+                    texture_type: TexDataType::from_gl_constant(raw_formats.texture_type).expect("unknown type"),
+                    depth: raw_formats.depth,
+                    stencil: raw_formats.stencil,
+                };
 
-                let mut value = [0];
+                let attrs = offscreen_gl_context::GLContextAttributes {
+                    alpha: *alpha,
+                    depth: *depth,
+                    stencil: *stencil,
+                    antialias: *antialias,
+                    premultiplied_alpha: false,
+                    preserve_drawing_buffer: false,
+                };
+                
+                let draw_buffer = match DrawBuffer::new_with_attributes(ctx, Size2D::new(resolution.width, resolution.height),
+                                                                        ColorAttachmentType::Texture, &attrs) {
+                    Ok(d) => d,
+                    Err(s) => {
+                        error!("couldn't create XR draw buffer: {}", s);
+                        let _ = sender.send(Err(()));
+                        return;
+                    }
+                };
+                let framebuffer = draw_buffer.get_framebuffer();
+                let attachments = draw_buffer.get_renderbuffer_attachments();
+                let (depth, stencil, packed) = match attachments {
+                    RenderbufferAttachments::DepthAndStencil(depth, stencil) => (depth, stencil, None),
+                    RenderbufferAttachments::PackedDepthAndStencil(packed) => (None, None, Some(packed)),
+                };
+                let texture = draw_buffer.get_bound_texture_id().expect("should have a texture");
+                state.draw_buffer = Some(draw_buffer);
+
+                /*let mut value = [0];
                 unsafe {
                     gl.get_integer_v(gl::TEXTURE_BINDING_2D, &mut value);
                 }
@@ -901,9 +938,9 @@ impl WebGLImpl {
                     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 old_framebuffer = value[0] as gl::GLuint;*/
                 
-                let texture = gl.gen_textures(1)[0];
+                /*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);
@@ -931,14 +968,21 @@ impl WebGLImpl {
                     texture,
                     0,
                 );
-				assert_eq!(gl.get_error(), gl::NO_ERROR);
+				assert_eq!(gl.get_error(), gl::NO_ERROR);*/
                 
-                gl.bind_texture(gl::TEXTURE_2D, old_texture);
+                /*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);
+				assert_eq!(gl.get_error(), gl::NO_ERROR);*/
                 unsafe {
-                    let _ = sender.send(Ok((WebGLFramebufferId::new(framebuffer), WebGLTextureId::new(texture))));
+                    let _ = sender.send(Ok((
+                        formats,
+                        WebGLFramebufferId::new(framebuffer),
+                        WebGLTextureId::new(texture),
+                        depth.map(|d| WebGLRenderbufferId::new(d)),
+                        stencil.map(|s| WebGLRenderbufferId::new(s)),
+                        packed.map(|p| WebGLRenderbufferId::new(p)),
+                    )));
                 }
             }
             WebGLCommand::GetContextAttributes(ref sender) => sender
diff --git a/components/canvas_traits/webgl.rs b/components/canvas_traits/webgl.rs
index e725b6d6703e..631de2c7203e 100644
--- a/components/canvas_traits/webgl.rs
+++ b/components/canvas_traits/webgl.rs
@@ -211,12 +211,34 @@ impl<T> Deref for TruncatedDebug<T> {
     }
 }
 
+#[derive(Debug, Deserialize, Serialize)]
+pub struct GLFormats {
+    pub texture_internal: TexFormat,
+    pub texture_type: TexDataType,
+    pub depth: u32,
+    pub stencil: u32,
+}
+
 /// WebGL Commands for a specific WebGLContext
 #[derive(Debug, Deserialize, Serialize)]
 pub enum WebGLCommand {
-    CreateXRWebGLLayer(TypedSize2D<i32, webxr_api::Viewport>, WebGLSender<Result<(WebGLFramebufferId, WebGLTextureId), ()>>),
+    CreateXRWebGLLayer(
+        TypedSize2D<i32, webxr_api::Viewport>,
+        WebGLSender<Result<(
+            GLFormats,
+            WebGLFramebufferId,
+            WebGLTextureId,
+            Option<WebGLRenderbufferId>, // depth
+            Option<WebGLRenderbufferId>, // stencil
+            Option<WebGLRenderbufferId>, // packed
+        ), ()>>,
+        bool, // alpha
+        bool, // depth
+        bool, // stencil
+        bool, // antialias
+    ),
     GetContextAttributes(WebGLSender<GLContextAttributes>),
     ActiveTexture(u32),
     BlendColor(f32, f32, f32, f32),
@@ -786,6 +808,7 @@ gl_enums! {
         DepthComponent = gl::DEPTH_COMPONENT,
         Alpha = gl::ALPHA,
         RGB = gl::RGB,
+        RGB8 = gl::RGB8,
         RGBA = gl::RGBA,
         Luminance = gl::LUMINANCE,
         LuminanceAlpha = gl::LUMINANCE_ALPHA,
diff --git a/components/script/dom/webglframebuffer.rs b/components/script/dom/webglframebuffer.rs
index faf9c720c856..cd503c926a79 100644
--- a/components/script/dom/webglframebuffer.rs
+++ b/components/script/dom/webglframebuffer.rs
@@ -6,6 +6,7 @@
 use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::codegen::Bindings::WebGLFramebufferBinding;
 use crate::dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLRenderingContextConstants as constants;
+use crate::dom::bindings::codegen::Bindings::WebGL2RenderingContextBinding::WebGL2RenderingContextBinding::WebGL2RenderingContextConstants as constants2;
 use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::reflector::{reflect_dom_object, DomObject};
 use crate::dom::bindings::root::{Dom, DomRoot};
@@ -15,7 +16,10 @@ 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::{TexFormat, TexDataType, WebGLCommand, WebGLFramebufferBindingRequest, WebGLFramebufferId, WebGLTextureId};
+use canvas_traits::webgl::{
+    GLFormats, TexFormat, TexDataType, WebGLCommand, WebGLFramebufferBindingRequest, WebGLFramebufferId, WebGLRenderbufferId,
+    WebGLTextureId
+};
 use dom_struct::dom_struct;
 use euclid::default::Size2D;
 use std::cell::Cell;
@@ -121,10 +125,14 @@ impl WebGLFramebuffer {
         )
     }
     
-    pub(crate) fn new_with_color_attachment(
+    pub(crate) fn new_with_attachments(
         context: &WebGLRenderingContext,
         id: WebGLFramebufferId,
         texture: WebGLTextureId,
+        depth: Option<WebGLRenderbufferId>,
+        stencil: Option<WebGLRenderbufferId>,
+        packed: Option<WebGLRenderbufferId>,
+        formats: GLFormats,
         size: Size2D<u32>,
     ) -> DomRoot<Self> {
         let fb = reflect_dom_object(
@@ -134,12 +142,26 @@ impl WebGLFramebuffer {
         );
         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();
+        texture.initialize(TexImageTarget::Texture2D, size.width, size.height, 1, formats.texture_internal, 0, Some(formats.texture_type)).unwrap();
         *fb.color.borrow_mut() = Some(WebGLFramebufferAttachment::Texture {
             texture: Dom::from_ref(&*texture),
             level: 0,
         });
-        //fb.status.set(constants::FRAMEBUFFER_COMPLETE);
+        if let Some(depth) = depth {
+            let depth = WebGLRenderbuffer::new(context, depth);
+            depth.init_with_existing_storage(formats.depth, size.width as i32, size.height as i32);
+            *fb.depth.borrow_mut() = Some(WebGLFramebufferAttachment::Renderbuffer(Dom::from_ref(&*depth)));
+        }
+        if let Some(stencil) = stencil {
+            let stencil = WebGLRenderbuffer::new(context, stencil);
+            stencil.init_with_existing_storage(formats.stencil, size.width as i32, size.height as i32);
+            *fb.stencil.borrow_mut() = Some(WebGLFramebufferAttachment::Renderbuffer(Dom::from_ref(&*stencil)));
+        }
+        if let Some(packed) = packed {
+            let packed = WebGLRenderbuffer::new(context, packed);
+            packed.init_with_existing_storage(constants2::DEPTH24_STENCIL8, size.width as i32, size.height as i32);
+            *fb.depthstencil.borrow_mut() = Some(WebGLFramebufferAttachment::Renderbuffer(Dom::from_ref(&*packed)));
+        }
         fb.update_status();
         fb
     }
@@ -202,8 +224,11 @@ impl WebGLFramebuffer {
                 constants::RGB5_A1,
                 constants::RGB565,
                 constants::RGBA,
+				constants::RGB,
+				constants2::RGB8,
+				constants2::RGBA8,
             ][..],
-            &[constants::DEPTH_COMPONENT16][..],
+            &[constants::DEPTH_COMPONENT16, constants2::DEPTH_COMPONENT24][..],
             &[constants::STENCIL_INDEX8][..],
             &[constants::DEPTH_STENCIL][..],
         ];
diff --git a/components/script/dom/webglrenderbuffer.rs b/components/script/dom/webglrenderbuffer.rs
index f210b579e88b..8ee6a9fdbf3e 100644
--- a/components/script/dom/webglrenderbuffer.rs
+++ b/components/script/dom/webglrenderbuffer.rs
@@ -183,9 +183,6 @@ impl WebGLRenderbuffer {
             _ => return Err(WebGLError::InvalidEnum),
         };
 
-        self.internal_format.set(Some(internal_format));
-        self.is_initialized.set(false);
-
         self.upcast::<WebGLObject>()
             .context()
             .send_command(WebGLCommand::RenderbufferStorage(
@@ -195,10 +192,17 @@ impl WebGLRenderbuffer {
                 height,
             ));
 
-        self.size.set(Some((width, height)));
+        self.init_with_existing_storage(internal_format, width, height);
 
         Ok(())
     }
+    
+    pub fn init_with_existing_storage(&self, internal_format: u32, width: i32, height: i32) {
+        self.internal_format.set(Some(internal_format));
+        self.is_initialized.set(false);
+        self.size.set(Some((width, height)));
+        self.ever_bound.set(true);
+    }
 }
 
 impl Drop for WebGLRenderbuffer {
diff --git a/components/script/dom/xrwebgllayer.rs b/components/script/dom/xrwebgllayer.rs
index 84a37e03a854..6f993d5251aa 100644
--- a/components/script/dom/xrwebgllayer.rs
+++ b/components/script/dom/xrwebgllayer.rs
@@ -100,12 +100,16 @@ impl XRWebGLLayer {
             
         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.send_command(WebGLCommand::CreateXRWebGLLayer(resolution, sender, init.alpha, init.depth, init.stencil, init.antialias));
+        let (formats, framebuffer, texture, depth, stencil, packed) = receiver.recv().unwrap().map_err(|()| Error::Operation)?;
+        let framebuffer = WebGLFramebuffer::new_with_attachments(
             context,
             framebuffer,
             texture,
+            depth,
+            stencil,
+            packed,
+            formats,
             Size2D::new(resolution.width as u32, resolution.height as u32)
         );

Originally posted by @jdm in #24082 (comment)

@jdm
Copy link
Member Author

@jdm jdm commented Sep 4, 2019

And a patch for rust-offscreen-gl-context which is required:

diff --git a/src/draw_buffer.rs b/src/draw_buffer.rs
index 6a72f45..d11f1e1 100644
--- a/src/draw_buffer.rs
+++ b/src/draw_buffer.rs
@@ -4,6 +4,7 @@ use gleam::gl::types::{GLuint, GLenum, GLint};
 use std::rc::Rc;
 
 use crate::GLContext;
+use crate::gl_context_attributes::GLContextAttributes;
 use crate::NativeGLContextMethods;
 
 #[derive(Debug)]
@@ -62,6 +63,11 @@ pub struct DrawBuffer {
     // samples: GLsizei,
 }
 
+pub enum RenderbufferAttachments {
+    DepthAndStencil(Option<GLuint>, Option<GLuint>),
+    PackedDepthAndStencil(GLuint),
+}
+
 /// Helper function to create a render buffer
 /// TODO(emilio): We'll need to switch between `glRenderbufferStorage` and
 /// `glRenderbufferStorageMultisample` when we support antialising
@@ -77,15 +83,26 @@ fn create_renderbuffer(gl_: &dyn gl::Gl,
 }
 
 impl DrawBuffer {
-    pub fn new<T: NativeGLContextMethods>(context: &GLContext<T>,
-                                          mut size: Size2D<i32>,
-                                          color_attachment_type: ColorAttachmentType)
-                                          -> Result<Self, &'static str>
+    pub fn new<T: NativeGLContextMethods>(
+        context: &GLContext<T>,
+        size: Size2D<i32>,
+        color_attachment_type: ColorAttachmentType
+    ) -> Result<Self, &'static str>
+    {
+        Self::new_with_attributes(context, size, color_attachment_type, context.borrow_attributes())
+    }
+
+    pub fn new_with_attributes<T>(
+        context: &GLContext<T>,
+        mut size: Size2D<i32>,
+        color_attachment_type: ColorAttachmentType,
+        attrs: &GLContextAttributes,
+    ) -> Result<Self, &'static str>
+        where T: NativeGLContextMethods
     {
         const MIN_DRAWING_BUFFER_SIZE: i32 = 16;
         use std::cmp;
 
-        let attrs = context.borrow_attributes();
         let capabilities = context.borrow_capabilities();
 
         debug!("Creating draw buffer {:?}, {:?}, attrs: {:?}, caps: {:?}",
@@ -118,7 +135,7 @@ impl DrawBuffer {
 
         context.make_current()?;
 
-        draw_buffer.init(context, color_attachment_type)?;
+        draw_buffer.init(context, color_attachment_type, attrs)?;
 
         debug_assert_eq!(draw_buffer.gl().check_frame_buffer_status(gl::FRAMEBUFFER),
                          gl::FRAMEBUFFER_COMPLETE);
@@ -158,6 +175,24 @@ impl DrawBuffer {
             &ColorAttachment::Texture(id) => Some(id),
         }
     }
+    
+    pub fn get_renderbuffer_attachments(&self) -> RenderbufferAttachments {
+        if self.packed_depth_stencil_renderbuffer != 0 {
+            RenderbufferAttachments::PackedDepthAndStencil(self.packed_depth_stencil_renderbuffer)
+        } else {
+            let depth = if self.depth_renderbuffer != 0 {
+                Some(self.depth_renderbuffer)
+            } else {
+                None
+            };
+            let stencil = if self.stencil_renderbuffer != 0 {
+                Some(self.stencil_renderbuffer)
+            } else {
+                None
+            };
+            RenderbufferAttachments::DepthAndStencil(depth, stencil)
+        }
+    }
 
     fn gl(&self) -> &dyn gl::Gl {
         &*self.gl_
@@ -166,9 +201,9 @@ impl DrawBuffer {
 
     fn init<T: NativeGLContextMethods>(&mut self,
                                        context: &GLContext<T>,
-                                       color_attachment_type: ColorAttachmentType)
+                                       color_attachment_type: ColorAttachmentType,
+                                       attrs: &GLContextAttributes)
         -> Result<(), &'static str> {
-        let attrs = context.borrow_attributes();
         let formats = context.borrow_formats();
 
         assert!(self.color_attachment.is_none(),
diff --git a/src/lib.rs b/src/lib.rs
index 45e1fd8..d9e6d1d 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -19,7 +19,7 @@ mod gl_context;
 pub use gl_context::{GLContext, GLContextDispatcher, GLVersion};
 
 mod draw_buffer;
-pub use draw_buffer::{DrawBuffer, ColorAttachmentType};
+pub use draw_buffer::{DrawBuffer, ColorAttachmentType, RenderbufferAttachments};
 
 mod gl_context_attributes;
 pub use gl_context_attributes::GLContextAttributes;
@jdm jdm changed the title GL error entering immersive mode in webxr content Depth issues fixes for post-atomic XR layer creation Sep 4, 2019
@jdm
Copy link
Member Author

@jdm jdm commented Sep 4, 2019

I removed the reference to StartXRFrame and EndXRFrame from this patch.

@atouchet atouchet added this to In progress in HoloLens Sep 5, 2019
@jdm
Copy link
Member Author

@jdm jdm commented Sep 30, 2019

This should no longer be necessary.

@jdm jdm closed this Sep 30, 2019
@atouchet atouchet moved this from In progress to Done in HoloLens Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
HoloLens
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.