From ea919cc95fd2df32ca4d9a5118fe0500cc9b8dbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 27 Aug 2016 01:35:29 -0700 Subject: [PATCH 1/2] Add tests that unsuccessfully try to reproduce the shadertoy issue. Figured I'd rather have them. --- src/draw_buffer.rs | 16 ++++--- src/gl_context_attributes.rs | 6 +-- src/tests.rs | 86 ++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 9 deletions(-) diff --git a/src/draw_buffer.rs b/src/draw_buffer.rs index 8d21e24..e9233a7 100644 --- a/src/draw_buffer.rs +++ b/src/draw_buffer.rs @@ -7,13 +7,14 @@ use NativeGLContextMethods; use std::ptr; +#[derive(Debug)] pub enum ColorAttachmentType { Texture, Renderbuffer, } -impl ColorAttachmentType { - pub fn default() -> ColorAttachmentType { +impl Default for ColorAttachmentType { + fn default() -> ColorAttachmentType { ColorAttachmentType::Renderbuffer } } @@ -23,6 +24,7 @@ impl ColorAttachmentType { /// Or a surface bound to a texture /// bound to a framebuffer as a color /// attachment +#[derive(Debug)] pub enum ColorAttachment { Renderbuffer(GLuint), Texture(GLuint), @@ -83,7 +85,7 @@ impl DrawBuffer { size: Size2D, color_attachment_type: ColorAttachmentType) -> Result { - + debug!("Creating draw buffer {:?}, {:?}", size, color_attachment_type); let attrs = context.borrow_attributes(); let capabilities = context.borrow_capabilities(); @@ -215,6 +217,8 @@ impl DrawBufferHelpers for DrawBuffer { gl::TexParameteri(gl::TEXTURE_2D, gl::TEXTURE_WRAP_S, gl::CLAMP_TO_EDGE as GLint); gl::TexParameteri(gl::TEXTURE_2D, gl::TEXTURE_WRAP_T, gl::CLAMP_TO_EDGE as GLint); + debug_assert!(gl::get_error() == gl::NO_ERROR); + Some(ColorAttachment::Texture(texture)) } }, @@ -246,15 +250,15 @@ impl DrawBufferHelpers for DrawBuffer { // NOTE: The assertion fails if the framebuffer is not bound debug_assert!(gl::IsFramebuffer(self.framebuffer) == gl::TRUE); - match self.color_attachment.as_ref().unwrap() { - &ColorAttachment::Renderbuffer(color_renderbuffer) => { + match *self.color_attachment.as_ref().unwrap() { + ColorAttachment::Renderbuffer(color_renderbuffer) => { gl::FramebufferRenderbuffer(gl::FRAMEBUFFER, gl::COLOR_ATTACHMENT0, gl::RENDERBUFFER, color_renderbuffer); debug_assert!(gl::IsRenderbuffer(color_renderbuffer) == gl::TRUE); }, - &ColorAttachment::Texture(texture_id) => { + ColorAttachment::Texture(texture_id) => { gl::FramebufferTexture2D(gl::FRAMEBUFFER, gl::COLOR_ATTACHMENT0, gl::TEXTURE_2D, diff --git a/src/gl_context_attributes.rs b/src/gl_context_attributes.rs index 456baa7..4b351bc 100644 --- a/src/gl_context_attributes.rs +++ b/src/gl_context_attributes.rs @@ -54,11 +54,13 @@ impl GLContextAttributes { preserve_drawing_buffer: false, } } +} +impl Default for GLContextAttributes { // FIXME(ecoal95): `antialias` should be true by default // but we do not support antialising so... We must change it // when we do. See GLFeature. - pub fn default() -> GLContextAttributes { + fn default() -> GLContextAttributes { GLContextAttributes { alpha: true, depth: true, @@ -69,5 +71,3 @@ impl GLContextAttributes { } } } - - diff --git a/src/tests.rs b/src/tests.rs index d46e3e1..a78520b 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -171,3 +171,89 @@ fn test_limits() { None).unwrap(); assert!(context.borrow_limits().max_vertex_attribs != 0); } + +#[test] +fn test_no_alpha() { + load_gl(); + let mut attributes = GLContextAttributes::default(); + attributes.alpha = false; + + let size = Size2D::new(256, 256); + let context = GLContext::::new(size, + attributes, + ColorAttachmentType::Texture, + None).unwrap(); + assert!(context.borrow_limits().max_vertex_attribs != 0); +} + +#[test] +fn test_no_depth() { + load_gl(); + let mut attributes = GLContextAttributes::default(); + attributes.depth = false; + + let size = Size2D::new(256, 256); + let context = GLContext::::new(size, + attributes, + ColorAttachmentType::Texture, + None).unwrap(); + assert!(context.borrow_limits().max_vertex_attribs != 0); +} + +#[test] +fn test_no_depth_no_alpha() { + load_gl(); + let mut attributes = GLContextAttributes::default(); + attributes.depth = false; + attributes.alpha = false; + + let size = Size2D::new(256, 256); + let context = GLContext::::new(size, + attributes, + ColorAttachmentType::Texture, + None).unwrap(); + assert!(context.borrow_limits().max_vertex_attribs != 0); +} + +#[test] +fn test_no_premul_alpha() { + load_gl(); + let mut attributes = GLContextAttributes::default(); + attributes.depth = false; + attributes.alpha = false; + attributes.premultiplied_alpha = false; + + let size = Size2D::new(256, 256); + let context = GLContext::::new(size, + attributes, + ColorAttachmentType::Texture, + None).unwrap(); + assert!(context.borrow_limits().max_vertex_attribs != 0); +} + +#[test] +fn test_in_a_row() { + load_gl(); + let mut attributes = GLContextAttributes::default(); + attributes.depth = false; + attributes.alpha = false; + attributes.premultiplied_alpha = false; + + let size = Size2D::new(256, 256); + let context = GLContext::::new(size, + attributes.clone(), + ColorAttachmentType::Texture, + None).unwrap(); + + let handle = context.handle(); + + GLContext::::new(size, + attributes.clone(), + ColorAttachmentType::Texture, + Some(&handle)).unwrap(); + + GLContext::::new(size, + attributes.clone(), + ColorAttachmentType::Texture, + Some(&handle)).unwrap(); +} From 90e8d1ea1d7096468f0f6d18a9a7dba025a6718a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 27 Aug 2016 01:54:50 -0700 Subject: [PATCH 2/2] Clamp draw buffer size to a minimum, in order to avoid crashes. When a framebuffer size is zero the reported status by OpenGL is (correctly) not FRAMEBUFFER_COMPLETE. We can't avoid creating a draw buffer for this, since we need to be able to execute actual GL calls. We could return an error, but I don't think that's what other implementations do. Instead, clamp the draw buffer size to a minimum of 16 pixels. --- src/draw_buffer.rs | 16 +++++++++++++--- src/gl_context.rs | 4 ++-- src/gl_context_capabilities.rs | 2 +- src/tests.rs | 10 ++++++++++ 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/draw_buffer.rs b/src/draw_buffer.rs index e9233a7..a0b9285 100644 --- a/src/draw_buffer.rs +++ b/src/draw_buffer.rs @@ -82,13 +82,19 @@ fn create_renderbuffer(format: GLenum, impl DrawBuffer { pub fn new(context: &GLContext, - size: Size2D, + mut size: Size2D, color_attachment_type: ColorAttachmentType) - -> Result { - debug!("Creating draw buffer {:?}, {:?}", size, color_attachment_type); + -> Result + { + 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: {:?}", + size, color_attachment_type, attrs, capabilities); + if attrs.antialias && capabilities.max_samples == 0 { return Err("The given GLContext doesn't support requested antialising"); } @@ -97,6 +103,10 @@ impl DrawBuffer { return Err("preserveDrawingBuffer is not supported yet"); } + // See https://github.com/servo/servo/issues/12320 + size.width = cmp::max(MIN_DRAWING_BUFFER_SIZE, size.width); + size.height = cmp::max(MIN_DRAWING_BUFFER_SIZE, size.height); + let mut draw_buffer = DrawBuffer { size: size, framebuffer: 0, diff --git a/src/gl_context.rs b/src/gl_context.rs index 23db5e5..3613f9c 100644 --- a/src/gl_context.rs +++ b/src/gl_context.rs @@ -100,7 +100,6 @@ impl GLContext self.native_context.handle() } - // Allow borrowing these unmutably pub fn borrow_attributes(&self) -> &GLContextAttributes { &self.attributes @@ -142,7 +141,8 @@ impl GLContext // in order to keep this generic pub fn resize(&mut self, size: Size2D) -> Result<(), &'static str> { if self.draw_buffer.is_some() { - let color_attachment_type = self.borrow_draw_buffer().unwrap().color_attachment_type(); + let color_attachment_type = + self.borrow_draw_buffer().unwrap().color_attachment_type(); self.create_draw_buffer(size, color_attachment_type) } else { Err("No DrawBuffer found") diff --git a/src/gl_context_capabilities.rs b/src/gl_context_capabilities.rs index 56a3a26..2604371 100644 --- a/src/gl_context_capabilities.rs +++ b/src/gl_context_capabilities.rs @@ -8,7 +8,7 @@ use GLFeature; /// should have under the field `capabilities`, as a public field /// This should allow us to know the capabilities of a given /// GLContext without repeating the same code over and over -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub struct GLContextCapabilities { // max antialising samples, 0 if no antialising supported pub max_samples: GLint, diff --git a/src/tests.rs b/src/tests.rs index a78520b..8c91445 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -257,3 +257,13 @@ fn test_in_a_row() { ColorAttachmentType::Texture, Some(&handle)).unwrap(); } + +#[test] +fn test_zero_size() { + load_gl(); + + GLContext::::new(Size2D::new(0, 320), + GLContextAttributes::default(), + ColorAttachmentType::Texture, + None).unwrap(); +}