From 614dc363abbc8778b1e4263b00b36438973fdeed Mon Sep 17 00:00:00 2001 From: lawnjelly Date: Mon, 14 Feb 2022 10:56:06 +0000 Subject: [PATCH] Fix GL buffer upload size bugs Wrapper functions for uploading buffers to OpenGL take all sizes and offsets in bytes. Some buffer sizes are specified as units (e.g. float) so require conversion to bytes when calling the buffer upload functions. Two such bugs have been fixed in blendshapes, and parameter names and comments have been changed to emphasize that sizes should be in bytes. In addition DEV_ASSERTS in the upload wrappers have been changed to ERR_FAIL. --- drivers/gles2/rasterizer_storage_gles2.cpp | 4 ++-- drivers/gles2/rasterizer_storage_gles2.h | 18 ++++++++------- drivers/gles3/rasterizer_storage_gles3.h | 26 ++++++++++++---------- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/drivers/gles2/rasterizer_storage_gles2.cpp b/drivers/gles2/rasterizer_storage_gles2.cpp index 10e45fc637e..80a6bf88230 100644 --- a/drivers/gles2/rasterizer_storage_gles2.cpp +++ b/drivers/gles2/rasterizer_storage_gles2.cpp @@ -3970,7 +3970,7 @@ void RasterizerStorageGLES2::update_dirty_blend_shapes() { s->blend_shape_buffer_size = buffer_size; glBufferData(GL_ARRAY_BUFFER, buffer_size * sizeof(float), transform_buffer.read().ptr(), GL_DYNAMIC_DRAW); } else { - buffer_orphan_and_upload(s->blend_shape_buffer_size, 0, buffer_size * sizeof(float), transform_buffer.read().ptr(), GL_ARRAY_BUFFER, true); + buffer_orphan_and_upload(s->blend_shape_buffer_size * sizeof(float), 0, buffer_size * sizeof(float), transform_buffer.read().ptr(), GL_ARRAY_BUFFER, true); } glBindBuffer(GL_ARRAY_BUFFER, 0); } @@ -3992,7 +3992,7 @@ void RasterizerStorageGLES2::_update_skeleton_transform_buffer(const PoolVector< glBufferData(GL_ARRAY_BUFFER, buffer_size, p_data.read().ptr(), GL_DYNAMIC_DRAW); } else { // this may not be best, it could be better to use glBufferData in both cases. - buffer_orphan_and_upload(resources.skeleton_transform_buffer_size, 0, buffer_size, p_data.read().ptr(), GL_ARRAY_BUFFER, true); + buffer_orphan_and_upload(resources.skeleton_transform_buffer_size * sizeof(float), 0, buffer_size, p_data.read().ptr(), GL_ARRAY_BUFFER, true); } glBindBuffer(GL_ARRAY_BUFFER, 0); diff --git a/drivers/gles2/rasterizer_storage_gles2.h b/drivers/gles2/rasterizer_storage_gles2.h index 77046c053c2..7dbf56501bf 100644 --- a/drivers/gles2/rasterizer_storage_gles2.h +++ b/drivers/gles2/rasterizer_storage_gles2.h @@ -1356,7 +1356,8 @@ public: virtual String get_video_adapter_name() const; virtual String get_video_adapter_vendor() const; - void buffer_orphan_and_upload(unsigned int p_buffer_size, unsigned int p_offset, unsigned int p_data_size, const void *p_data, GLenum p_target = GL_ARRAY_BUFFER, GLenum p_usage = GL_DYNAMIC_DRAW, bool p_optional_orphan = false) const; + // NOTE : THESE SIZES ARE IN BYTES. BUFFER SIZES MAY NOT BE SPECIFIED IN BYTES SO REMEMBER TO CONVERT THEM WHEN CALLING. + void buffer_orphan_and_upload(unsigned int p_buffer_size_bytes, unsigned int p_offset_bytes, unsigned int p_data_size_bytes, const void *p_data, GLenum p_target = GL_ARRAY_BUFFER, GLenum p_usage = GL_DYNAMIC_DRAW, bool p_optional_orphan = false) const; bool safe_buffer_sub_data(unsigned int p_total_buffer_size, GLenum p_target, unsigned int p_offset, unsigned int p_data_size, const void *p_data, unsigned int &r_offset_after) const; RasterizerStorageGLES2(); @@ -1376,17 +1377,18 @@ inline bool RasterizerStorageGLES2::safe_buffer_sub_data(unsigned int p_total_bu // standardize the orphan / upload in one place so it can be changed per platform as necessary, and avoid future // bugs causing pipeline stalls -inline void RasterizerStorageGLES2::buffer_orphan_and_upload(unsigned int p_buffer_size, unsigned int p_offset, unsigned int p_data_size, const void *p_data, GLenum p_target, GLenum p_usage, bool p_optional_orphan) const { +// NOTE : THESE SIZES ARE IN BYTES. BUFFER SIZES MAY NOT BE SPECIFIED IN BYTES SO REMEMBER TO CONVERT THEM WHEN CALLING. +inline void RasterizerStorageGLES2::buffer_orphan_and_upload(unsigned int p_buffer_size_bytes, unsigned int p_offset_bytes, unsigned int p_data_size_bytes, const void *p_data, GLenum p_target, GLenum p_usage, bool p_optional_orphan) const { // Orphan the buffer to avoid CPU/GPU sync points caused by glBufferSubData // Was previously #ifndef GLES_OVER_GL however this causes stalls on desktop mac also (and possibly other) if (!p_optional_orphan || (config.should_orphan)) { - glBufferData(p_target, p_buffer_size, nullptr, p_usage); + glBufferData(p_target, p_buffer_size_bytes, nullptr, p_usage); #ifdef RASTERIZER_EXTRA_CHECKS // fill with garbage off the end of the array - if (p_buffer_size) { - unsigned int start = p_offset + p_data_size; + if (p_buffer_size_bytes) { + unsigned int start = p_offset_bytes + p_data_size_bytes; unsigned int end = start + 1024; - if (end < p_buffer_size) { + if (end < p_buffer_size_bytes) { uint8_t *garbage = (uint8_t *)alloca(1024); for (int n = 0; n < 1024; n++) { garbage[n] = Math::random(0, 255); @@ -1396,8 +1398,8 @@ inline void RasterizerStorageGLES2::buffer_orphan_and_upload(unsigned int p_buff } #endif } - DEV_ASSERT((p_offset + p_data_size) <= p_buffer_size); - glBufferSubData(p_target, p_offset, p_data_size, p_data); + ERR_FAIL_COND((p_offset_bytes + p_data_size_bytes) > p_buffer_size_bytes); + glBufferSubData(p_target, p_offset_bytes, p_data_size_bytes, p_data); } #endif // RASTERIZERSTORAGEGLES2_H diff --git a/drivers/gles3/rasterizer_storage_gles3.h b/drivers/gles3/rasterizer_storage_gles3.h index 1490a24385f..5b63a05b0e3 100644 --- a/drivers/gles3/rasterizer_storage_gles3.h +++ b/drivers/gles3/rasterizer_storage_gles3.h @@ -1506,36 +1506,38 @@ public: virtual String get_video_adapter_name() const; virtual String get_video_adapter_vendor() const; - void buffer_orphan_and_upload(unsigned int p_buffer_size, unsigned int p_offset, unsigned int p_data_size, const void *p_data, GLenum p_target = GL_ARRAY_BUFFER, GLenum p_usage = GL_DYNAMIC_DRAW, bool p_optional_orphan = false) const; - bool safe_buffer_sub_data(unsigned int p_total_buffer_size, GLenum p_target, unsigned int p_offset, unsigned int p_data_size, const void *p_data, unsigned int &r_offset_after) const; + // NOTE : THESE SIZES ARE IN BYTES. BUFFER SIZES MAY NOT BE SPECIFIED IN BYTES SO REMEMBER TO CONVERT THEM WHEN CALLING. + void buffer_orphan_and_upload(unsigned int p_buffer_size_bytes, unsigned int p_offset_bytes, unsigned int p_data_size_bytes, const void *p_data, GLenum p_target = GL_ARRAY_BUFFER, GLenum p_usage = GL_DYNAMIC_DRAW, bool p_optional_orphan = false) const; + bool safe_buffer_sub_data(unsigned int p_total_buffer_size_bytes, GLenum p_target, unsigned int p_offset_bytes, unsigned int p_data_size_bytes, const void *p_data, unsigned int &r_offset_after_bytes) const; RasterizerStorageGLES3(); ~RasterizerStorageGLES3(); }; -inline bool RasterizerStorageGLES3::safe_buffer_sub_data(unsigned int p_total_buffer_size, GLenum p_target, unsigned int p_offset, unsigned int p_data_size, const void *p_data, unsigned int &r_offset_after) const { - r_offset_after = p_offset + p_data_size; +inline bool RasterizerStorageGLES3::safe_buffer_sub_data(unsigned int p_total_buffer_size_bytes, GLenum p_target, unsigned int p_offset_bytes, unsigned int p_data_size_bytes, const void *p_data, unsigned int &r_offset_after_bytes) const { + r_offset_after_bytes = p_offset_bytes + p_data_size_bytes; #ifdef DEBUG_ENABLED // we are trying to write across the edge of the buffer - if (r_offset_after > p_total_buffer_size) { + if (r_offset_after_bytes > p_total_buffer_size_bytes) { return false; } #endif - glBufferSubData(p_target, p_offset, p_data_size, p_data); + glBufferSubData(p_target, p_offset_bytes, p_data_size_bytes, p_data); return true; } // standardize the orphan / upload in one place so it can be changed per platform as necessary, and avoid future // bugs causing pipeline stalls -inline void RasterizerStorageGLES3::buffer_orphan_and_upload(unsigned int p_buffer_size, unsigned int p_offset, unsigned int p_data_size, const void *p_data, GLenum p_target, GLenum p_usage, bool p_optional_orphan) const { +// NOTE : THESE SIZES ARE IN BYTES. BUFFER SIZES MAY NOT BE SPECIFIED IN BYTES SO REMEMBER TO CONVERT THEM WHEN CALLING. +inline void RasterizerStorageGLES3::buffer_orphan_and_upload(unsigned int p_buffer_size_bytes, unsigned int p_offset_bytes, unsigned int p_data_size_bytes, const void *p_data, GLenum p_target, GLenum p_usage, bool p_optional_orphan) const { // Orphan the buffer to avoid CPU/GPU sync points caused by glBufferSubData // Was previously #ifndef GLES_OVER_GL however this causes stalls on desktop mac also (and possibly other) if (!p_optional_orphan || (config.should_orphan)) { - glBufferData(p_target, p_buffer_size, nullptr, p_usage); + glBufferData(p_target, p_buffer_size_bytes, nullptr, p_usage); #ifdef RASTERIZER_EXTRA_CHECKS // fill with garbage off the end of the array - if (p_buffer_size) { - unsigned int start = p_offset + p_data_size; + if (p_buffer_size_bytes) { + unsigned int start = p_offset_bytes + p_data_size_bytes; unsigned int end = start + 1024; if (end < p_buffer_size) { uint8_t *garbage = (uint8_t *)alloca(1024); @@ -1547,8 +1549,8 @@ inline void RasterizerStorageGLES3::buffer_orphan_and_upload(unsigned int p_buff } #endif } - DEV_ASSERT((p_offset + p_data_size) <= p_buffer_size); - glBufferSubData(p_target, p_offset, p_data_size, p_data); + ERR_FAIL_COND((p_offset_bytes + p_data_size_bytes) > p_buffer_size_bytes); + glBufferSubData(p_target, p_offset_bytes, p_data_size_bytes, p_data); } #endif // RASTERIZERSTORAGEGLES3_H