From bb8d9ba74c024a317fbbd49811541632404ef51c Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Fri, 24 Aug 2018 16:10:28 -0400 Subject: [PATCH] webgl: Ensure that depth and stencil attachments are rebound after messing with DEPTH_STENCIL attachments. --- components/script/dom/webglframebuffer.rs | 192 +++++++++++++----- components/script/dom/webglrenderbuffer.rs | 44 +++- .../script/dom/webglrenderingcontext.rs | 34 +--- components/script/dom/webgltexture.rs | 43 +++- .../framebuffer-object-attachment.html.ini | 3 - 5 files changed, 233 insertions(+), 83 deletions(-) delete mode 100644 tests/wpt/webgl/meta/conformance/renderbuffers/framebuffer-object-attachment.html.ini diff --git a/components/script/dom/webglframebuffer.rs b/components/script/dom/webglframebuffer.rs index 277b8b2fcba..76be81aee87 100644 --- a/components/script/dom/webglframebuffer.rs +++ b/components/script/dom/webglframebuffer.rs @@ -45,6 +45,15 @@ impl WebGLFramebufferAttachment { WebGLFramebufferAttachment::Texture { .. } => () } } + + fn root(&self) -> WebGLFramebufferAttachmentRoot { + match *self { + WebGLFramebufferAttachment::Renderbuffer(ref rb) => + WebGLFramebufferAttachmentRoot::Renderbuffer(DomRoot::from_ref(&rb)), + WebGLFramebufferAttachment::Texture { ref texture, .. } => + WebGLFramebufferAttachmentRoot::Texture(DomRoot::from_ref(&texture)), + } + } } #[derive(Clone, JSTraceable, MallocSizeOf)] @@ -261,24 +270,16 @@ impl WebGLFramebuffer { } pub fn renderbuffer(&self, attachment: u32, rb: Option<&WebGLRenderbuffer>) -> WebGLResult<()> { - let binding = match attachment { - constants::COLOR_ATTACHMENT0 => &self.color, - constants::DEPTH_ATTACHMENT => &self.depth, - constants::STENCIL_ATTACHMENT => &self.stencil, - constants::DEPTH_STENCIL_ATTACHMENT => &self.depthstencil, - _ => return Err(WebGLError::InvalidEnum), - }; + let binding = self.attachment_binding(attachment).ok_or(WebGLError::InvalidEnum)?; let rb_id = match rb { Some(rb) => { + rb.attach(self); *binding.borrow_mut() = Some(WebGLFramebufferAttachment::Renderbuffer(Dom::from_ref(rb))); Some(rb.id()) } - _ => { - *binding.borrow_mut() = None; - None - } + _ => None }; self.upcast::().context().send_command( @@ -290,39 +291,100 @@ impl WebGLFramebuffer { ), ); + if rb.is_none() { + self.detach_binding(binding, attachment); + } + self.update_status(); self.is_initialized.set(false); Ok(()) } - pub fn attachment(&self, attachment: u32) -> Option { - let binding = match attachment { - constants::COLOR_ATTACHMENT0 => &self.color, - constants::DEPTH_ATTACHMENT => &self.depth, - constants::STENCIL_ATTACHMENT => &self.stencil, - constants::DEPTH_STENCIL_ATTACHMENT => &self.depthstencil, - _ => return None, + fn detach_binding( + &self, + binding: &DomRefCell>, + attachment: u32, + ) { + let attachment_obj = binding.borrow().as_ref().map(WebGLFramebufferAttachment::root); + match attachment_obj { + Some(WebGLFramebufferAttachmentRoot::Renderbuffer(ref rb)) => + rb.unattach(self), + Some(WebGLFramebufferAttachmentRoot::Texture(ref texture)) => + texture.unattach(self), + None => (), + } + *binding.borrow_mut() = None; + + if INTERESTING_ATTACHMENT_POINTS.contains(&attachment) { + self.reattach_depth_stencil(); + } + } + + fn attachment_binding( + &self, + attachment: u32 + ) -> Option<&DomRefCell>> + { + match attachment { + constants::COLOR_ATTACHMENT0 => Some(&self.color), + constants::DEPTH_ATTACHMENT => Some(&self.depth), + constants::STENCIL_ATTACHMENT => Some(&self.stencil), + constants::DEPTH_STENCIL_ATTACHMENT => Some(&self.depthstencil), + _ => None + } + } + + fn reattach_depth_stencil(&self) { + let reattach = |attachment: &WebGLFramebufferAttachment, attachment_point| { + let context = self.upcast::().context(); + match *attachment { + WebGLFramebufferAttachment::Renderbuffer(ref rb) => { + context.send_command( + WebGLCommand::FramebufferRenderbuffer( + constants::FRAMEBUFFER, + attachment_point, + constants::RENDERBUFFER, + Some(rb.id()) + ) + ); + } + WebGLFramebufferAttachment::Texture { ref texture, level } => { + context.send_command( + WebGLCommand::FramebufferTexture2D( + constants::FRAMEBUFFER, + attachment_point, + texture.target().expect("missing texture target"), + Some(texture.id()), + level + ) + ); + } + } }; - binding.borrow().as_ref().map(|bin| { - match bin { - &WebGLFramebufferAttachment::Renderbuffer(ref rb) => - WebGLFramebufferAttachmentRoot::Renderbuffer(DomRoot::from_ref(&rb)), - &WebGLFramebufferAttachment::Texture { ref texture, .. } => - WebGLFramebufferAttachmentRoot::Texture(DomRoot::from_ref(&texture)), - } - }) + // Since the DEPTH_STENCIL attachment causes both the DEPTH and STENCIL + // attachments to be overwritten, we need to ensure that we reattach + // the DEPTH and STENCIL attachments when any of those attachments + // is cleared. + if let Some(ref depth) = *self.depth.borrow() { + reattach(depth, constants::DEPTH_ATTACHMENT); + } + if let Some(ref stencil) = *self.stencil.borrow() { + reattach(stencil, constants::STENCIL_ATTACHMENT); + } + if let Some(ref depth_stencil) = *self.depthstencil.borrow() { + reattach(depth_stencil, constants::DEPTH_STENCIL_ATTACHMENT); + } + } + + pub fn attachment(&self, attachment: u32) -> Option { + let binding = self.attachment_binding(attachment)?; + binding.borrow().as_ref().map(WebGLFramebufferAttachment::root) } pub fn texture2d(&self, attachment: u32, textarget: u32, texture: Option<&WebGLTexture>, level: i32) -> WebGLResult<()> { - let binding = match attachment { - constants::COLOR_ATTACHMENT0 => &self.color, - constants::DEPTH_ATTACHMENT => &self.depth, - constants::STENCIL_ATTACHMENT => &self.stencil, - constants::DEPTH_STENCIL_ATTACHMENT => &self.depthstencil, - _ => return Err(WebGLError::InvalidEnum), - }; + let binding = self.attachment_binding(attachment).ok_or(WebGLError::InvalidEnum)?; let tex_id = match texture { // Note, from the GLES 2.0.25 spec, page 113: @@ -367,6 +429,7 @@ impl WebGLFramebuffer { _ => return Err(WebGLError::InvalidOperation), } + texture.attach(self); *binding.borrow_mut() = Some(WebGLFramebufferAttachment::Texture { texture: Dom::from_ref(texture), level: level } @@ -376,7 +439,6 @@ impl WebGLFramebuffer { } _ => { - *binding.borrow_mut() = None; None } }; @@ -391,20 +453,26 @@ impl WebGLFramebuffer { ), ); + if texture.is_none() { + self.detach_binding(binding, attachment); + } + self.update_status(); self.is_initialized.set(false); Ok(()) } fn with_matching_renderbuffers(&self, rb: &WebGLRenderbuffer, mut closure: F) - where F: FnMut(&DomRefCell>) + where F: FnMut(&DomRefCell>, u32) { - let attachments = [&self.color, - &self.depth, - &self.stencil, - &self.depthstencil]; + let attachments = [ + (&self.color, constants::COLOR_ATTACHMENT0), + (&self.depth, constants::DEPTH_ATTACHMENT), + (&self.stencil, constants::STENCIL_ATTACHMENT), + (&self.depthstencil, constants::DEPTH_STENCIL_ATTACHMENT) + ]; - for attachment in &attachments { + for (attachment, name) in &attachments { let matched = { match *attachment.borrow() { Some(WebGLFramebufferAttachment::Renderbuffer(ref att_rb)) @@ -414,20 +482,22 @@ impl WebGLFramebuffer { }; if matched { - closure(attachment); + closure(attachment, *name); } } } fn with_matching_textures(&self, texture: &WebGLTexture, mut closure: F) - where F: FnMut(&DomRefCell>) + where F: FnMut(&DomRefCell>, u32) { - let attachments = [&self.color, - &self.depth, - &self.stencil, - &self.depthstencil]; + let attachments = [ + (&self.color, constants::COLOR_ATTACHMENT0), + (&self.depth, constants::DEPTH_ATTACHMENT), + (&self.stencil, constants::STENCIL_ATTACHMENT), + (&self.depthstencil, constants::DEPTH_STENCIL_ATTACHMENT) + ]; - for attachment in &attachments { + for (attachment, name) in &attachments { let matched = { match *attachment.borrow() { Some(WebGLFramebufferAttachment::Texture { texture: ref att_texture, .. }) @@ -437,33 +507,45 @@ impl WebGLFramebuffer { }; if matched { - closure(attachment); + closure(attachment, *name); } } } pub fn detach_renderbuffer(&self, rb: &WebGLRenderbuffer) { - self.with_matching_renderbuffers(rb, |att| { + let mut depth_or_stencil_updated = false; + self.with_matching_renderbuffers(rb, |att, name| { + depth_or_stencil_updated |= INTERESTING_ATTACHMENT_POINTS.contains(&name); *att.borrow_mut() = None; self.update_status(); }); + + if depth_or_stencil_updated { + self.reattach_depth_stencil(); + } } pub fn detach_texture(&self, texture: &WebGLTexture) { - self.with_matching_textures(texture, |att| { + let mut depth_or_stencil_updated = false; + self.with_matching_textures(texture, |att, name| { + depth_or_stencil_updated |= INTERESTING_ATTACHMENT_POINTS.contains(&name); *att.borrow_mut() = None; self.update_status(); }); + + if depth_or_stencil_updated { + self.reattach_depth_stencil(); + } } pub fn invalidate_renderbuffer(&self, rb: &WebGLRenderbuffer) { - self.with_matching_renderbuffers(rb, |_att| { + self.with_matching_renderbuffers(rb, |_att, _| { self.update_status(); }); } pub fn invalidate_texture(&self, texture: &WebGLTexture) { - self.with_matching_textures(texture, |_att| { + self.with_matching_textures(texture, |_att, _name| { self.update_status(); }); } @@ -478,3 +560,9 @@ impl Drop for WebGLFramebuffer { self.delete(); } } + +static INTERESTING_ATTACHMENT_POINTS: &[u32] = &[ + constants::DEPTH_ATTACHMENT, + constants::STENCIL_ATTACHMENT, + constants::DEPTH_STENCIL_ATTACHMENT, +]; diff --git a/components/script/dom/webglrenderbuffer.rs b/components/script/dom/webglrenderbuffer.rs index f346407581f..7ee108ebdc5 100644 --- a/components/script/dom/webglrenderbuffer.rs +++ b/components/script/dom/webglrenderbuffer.rs @@ -4,12 +4,14 @@ // https://www.khronos.org/registry/webgl/specs/latest/1.0/webgl.idl use canvas_traits::webgl::{webgl_channel, WebGLCommand, WebGLError, WebGLRenderbufferId, WebGLResult}; +use dom::bindings::cell::DomRefCell; use dom::bindings::codegen::Bindings::WebGL2RenderingContextBinding::WebGL2RenderingContextConstants as WebGl2Constants; use dom::bindings::codegen::Bindings::WebGLRenderbufferBinding; use dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLRenderingContextConstants as constants; use dom::bindings::inheritance::Castable; use dom::bindings::reflector::{DomObject, reflect_dom_object}; -use dom::bindings::root::DomRoot; +use dom::bindings::root::{DomRoot, Dom}; +use dom::webglframebuffer::WebGLFramebuffer; use dom::webglobject::WebGLObject; use dom::webglrenderingcontext::{WebGLRenderingContext, is_gles}; use dom_struct::dom_struct; @@ -24,6 +26,8 @@ pub struct WebGLRenderbuffer { size: Cell>, internal_format: Cell>, is_initialized: Cell, + // Framebuffer that this texture is attached to. + attached_framebuffers: DomRefCell>>, } impl WebGLRenderbuffer { @@ -36,6 +40,7 @@ impl WebGLRenderbuffer { internal_format: Cell::new(None), size: Cell::new(None), is_initialized: Cell::new(false), + attached_framebuffers: Default::default(), } } @@ -86,6 +91,29 @@ impl WebGLRenderbuffer { pub fn delete(&self) { if !self.is_deleted.get() { self.is_deleted.set(true); + + /* + If a renderbuffer object is deleted while its image is attached to the currently + bound framebuffer, then it is as if FramebufferRenderbuffer had been called, with + a renderbuffer of 0, for each attachment point to which this image was attached + in the currently bound framebuffer. + - GLES 2.0, 4.4.3, "Attaching Renderbuffer Images to a Framebuffer" + */ + let currently_bound_framebuffer = + self.upcast::() + .context() + .bound_framebuffer() + .map_or(0, |fb| fb.id().get()); + let current_framebuffer = + self.attached_framebuffers + .borrow() + .iter() + .position(|fb| fb.id().get() == currently_bound_framebuffer); + if let Some(fb_index) = current_framebuffer { + self.attached_framebuffers.borrow()[fb_index].detach_renderbuffer(self); + self.attached_framebuffers.borrow_mut().remove(fb_index); + } + self.upcast::() .context() .send_command(WebGLCommand::DeleteRenderbuffer(self.id)); @@ -145,4 +173,18 @@ impl WebGLRenderbuffer { Ok(()) } + + pub fn attach(&self, framebuffer: &WebGLFramebuffer) { + self.attached_framebuffers.borrow_mut().push(Dom::from_ref(framebuffer)); + } + + pub fn unattach(&self, fb: &WebGLFramebuffer) { + let mut attached_framebuffers = self.attached_framebuffers.borrow_mut(); + let idx = attached_framebuffers.iter().position(|attached| { + attached.id() == fb.id() + }); + if let Some(idx) = idx { + attached_framebuffers.remove(idx); + } + } } diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index 6ca010a0b6e..19b0c20f260 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -1072,6 +1072,10 @@ impl WebGLRenderingContext { stencil: clear_bits & constants::STENCIL_BUFFER_BIT != 0, }); } + + pub fn bound_framebuffer(&self) -> Option> { + self.bound_framebuffer.get() + } } impl Drop for WebGLRenderingContext { @@ -1587,6 +1591,10 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { renderbuffer.bind(target); } _ => { + if renderbuffer.is_some() { + self.webgl_error(InvalidOperation); + } + self.bound_renderbuffer.set(None); // Unbind the currently bound renderbuffer self.send_command(WebGLCommand::BindRenderbuffer(target, None)); @@ -1986,20 +1994,6 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { handle_potential_webgl_error!(self, self.validate_ownership(renderbuffer), return); handle_object_deletion!(self, self.bound_renderbuffer, renderbuffer, Some(WebGLCommand::BindRenderbuffer(constants::RENDERBUFFER, None))); - // From the GLES 2.0.25 spec, page 113: - // - // "If a renderbuffer object is deleted while its - // image is attached to the currently bound - // framebuffer, then it is as if - // FramebufferRenderbuffer had been called, with a - // renderbuffer of 0, for each attachment point to - // which this image was attached in the currently - // bound framebuffer." - // - if let Some(fb) = self.bound_framebuffer.get() { - fb.detach_renderbuffer(renderbuffer); - } - renderbuffer.delete() } } @@ -2035,18 +2029,6 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { )); } - // From the GLES 2.0.25 spec, page 113: - // - // "If a texture object is deleted while its image is - // attached to the currently bound framebuffer, then - // it is as if FramebufferTexture2D had been called, - // with a texture of 0, for each attachment point to - // which this image was attached in the currently - // bound framebuffer." - if let Some(fb) = self.bound_framebuffer.get() { - fb.detach_texture(texture); - } - texture.delete() } } diff --git a/components/script/dom/webgltexture.rs b/components/script/dom/webgltexture.rs index 83378519848..4d085d51442 100644 --- a/components/script/dom/webgltexture.rs +++ b/components/script/dom/webgltexture.rs @@ -12,8 +12,9 @@ use dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLRenderi use dom::bindings::codegen::Bindings::WebGLTextureBinding; use dom::bindings::inheritance::Castable; use dom::bindings::reflector::{DomObject, reflect_dom_object}; -use dom::bindings::root::DomRoot; +use dom::bindings::root::{Dom, DomRoot}; use dom::webgl_validations::types::{TexImageTarget, TexFormat, TexDataType}; +use dom::webglframebuffer::WebGLFramebuffer; use dom::webglobject::WebGLObject; use dom::webglrenderingcontext::WebGLRenderingContext; use dom_struct::dom_struct; @@ -48,6 +49,8 @@ pub struct WebGLTexture { mag_filter: Cell, /// True if this texture is used for the DOMToTexture feature. attached_to_dom: Cell, + // Framebuffer that this texture is attached to. + attached_framebuffers: DomRefCell>>, } impl WebGLTexture { @@ -63,6 +66,7 @@ impl WebGLTexture { mag_filter: Cell::new(constants::LINEAR), image_info_array: DomRefCell::new([ImageInfo::new(); MAX_LEVEL_COUNT * MAX_FACE_COUNT]), attached_to_dom: Cell::new(false), + attached_framebuffers: Default::default(), } } @@ -186,6 +190,29 @@ impl WebGLTexture { DOMToTextureCommand::Detach(self.id), ); } + + /* + If a texture object is deleted while its image is attached to the currently + bound framebuffer, then it is as if FramebufferTexture2D had been called, with + a texture of 0, for each attachment point to which this image was attached + in the currently bound framebuffer. + - GLES 2.0, 4.4.3, "Attaching Texture Images to a Framebuffer" + */ + let currently_bound_framebuffer = + self.upcast::() + .context() + .bound_framebuffer() + .map_or(0, |fb| fb.id().get()); + let current_framebuffer = + self.attached_framebuffers + .borrow() + .iter() + .position(|fb| fb.id().get() == currently_bound_framebuffer); + if let Some(fb_index) = current_framebuffer { + self.attached_framebuffers.borrow()[fb_index].detach_texture(self); + self.attached_framebuffers.borrow_mut().remove(fb_index); + } + context.send_command(WebGLCommand::DeleteTexture(self.id)); } } @@ -398,6 +425,20 @@ impl WebGLTexture { pub fn set_attached_to_dom(&self) { self.attached_to_dom.set(true); } + + pub fn attach(&self, framebuffer: &WebGLFramebuffer) { + self.attached_framebuffers.borrow_mut().push(Dom::from_ref(framebuffer)); + } + + pub fn unattach(&self, fb: &WebGLFramebuffer) { + let mut attached_framebuffers = self.attached_framebuffers.borrow_mut(); + let idx = attached_framebuffers.iter().position(|attached| { + attached.id() == fb.id() + }); + if let Some(idx) = idx { + attached_framebuffers.remove(idx); + } + } } impl Drop for WebGLTexture { diff --git a/tests/wpt/webgl/meta/conformance/renderbuffers/framebuffer-object-attachment.html.ini b/tests/wpt/webgl/meta/conformance/renderbuffers/framebuffer-object-attachment.html.ini deleted file mode 100644 index b0777f21743..00000000000 --- a/tests/wpt/webgl/meta/conformance/renderbuffers/framebuffer-object-attachment.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[framebuffer-object-attachment.html] - [WebGL test #139: at (0, 0) expected: 0,255,0,255 was 255,0,0,255] - expected: fail