Panic when indirect buffer is 0 sized

[schell]

Description
I'm tracking trunk in my project and just hit a panic in changes related to #5714:

thread 'test::unlit_textured_cube_material' panicked at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu-core/src/indirect_validation.rs:288:58:
called `Option::unwrap()` on a `None` value

I'm guessing it's this binding binding_size on line 276 in the file mentioned:

        let binding_size = calculate_src_buffer_binding_size(buffer_size, limits);

being used on line 288:

                size: Some(NonZeroU64::new(binding_size).unwrap()),

...so for some reason in my case binding_size is 0. Maybe the buffer passed is 0? 🤷

My project creates buffers that are zero-sized as a placeholder until there's something to fill them with, at which point they'll be recreated. Is this a generally accepted pattern? If so, we probably need to handle that case?

Just to be clear, this is happening during buffer creation.

Expected vs observed behavior
I would expect to be able to create an indirect draw buffer that is zero sized.

Extra materials
Backtrace:

thread 'test::unlit_textured_cube_material' panicked at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu-core/src/indirect_validation.rs:288:58:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14
   2: core::panicking::panic
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:148:5
   3: core::option::unwrap_failed
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/option.rs:2020:5
   4: core::option::Option<T>::unwrap
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/option.rs:970:21
   5: wgpu_core::indirect_validation::IndirectValidation::create_src_bind_group
             at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu-core/src/indirect_validation.rs:288:28
   6: wgpu_core::device::resource::Device::create_indirect_validation_bind_group
             at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu-core/src/device/resource.rs:780:30
   7: wgpu_core::device::resource::Device::create_buffer
             at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu-core/src/device/resource.rs:627:13
   8: wgpu_core::device::global::<impl wgpu_core::global::Global>::device_create_buffer
             at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu-core/src/device/global.rs:122:32
   9: <wgpu::backend::wgpu_core::ContextWgpuCore as wgpu::context::Context>::device_create_buffer
             at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu/src/backend/wgpu_core.rs:1196:13
  10: <T as wgpu::context::DynContext>::device_create_buffer
             at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu/src/context.rs:1583:20
  11: wgpu::api::device::Device::create_buffer
             at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu/src/api/device.rs:235:20
  12: <wgpu::api::buffer::Buffer as renderling::slab::cpu::IsBuffer>::buffer_create
             at ./src/slab/cpu.rs:283:9
  13: renderling::slab::cpu::SlabAllocator<Buffer>::recreate_buffer
             at ./src/slab/cpu.rs:438:35
  14: renderling::slab::cpu::SlabAllocator<Buffer>::upkeep
             at ./src/slab/cpu.rs:535:18
  15: renderling::draw::cpu::IndirectDraws::sync_with_internal_renderlets
             at ./src/draw/cpu.rs:81:45
  16: renderling::draw::cpu::DrawCalls::upkeep
             at ./src/draw/cpu.rs:278:13
  17: renderling::stage::cpu::Stage::tick_internal
             at ./src/stage/cpu.rs:701:9
  18: renderling::stage::cpu::Stage::render
             at ./src/stage/cpu.rs:734:27
  19: renderling::test::unlit_textured_cube_material
             at ./src/lib.rs:665:9
  20: renderling::test::unlit_textured_cube_material::{{closure}}
             at ./src/lib.rs:633:38
  21: core::ops::function::FnOnce::call_once
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:250:5
  22: core::ops::function::FnOnce::call_once
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:250:5

[schell]

Setting all of my managed buffers to start creation as size 1 fixes this issue for me, but is this what is expected?

[sagudev]

Per spec indirect buffer must have non-zero size, but this validation should be done later (in dispatchWorkgroupsIndirect, drawIndirect, ...).

[teoxoy]

I will put up a fix for this tomorrow.

[schell]

@sagudev Can you point me to the part of the spec that says the indirect buffer must be non-zero 🙏 ?

Of course I believe you, but I need practice reading the spec and can't seem to find that requirement. I'm looking at https://www.w3.org/TR/webgpu/#dom-gpurendercommandsmixin-drawindirect.

[kpreid]

The spec says:

2. If any of the following conditions are unsatisfied, invalidate this and return.
   • indirectOffset + sizeof(indirect draw parameters) ≤ indirectBuffer.size.

The LHS of that expression is at least 16 bytes, so every drawIndirect() call will require the indirect buffer to be at least 16 bytes in size. Of course, that should not apply until an actual drawIndirect() call is performed.

[sagudev]

It is not written directly but in device timeline (red) there is this check:

indirectOffset + sizeof(indirect draw parameters) ≤ indirectBuffer.size.

if this condition is not satisfied validation error is generated. Then:

indirect draw parameters encoded in the buffer must be a tightly packed block of four 32-bit unsigned integer values (16 bytes total)

So the condition is actually indirectOffset + 16 ≤ indirectBuffer.size, which implies that 0 ≤16 ≤ indirectBuffer.size.

[schell]

Thanks @kpreid and @sagudev .

That's interesting in that then wouldn't the smallest indirect buffer size be 16? But the fix for me was to set my buffers to size 1. So maybe the validation still isn't right?

Also, I guess I had read indirect parameters to implicitly include the number of parametered calls. Like number of calls * size of one call. In fact I think that's what's implied, because if you have an offset, you need the buffer to be offset + number of calls * sizeof(call) to be <= buffer .size so that you don't read past the end of the buffer. Interpreting this as only offset + sizeof(call) assumes there's only ever one call, which is obviously incorrect - the buffer must be bigger than that if you have 2 or more calls.

So if it is to be interpreted as len(call) * sizeof(call) then 0 should be fine.

But maybe I don't understand the problem?

[sagudev]

The problem is that previous impl failed to early (in creating buffer where no size validation should occur) instead of where buffer is actually used.

[schell]

@sagudev right, that makes sense. I'm just wondering why setting the buffer size to 1 would make it pass validation if the lower limit is 16.

I'm also not totally convinced that a buffer size of 0 is invalid by the spec (so long as the draw call count is 0), as I'm not clear on what's intended by "sizeof([indirect draw parameters](https://www.w3.org/TR/webgpu/#indirect-draw-parameters))". But as I said earlier I need practice reading the spec 🙇

[kpreid]

The validation actually in the spec can be described as: when drawIndirect() is called, it must check that the portion of the buffer it is to read is not outside of the bounds of the buffer — nothing more than that. If there are zero calls, there should not be an error because that particular validation never runs. If there are one or more calls, then the buffer must be big enough for each of those calls’ offsets. There is no “number of calls” counting involved, because each call can have whatever offset, and thus required size, it likes.

[schell]

Thanks for explaining, @kpreid 🙇 :)