From a0fc4c98321be216dca4228b91baadc2cb0a9b2a Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Fri, 27 Jul 2018 13:12:25 +0200 Subject: [PATCH] Fix program and shader lifetime cycle --- components/script/dom/webglprogram.rs | 81 ++++++++++++------- .../script/dom/webglrenderingcontext.rs | 29 ++++--- components/script/dom/webglshader.rs | 17 ++-- .../misc/object-deletion-behaviour.html.ini | 4 - .../programs/program-test.html.ini | 13 --- 5 files changed, 80 insertions(+), 64 deletions(-) delete mode 100644 tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini delete mode 100644 tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini diff --git a/components/script/dom/webglprogram.rs b/components/script/dom/webglprogram.rs index 6f371701074..4fb1147e042 100644 --- a/components/script/dom/webglprogram.rs +++ b/components/script/dom/webglprogram.rs @@ -25,7 +25,8 @@ use std::cell::{Cell, Ref}; pub struct WebGLProgram { webgl_object: WebGLObject, id: WebGLProgramId, - is_deleted: Cell, + is_in_use: Cell, + marked_for_deletion: Cell, link_called: Cell, linked: Cell, link_generation: Cell, @@ -40,9 +41,10 @@ impl WebGLProgram { Self { webgl_object: WebGLObject::new_inherited(context), id: id, - is_deleted: Cell::new(false), - link_called: Cell::new(false), - linked: Cell::new(false), + is_in_use: Default::default(), + marked_for_deletion: Default::default(), + link_called: Default::default(), + linked: Default::default(), link_generation: Default::default(), fragment_shader: Default::default(), vertex_shader: Default::default(), @@ -73,25 +75,51 @@ impl WebGLProgram { } /// glDeleteProgram - pub fn delete(&self) { - if !self.is_deleted.get() { - self.is_deleted.set(true); - self.upcast::() - .context() - .send_command(WebGLCommand::DeleteProgram(self.id)); - - if let Some(shader) = self.fragment_shader.get() { - shader.decrement_attached_counter(); - } - - if let Some(shader) = self.vertex_shader.get() { - shader.decrement_attached_counter(); - } + pub fn mark_for_deletion(&self) { + if self.marked_for_deletion.get() { + return; + } + self.marked_for_deletion.set(true); + self.upcast::() + .context() + .send_command(WebGLCommand::DeleteProgram(self.id)); + if self.is_deleted() { + self.detach_shaders(); } } + pub fn in_use(&self, value: bool) { + if self.is_in_use.get() == value { + return; + } + self.is_in_use.set(value); + if self.is_deleted() { + self.detach_shaders(); + } + } + + fn detach_shaders(&self) { + assert!(self.is_deleted()); + if let Some(shader) = self.fragment_shader.get() { + shader.decrement_attached_counter(); + self.fragment_shader.set(None); + } + if let Some(shader) = self.vertex_shader.get() { + shader.decrement_attached_counter(); + self.vertex_shader.set(None); + } + } + + pub fn is_in_use(&self) -> bool { + self.is_in_use.get() + } + + pub fn is_marked_for_deletion(&self) -> bool { + self.marked_for_deletion.get() + } + pub fn is_deleted(&self) -> bool { - self.is_deleted.get() + self.marked_for_deletion.get() && !self.is_in_use.get() } pub fn is_linked(&self) -> bool { @@ -99,10 +127,7 @@ impl WebGLProgram { } /// glLinkProgram - pub fn link(&self) -> WebGLResult<()> { - if self.is_deleted() { - return Err(WebGLError::InvalidOperation); - } + pub fn link(&self) -> WebGLResult<()> { self.linked.set(false); self.link_generation.set(self.link_generation.get().checked_add(1).unwrap()); *self.active_attribs.borrow_mut() = Box::new([]); @@ -214,10 +239,7 @@ impl WebGLProgram { let shader_slot = match shader.gl_type() { constants::FRAGMENT_SHADER => &self.fragment_shader, constants::VERTEX_SHADER => &self.vertex_shader, - _ => { - error!("detachShader: Unexpected shader type"); - return Err(WebGLError::InvalidValue); - } + _ => return Err(WebGLError::InvalidValue), }; match shader_slot.get() { @@ -389,7 +411,7 @@ impl WebGLProgram { } pub fn attached_shaders(&self) -> WebGLResult>> { - if self.is_deleted.get() { + if self.marked_for_deletion.get() { return Err(WebGLError::InvalidValue); } Ok(match (self.vertex_shader.get(), self.fragment_shader.get()) { @@ -408,7 +430,8 @@ impl WebGLProgram { impl Drop for WebGLProgram { fn drop(&mut self) { - self.delete(); + self.in_use(false); + self.mark_for_deletion(); } } diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index 78dde389f04..f5f4674793d 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -2354,11 +2354,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { fn DeleteProgram(&self, program: Option<&WebGLProgram>) { if let Some(program) = program { handle_potential_webgl_error!(self, self.validate_ownership(program), return); - // FIXME: We should call glUseProgram(0), but - // WebGLCommand::UseProgram() doesn't take an Option - // currently. This is also a problem for useProgram(null) - handle_object_deletion!(self, self.current_program, program, None); - program.delete() + program.mark_for_deletion() } } @@ -2366,7 +2362,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { fn DeleteShader(&self, shader: Option<&WebGLShader>) { if let Some(shader) = shader { handle_potential_webgl_error!(self, self.validate_ownership(shader), return); - shader.delete() + shader.mark_for_deletion() } } @@ -2700,8 +2696,12 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9 unsafe fn GetProgramParameter(&self, _: *mut JSContext, program: &WebGLProgram, param: u32) -> JSVal { handle_potential_webgl_error!(self, self.validate_ownership(program), return NullValue()); + if program.is_deleted() { + self.webgl_error(InvalidOperation); + return NullValue(); + } match param { - constants::DELETE_STATUS => BooleanValue(program.is_deleted()), + constants::DELETE_STATUS => BooleanValue(program.is_marked_for_deletion()), constants::LINK_STATUS => BooleanValue(program.is_linked()), constants::VALIDATE_STATUS => { // FIXME(nox): This could be cached on the DOM side when we call validateProgram @@ -2733,7 +2733,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9 unsafe fn GetShaderParameter(&self, _: *mut JSContext, shader: &WebGLShader, param: u32) -> JSVal { handle_potential_webgl_error!(self, self.validate_ownership(shader), return NullValue()); - if shader.is_deleted() && !shader.is_attached() { + if shader.is_deleted() { self.webgl_error(InvalidValue); return NullValue(); } @@ -2903,7 +2903,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9 fn IsShader(&self, shader: Option<&WebGLShader>) -> bool { - shader.map_or(false, |s| !s.is_deleted() || s.is_attached()) + shader.map_or(false, |s| !s.is_deleted()) } // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.8 @@ -3166,6 +3166,9 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9 fn LinkProgram(&self, program: &WebGLProgram) { handle_potential_webgl_error!(self, self.validate_ownership(program), return); + if program.is_deleted() { + return self.webgl_error(InvalidValue); + } handle_potential_webgl_error!(self, program.link()); } @@ -3721,6 +3724,14 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { if program.is_deleted() || !program.is_linked() { return self.webgl_error(InvalidOperation); } + if program.is_in_use() { + return; + } + program.in_use(true); + } + match self.current_program.get() { + Some(ref current) if program != Some(&**current) => current.in_use(false), + _ => {} } self.send_command(WebGLCommand::UseProgram(program.map(|p| p.id()))); self.current_program.set(program); diff --git a/components/script/dom/webglshader.rs b/components/script/dom/webglshader.rs index b85b2d93116..558d43c220c 100644 --- a/components/script/dom/webglshader.rs +++ b/components/script/dom/webglshader.rs @@ -38,7 +38,7 @@ pub struct WebGLShader { gl_type: u32, source: DomRefCell, info_log: DomRefCell, - is_deleted: Cell, + marked_for_deletion: Cell, attached_counter: Cell, compilation_status: Cell, } @@ -58,7 +58,7 @@ impl WebGLShader { gl_type: shader_type, source: Default::default(), info_log: Default::default(), - is_deleted: Cell::new(false), + marked_for_deletion: Cell::new(false), attached_counter: Cell::new(0), compilation_status: Cell::new(ShaderCompilationStatus::NotCompiled), } @@ -101,7 +101,7 @@ impl WebGLShader { limits: &GLLimits, ext: &WebGLExtensions, ) -> WebGLResult<()> { - if self.is_deleted.get() && !self.is_attached() { + if self.marked_for_deletion.get() && !self.is_attached() { return Err(WebGLError::InvalidValue); } if self.compilation_status.get() != ShaderCompilationStatus::NotCompiled { @@ -183,9 +183,9 @@ impl WebGLShader { /// Mark this shader as deleted (if it wasn't previously) /// and delete it as if calling glDeleteShader. /// Currently does not check if shader is attached - pub fn delete(&self) { - if !self.is_deleted.get() { - self.is_deleted.set(true); + pub fn mark_for_deletion(&self) { + if !self.marked_for_deletion.get() { + self.marked_for_deletion.set(true); self.upcast::() .context() .send_command(WebGLCommand::DeleteShader(self.id)); @@ -193,7 +193,7 @@ impl WebGLShader { } pub fn is_deleted(&self) -> bool { - self.is_deleted.get() + self.marked_for_deletion.get() && !self.is_attached() } pub fn is_attached(&self) -> bool { @@ -231,7 +231,6 @@ impl WebGLShader { impl Drop for WebGLShader { fn drop(&mut self) { - assert_eq!(self.attached_counter.get(), 0); - self.delete(); + self.mark_for_deletion(); } } diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini deleted file mode 100644 index e95bb4c4e69..00000000000 --- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini +++ /dev/null @@ -1,4 +0,0 @@ -[object-deletion-behaviour.html] - [WebGL test #17: gl.isProgram(program) should be true. Was false.] - expected: FAIL - diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini deleted file mode 100644 index 164f0e3c2a2..00000000000 --- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini +++ /dev/null @@ -1,13 +0,0 @@ -[program-test.html] - [WebGL test #64: getError expected: NO_ERROR. Was INVALID_OPERATION : delete the current program shouldn't change the current rendering state] - expected: FAIL - - [WebGL test #65: getError expected: NO_ERROR. Was INVALID_OPERATION : The current program shouldn't be deleted] - expected: FAIL - - [WebGL test #69: an attached shader shouldn't be deleted] - expected: FAIL - - [WebGL test #70: getError expected: INVALID_VALUE. Was INVALID_OPERATION : a delete-marked program should be deleted once it's no longer the current program] - expected: FAIL -