From 6c7fe43734663cb89fb61a2933a302462a0bc139 Mon Sep 17 00:00:00 2001 From: Dimitri Sabadie Date: Tue, 4 May 2021 01:17:50 +0200 Subject: [PATCH 1/4] [luminance-webgl] Fix buffer target corruption in WebGL2 backend. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the OpenGL 3.3 backend, there is an optimization that allows a buffer object to be created and bound to a given target (for instance `ARRAY_BUFFER`) and allows to rebind the buffer later to another target (for instance `ELEMENT_ARRAY_BUFFER`). That optimization is handy because it allows to dissociate data storage from data representation, allowing to send a bunch of bytes and then using the target as source of representation. Well, turns out that optimization doesn’t work in WebGL2, so this commit implements a way to keep track of what a buffer was created as and then re-use that information every time we need to bind the buffer. That should fix #483 (and probably other bugs no one has encountered yet). Some integration tests are needed to ensure the fix does what it should. --- luminance-webgl/src/webgl2/buffer.rs | 29 +++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/luminance-webgl/src/webgl2/buffer.rs b/luminance-webgl/src/webgl2/buffer.rs index ee0d6ead..b1f1c311 100644 --- a/luminance-webgl/src/webgl2/buffer.rs +++ b/luminance-webgl/src/webgl2/buffer.rs @@ -20,6 +20,11 @@ use luminance::buffer::BufferError; #[derive(Clone, Debug)] struct BufferWrapper { handle: WebGlBuffer, + /// Target the buffer was created with; WebGL2 doesn’t play well with rebinding a buffer to a different target (unlike + /// OpenGL, in which that optimization is handy). + /// + /// See https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/bindBuffer#exceptions for further details + target: u32, state: Rc>, } @@ -70,6 +75,7 @@ impl Buffer { let gl_buf = BufferWrapper { handle, + target, state: webgl2.state.clone(), }; @@ -98,6 +104,7 @@ impl Buffer { let gl_buf = BufferWrapper { handle, + target, state: webgl2.state.clone(), }; @@ -177,6 +184,7 @@ where let mut state = buffer.gl_buf.state.borrow_mut(); let bytes = mem::size_of::() * buffer_len; update_webgl_buffer( + buffer.gl_buf.target, &mut state, &buffer.gl_buf.handle, buffer.buf.as_ptr() as *const u8, @@ -210,6 +218,7 @@ where let mut state = buffer.gl_buf.state.borrow_mut(); let bytes = mem::size_of::() * buffer_len; update_webgl_buffer( + buffer.gl_buf.target, &mut state, &buffer.gl_buf.handle, buffer.buf.as_ptr() as *const u8, @@ -229,6 +238,7 @@ where let mut state = buffer.gl_buf.state.borrow_mut(); let bytes = buffer.buf.len() * mem::size_of::(); update_webgl_buffer( + buffer.gl_buf.target, &mut state, &buffer.gl_buf.handle, buffer.buf.as_ptr() as *const u8, @@ -280,6 +290,7 @@ impl Deref for BufferSlice { /// When a buffer is mapped, we are the only owner of it. We can then read or write from/to the /// mapped buffer, and then update the GPU buffer on the [`Drop`] implementation. pub struct BufferSliceMutWrapper { + target: u32, handle: WebGlBuffer, ptr: *mut u8, bytes: usize, @@ -289,7 +300,14 @@ pub struct BufferSliceMutWrapper { impl Drop for BufferSliceMutWrapper { fn drop(&mut self) { let mut state = self.state.borrow_mut(); - update_webgl_buffer(&mut state, &self.handle, self.ptr, self.bytes, 0); + update_webgl_buffer( + self.target, + &mut state, + &self.handle, + self.ptr, + self.bytes, + 0, + ); } } @@ -355,6 +373,7 @@ where buffer: &mut Self::BufferRepr, ) -> Result { let raw = BufferSliceMutWrapper { + target: buffer.gl_buf.target, handle: buffer.gl_buf.handle.clone(), ptr: buffer.buf.as_mut_ptr() as *mut u8, bytes: buffer.buf.len() * mem::size_of::(), @@ -371,6 +390,7 @@ where /// Update a WebGL buffer by copying an input slice. fn update_webgl_buffer( + target: u32, state: &mut WebGL2State, gl_buf: &WebGlBuffer, data: *const u8, @@ -382,10 +402,5 @@ fn update_webgl_buffer( let data = unsafe { slice::from_raw_parts(data as _, bytes) }; state .ctx - .buffer_sub_data_with_i32_and_u8_array_and_src_offset( - WebGl2RenderingContext::ARRAY_BUFFER, - offset as _, - data, - 0, - ); + .buffer_sub_data_with_i32_and_u8_array_and_src_offset(target, offset as _, data, 0); } From be6df76658076fb151327c95efc09a9e5aff4b9a Mon Sep 17 00:00:00 2001 From: Dimitri Sabadie Date: Mon, 28 Jun 2021 19:41:19 +0200 Subject: [PATCH 2/4] [examples] Add funtest for #483. This relates to #483 for testing corruption of `indices_mut()`. The example showcases the bug when used on the `web` executor (no issue with the `desktop` one). This bug is fixed in the next commit. --- .../src/funtest_483_indices_mut_corruption.rs | 45 +++++++++++++++++++ examples/common/src/lib.rs | 2 + examples/desktop/src/main.rs | 1 + examples/web/src/lib.rs | 1 + 4 files changed, 49 insertions(+) create mode 100644 examples/common/src/funtest_483_indices_mut_corruption.rs diff --git a/examples/common/src/funtest_483_indices_mut_corruption.rs b/examples/common/src/funtest_483_indices_mut_corruption.rs new file mode 100644 index 00000000..0c27d4c5 --- /dev/null +++ b/examples/common/src/funtest_483_indices_mut_corruption.rs @@ -0,0 +1,45 @@ +use crate::{shared::Vertex, Example, InputAction, LoopFeedback, PlatformServices}; +use luminance_front::{context::GraphicsContext, framebuffer::Framebuffer, texture::Dim2, Backend}; +use std::ops::Deref as _; + +pub struct LocalExample; + +impl Example for LocalExample { + fn bootstrap( + _: &mut impl PlatformServices, + context: &mut impl GraphicsContext, + ) -> Self { + let vertices = [ + Vertex::new([1., 2.].into(), [1., 1., 1.].into()), + Vertex::new([-1., 2.].into(), [1., 0., 1.].into()), + Vertex::new([1., -2.].into(), [1., 1., 0.].into()), + ]; + let mut tess = context + .new_tess() + .set_vertices(&vertices[..]) + .set_indices([0u8, 1, 2]) + .set_mode(luminance_front::tess::Mode::Point) + .build() + .expect("tessellation"); + + { + let mut slice = tess.indices_mut().expect("sliced indices"); + log::info!("slice before mutation is: {:?}", slice.deref()); + + slice[1] = 2; + log::info!("slice after mutation is: {:?}", slice.deref()); + } + + LocalExample + } + + fn render_frame( + self, + _: f32, + _: Framebuffer, + _: impl Iterator, + _: &mut impl GraphicsContext, + ) -> LoopFeedback { + LoopFeedback::Exit + } +} diff --git a/examples/common/src/lib.rs b/examples/common/src/lib.rs index c801af87..93933c06 100644 --- a/examples/common/src/lib.rs +++ b/examples/common/src/lib.rs @@ -49,6 +49,8 @@ pub mod vertex_instancing; #[cfg(feature = "funtest")] pub mod funtest_360_manually_drop_framebuffer; #[cfg(feature = "funtest")] +pub mod funtest_483_indices_mut_corruption; +#[cfg(feature = "funtest")] pub mod funtest_flatten_slice; #[cfg(all(feature = "funtest", feature = "funtest-gl33-f64-uniform"))] pub mod funtest_gl33_f64_uniform; diff --git a/examples/desktop/src/main.rs b/examples/desktop/src/main.rs index 7f1066a9..b853741e 100644 --- a/examples/desktop/src/main.rs +++ b/examples/desktop/src/main.rs @@ -210,6 +210,7 @@ examples! { "funtest-360-manually-drop-framebuffer", funtest_360_manually_drop_framebuffer, "funtest-flatten-slice", funtest_flatten_slice, "funtest-pixel-array-encoding", funtest_pixel_array_encoding, + "funtest-483-indices-mut-corruption", funtest_483_indices_mut_corruption, } fn main() { diff --git a/examples/web/src/lib.rs b/examples/web/src/lib.rs index 4ab144b4..bcab2dda 100644 --- a/examples/web/src/lib.rs +++ b/examples/web/src/lib.rs @@ -282,6 +282,7 @@ examples! { "funtest-360-manually-drop-framebuffer", funtest_360_manually_drop_framebuffer, "funtest-flatten-slice", funtest_flatten_slice, "funtest-pixel-array-encoding", funtest_pixel_array_encoding, + "funtest-483-indices-mut-corruption", funtest_483_indices_mut_corruption, } #[wasm_bindgen] From ba6d169f1d1d9ae34c2820c6f825db2602a3405a Mon Sep 17 00:00:00 2001 From: Dimitri Sabadie Date: Mon, 28 Jun 2021 19:45:16 +0200 Subject: [PATCH 3/4] [luminance-webgl] Fix indices_mut() corruption. The bug was due to still using `bind_array_buffer` when streaming buffer updates to the GPU. Relates to #483. --- luminance-webgl/src/webgl2/buffer.rs | 59 ++++++++++++++++------------ 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/luminance-webgl/src/webgl2/buffer.rs b/luminance-webgl/src/webgl2/buffer.rs index b1f1c311..80544ed4 100644 --- a/luminance-webgl/src/webgl2/buffer.rs +++ b/luminance-webgl/src/webgl2/buffer.rs @@ -66,7 +66,7 @@ impl Buffer { .create_buffer() .ok_or_else(|| BufferError::cannot_create())?; - Self::bind(&mut state, &handle, target)?; + bind_buffer(&mut state, &handle, target, Bind::Forced)?; let bytes = mem::size_of::() * len; state @@ -94,7 +94,7 @@ impl Buffer { .create_buffer() .ok_or_else(|| BufferError::cannot_create())?; - Self::bind(&mut state, &handle, target)?; + bind_buffer(&mut state, &handle, target, Bind::Forced)?; let bytes = mem::size_of::() * len; let data = unsafe { slice::from_raw_parts(vec.as_ptr() as *const _, bytes) }; @@ -111,23 +111,6 @@ impl Buffer { Ok(Buffer { gl_buf, buf: vec }) } - /// Bind a buffer to a given state regarding the input target. - fn bind(state: &mut WebGL2State, handle: &WebGlBuffer, target: u32) -> Result<(), BufferError> { - // depending on the buffer target, we are not going to bind it the same way, as the first bind - // is actually meaningful in WebGL2 - match target { - WebGl2RenderingContext::ARRAY_BUFFER => state.bind_array_buffer(Some(handle), Bind::Forced), - WebGl2RenderingContext::ELEMENT_ARRAY_BUFFER => { - state.bind_element_array_buffer(Some(handle), Bind::Forced) - } - - // a bit opaque but should never happen - _ => return Err(BufferError::CannotCreate), - } - - Ok(()) - } - pub(crate) fn handle(&self) -> &WebGlBuffer { &self.gl_buf.handle } @@ -190,7 +173,7 @@ where buffer.buf.as_ptr() as *const u8, bytes, i, - ); + )?; Ok(()) } @@ -224,7 +207,7 @@ where buffer.buf.as_ptr() as *const u8, bytes, 0, - ); + )?; Ok(()) } @@ -244,7 +227,7 @@ where buffer.buf.as_ptr() as *const u8, bytes, 0, - ); + )?; Ok(()) } @@ -300,7 +283,7 @@ pub struct BufferSliceMutWrapper { impl Drop for BufferSliceMutWrapper { fn drop(&mut self) { let mut state = self.state.borrow_mut(); - update_webgl_buffer( + let _ = update_webgl_buffer( self.target, &mut state, &self.handle, @@ -388,19 +371,43 @@ where } } +/// Bind a buffer to a given state regarding the input target. +fn bind_buffer( + state: &mut WebGL2State, + handle: &WebGlBuffer, + target: u32, + bind_mode: Bind, +) -> Result<(), BufferError> { + // depending on the buffer target, we are not going to bind it the same way, as the first bind + // is actually meaningful in WebGL2 + match target { + WebGl2RenderingContext::ARRAY_BUFFER => state.bind_array_buffer(Some(handle), bind_mode), + WebGl2RenderingContext::ELEMENT_ARRAY_BUFFER => { + state.bind_element_array_buffer(Some(handle), bind_mode) + } + + // a bit opaque but should never happen + _ => return Err(BufferError::CannotCreate), + } + + Ok(()) +} + /// Update a WebGL buffer by copying an input slice. fn update_webgl_buffer( target: u32, state: &mut WebGL2State, - gl_buf: &WebGlBuffer, + handle: &WebGlBuffer, data: *const u8, bytes: usize, offset: usize, -) { - state.bind_array_buffer(Some(gl_buf), Bind::Cached); +) -> Result<(), BufferError> { + bind_buffer(state, handle, target, Bind::Cached)?; let data = unsafe { slice::from_raw_parts(data as _, bytes) }; state .ctx .buffer_sub_data_with_i32_and_u8_array_and_src_offset(target, offset as _, data, 0); + + Ok(()) } From e491d413d9eeea02ec4f95d52ab693d568e97987 Mon Sep 17 00:00:00 2001 From: Dimitri Sabadie Date: Mon, 6 Sep 2021 23:13:00 +0200 Subject: [PATCH 4/4] [luminance-webgl] Changelog update. --- luminance-webgl/CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/luminance-webgl/CHANGELOG.md b/luminance-webgl/CHANGELOG.md index dd820de0..fbe2081e 100644 --- a/luminance-webgl/CHANGELOG.md +++ b/luminance-webgl/CHANGELOG.md @@ -11,6 +11,8 @@ compatible with as many crates as possible. In that case, you want `cargo update +* [0.4 + 1](#04--1) + * [Patch changes](#patch-changes) * [0.4](#04) * [0.3.2](#032) * [0.3.1](#031) @@ -25,6 +27,15 @@ compatible with as many crates as possible. In that case, you want `cargo update +# 0.4 + 1 + +> ? + +## Patch changes + +- Fix buffer kind not correctly being used (i.e. mixing vertex and index buffers is not possible, for instance). This + fix was the premise of the full fix, as a redesign of luminance’s buffer interface was needed to fully fix the problem. + # 0.4 > Apr 25, 2021