Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DX12 Push Constant out of bound #3822

Closed
abdo643-HULK opened this issue May 31, 2023 · 4 comments · Fixed by #4607
Closed

DX12 Push Constant out of bound #3822

abdo643-HULK opened this issue May 31, 2023 · 4 comments · Fixed by #4607
Labels
api: dx12 Issues with DX12 or DXGI type: bug Something isn't working

Comments

@abdo643-HULK
Copy link

Description
I think I found an issue with the current implementation of DX12 push constants. Currently if I try to use anything bigger than 64 bytes wgpu panics but from the docs and also the limits I should be able to use more. I think the problem is how the offset is currently used/calculated on this line.

self.pass.constant_data[(offset as usize)..(offset as usize + data.len())]

The offset is the one originally passed in from the renderpass and that's in bytes but root constants are expected to use 4 byte offsets.

I think a fix for the issue could look something like this:

self.pass.constant_data[((offset / 4) as usize)..((offset / 4) as usize + data.len())]
          .copy_from_slice(data);

Repro steps
This is the project where I able to produce the issue.
https://github.com/abdo643-HULK/Bachelor-Thesis/tree/main/source-code/app

Expected vs observed behavior
Expected: Just pass the offset in bytes like for the Vulkan backend.
Observed: DX12 uses incorrect size/offset.

Extra materials
image

Platform
OS: Windows 11 (build 22621.1778)
wgpu: 0.16.1

This is my first real bug report I hope this is helpful.

@teoxoy teoxoy added type: bug Something isn't working api: dx12 Issues with DX12 or DXGI labels Jun 1, 2023
@initial-algebra
Copy link
Contributor

Confirming that with wgpu 0.18 on Windows 11 (22621.2428) using the DX12 backend, setting push constants doesn't work correctly with a non-zero offset.

let width_bytes = self.width.to_le_bytes();
let height_bytes = self.height.to_le_bytes();
let push_constants_bytes = [
	width_bytes[0],
	width_bytes[1],
	width_bytes[2],
	width_bytes[3],
	height_bytes[0],
	height_bytes[1],
	height_bytes[2],
	height_bytes[3]
];
render_pass.set_push_constants(wgpu::ShaderStages::VERTEX_FRAGMENT, 0, &push_constants_bytes);

The preceding code works fine, but the following code works only with Vulkan.

render_pass.set_push_constants(wgpu::ShaderStages::VERTEX_FRAGMENT, 0, &self.width.to_le_bytes());
render_pass.set_push_constants(wgpu::ShaderStages::VERTEX_FRAGMENT, 4, &self.height.to_le_bytes()); // is read as 0 by the shader

However, I do not experience panicking or validation errors, even though the push constant size limit is set to 8 bytes.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Nov 5, 2023

The offset problem is fixed by #4607 (found it while testing gl), I'm not sure about the out of bounds issue.

@cwfitzgerald
Copy link
Member

The offset problem is fixed by #4607, I'm not sure about the out of bounds issue.

@cwfitzgerald
Copy link
Member

I think they're the same problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: dx12 Issues with DX12 or DXGI type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants