Skip to content

Commit

Permalink
re_renderer: detect badly sized texture buffers before handing to wgpu (
Browse files Browse the repository at this point in the history
#4005)

### What
* Closes #3927

wgpu does no validation right away, but defer it to a background thread,
leading to lack of good error messages, and sometimes panics.

So we need to do the best we can up-front!

I tested this with:

```diff
--- a/docs/code-examples/image_simple.cpp
+++ b/docs/code-examples/image_simple.cpp
@@ -21,5 +21,5 @@ int main() {
         }
     }
 
-    rec.log("image", rerun::Image({HEIGHT, WIDTH, 3}, std::move(data)));
+    rec.log("image", rerun::Image({HEIGHT + 100, WIDTH, 3}, std::move(data)));
 }
```

Result:

> [2023-10-25T13:10:56Z ERROR re_space_view_spatial::parts::images]
Failed to create texture for "image": Wrong number of bytes in the
texture data buffer - expected 300x300x4=360000, got 240000



### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/4005) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4005)
- [Docs
preview](https://rerun.io/preview/d94937b6991a03e998e9106bdf51e7390263b4ff/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/d94937b6991a03e998e9106bdf51e7390263b4ff/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
  • Loading branch information
emilk authored Oct 26, 2023
1 parent fab45c3 commit 794b671
Showing 1 changed file with 54 additions and 16 deletions.
70 changes: 54 additions & 16 deletions crates/re_renderer/src/resource_managers/texture_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,18 @@ pub enum TextureCreationError {
max_texture_dimension_2d: u32,
},

#[error(
"Wrong number of bytes in the texture data buffer - expected {width}x{height}x{bytes_per_texel}={expected_bytes}, got {actual_bytes}"
)]
WrongBufferSize {
width: u32,
height: u32,
bytes_per_texel: u32,
expected_bytes: usize, // product of the avbove

actual_bytes: usize,
},

#[error(
"Texture with debug label {label:?} has a format {format:?} that data can't be transferred to!"
)]
Expand Down Expand Up @@ -346,55 +358,81 @@ impl TextureManager2D {
) -> Result<GpuTexture2D, TextureCreationError> {
re_tracing::profile_function!();

if creation_desc.width == 0 || creation_desc.height == 0 {
return Err(TextureCreationError::ZeroSize(creation_desc.label.clone()));
let Texture2DCreationDesc {
label,
data,
format,
width,
height,
} = creation_desc;
let (width, height, format) = (*width, *height, *format);

if width == 0 || height == 0 {
return Err(TextureCreationError::ZeroSize(label.clone()));
}

let max_texture_dimension_2d = device.limits().max_texture_dimension_2d;
if creation_desc.width > max_texture_dimension_2d
|| creation_desc.height > max_texture_dimension_2d
{
if width > max_texture_dimension_2d || height > max_texture_dimension_2d {
return Err(TextureCreationError::TooLarge {
width: creation_desc.width,
height: creation_desc.height,
width,
height,
max_texture_dimension_2d,
});
}

if !format.is_compressed() {
if let Some(bytes_per_texel) = creation_desc
.format
.block_size(Some(wgpu::TextureAspect::All))
{
let expected_bytes = width as usize * height as usize * bytes_per_texel as usize;

if data.len() != expected_bytes {
return Err(TextureCreationError::WrongBufferSize {
width,
height,
bytes_per_texel,
expected_bytes,

actual_bytes: data.len(),
});
}
}
}

let size = wgpu::Extent3d {
width: creation_desc.width,
height: creation_desc.height,
width,
height,
depth_or_array_layers: 1,
};
let texture = texture_pool.alloc(
device,
&TextureDesc {
label: creation_desc.label.clone(),
label: label.clone(),
size,
mip_level_count: 1, // TODO(andreas)
sample_count: 1,
dimension: wgpu::TextureDimension::D2,
format: creation_desc.format,
format,
usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST,
},
);

let width_blocks = creation_desc.width / creation_desc.format.block_dimensions().0;
let width_blocks = width / format.block_dimensions().0;
let block_size = creation_desc
.format
.block_size(Some(wgpu::TextureAspect::All))
.ok_or_else(|| TextureCreationError::UnsupportedFormatForTransfer {
label: creation_desc.label.clone(),
format: creation_desc.format,
label: label.clone(),
format,
})?;
let bytes_per_row_unaligned = width_blocks * block_size;

// TODO(andreas): Once we have our own temp buffer for uploading, we can do the padding inplace
// I.e. the only difference will be if we do one memcopy or one memcopy per row, making row padding a nuisance!
let data: &[u8] = creation_desc.data.as_ref();
let data: &[u8] = data.as_ref();

// TODO(andreas): temp allocator for staging data?
// We don't do any further validation of the buffer here as wgpu does so extensively.
re_tracing::profile_scope!("write_texture");
queue.write_texture(
wgpu::ImageCopyTexture {
Expand Down

0 comments on commit 794b671

Please sign in to comment.