webgl: Ensure that depth and stencil attachments are rebound after messing with DEPTH_STENCIL attachments.

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 {
+        let binding = self.attachment_binding(attachment).ok_or(WebGLError::InvalidEnum)?;
-            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 rb_id = match rb {
             Some(rb) => {
+                rb.attach(self);
                 *binding.borrow_mut() = Some(WebGLFramebufferAttachment::Renderbuffer(Dom::from_ref(rb)));
                 Some(rb.id())
             }
 
-            _ => {
+            _ => None
-                *binding.borrow_mut() = None;
-                None
-            }
         };
 
         self.upcast::<WebGLObject>().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<WebGLFramebufferAttachmentRoot> {
+    fn detach_binding(
-        let binding = match attachment {
+        &self,
-            constants::COLOR_ATTACHMENT0 => &self.color,
+        binding: &DomRefCell<Option<WebGLFramebufferAttachment>>,
-            constants::DEPTH_ATTACHMENT => &self.depth,
+        attachment: u32,
-            constants::STENCIL_ATTACHMENT => &self.stencil,
+    ) {
-            constants::DEPTH_STENCIL_ATTACHMENT => &self.depthstencil,
+        let attachment_obj = binding.borrow().as_ref().map(WebGLFramebufferAttachment::root);
-            _ => return None,
+        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<Option<WebGLFramebufferAttachment>>>
+    {
+        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::<WebGLObject>().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| {
+        // Since the DEPTH_STENCIL attachment causes both the DEPTH and STENCIL
-            match bin {
+        // attachments to be overwritten, we need to ensure that we reattach
-                &WebGLFramebufferAttachment::Renderbuffer(ref rb) =>
+        // the DEPTH and STENCIL attachments when any of those attachments
-                    WebGLFramebufferAttachmentRoot::Renderbuffer(DomRoot::from_ref(&rb)),
+        // is cleared.
-                &WebGLFramebufferAttachment::Texture { ref texture, .. } =>
+        if let Some(ref depth) = *self.depth.borrow() {
-                    WebGLFramebufferAttachmentRoot::Texture(DomRoot::from_ref(&texture)),
+            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<WebGLFramebufferAttachmentRoot> {
+        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 {
+        let binding = self.attachment_binding(attachment).ok_or(WebGLError::InvalidEnum)?;
-            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 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<F>(&self, rb: &WebGLRenderbuffer, mut closure: F)
-        where F: FnMut(&DomRefCell<Option<WebGLFramebufferAttachment>>)
+        where F: FnMut(&DomRefCell<Option<WebGLFramebufferAttachment>>, u32)
     {
-        let attachments = [&self.color,
+        let attachments = [
-                           &self.depth,
+            (&self.color, constants::COLOR_ATTACHMENT0),
-                           &self.stencil,
+            (&self.depth, constants::DEPTH_ATTACHMENT),
-                           &self.depthstencil];
+            (&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<F>(&self, texture: &WebGLTexture, mut closure: F)
-        where F: FnMut(&DomRefCell<Option<WebGLFramebufferAttachment>>)
+        where F: FnMut(&DomRefCell<Option<WebGLFramebufferAttachment>>, u32)
     {
-        let attachments = [&self.color,
+        let attachments = [
-                           &self.depth,
+            (&self.color, constants::COLOR_ATTACHMENT0),
-                           &self.stencil,
+            (&self.depth, constants::DEPTH_ATTACHMENT),
-                           &self.depthstencil];
+            (&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,
+];

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<Option<(i32, i32)>>,
     internal_format: Cell<Option<u32>>,
     is_initialized: Cell<bool>,
+    // Framebuffer that this texture is attached to.
+    attached_framebuffers: DomRefCell<Vec<Dom<WebGLFramebuffer>>>,
 }
 
 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::<WebGLObject>()
+                    .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::<WebGLObject>()
                 .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);
+        }
+    }
 }

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<DomRoot<WebGLFramebuffer>> {
+        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()
         }
     }

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<u32>,
     /// True if this texture is used for the DOMToTexture feature.
     attached_to_dom: Cell<bool>,
+    // Framebuffer that this texture is attached to.
+    attached_framebuffers: DomRefCell<Vec<Dom<WebGLFramebuffer>>>,
 }
 
 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::<WebGLObject>()
+                    .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 {

tests/wpt/webgl/meta/conformance/renderbuffers/framebuffer-object-attachment.html.ini

@@ -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