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

Fix resolve_queries calls per frame causing invalid buffer copy operations #83

Merged
merged 2 commits into from
Sep 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,8 @@ impl GpuProfiler {
continue;
}

assert!(num_resolved_queries < num_used_queries);
debug_assert!(query_pool.capacity >= num_used_queries);
debug_assert!(num_resolved_queries < num_used_queries);

// Resolve into offset 0 of the resolve buffer - this way we don't have to worry about
// the offset restrictions on resolve buffers (`wgpu::QUERY_RESOLVE_BUFFER_ALIGNMENT`)
Expand All @@ -364,14 +365,16 @@ impl GpuProfiler {
&query_pool.resolve_buffer,
0,
);
// Copy the resolved queries into the read buffer, making sure
// Copy the newly resolved queries into the read buffer, making sure
// that we don't override any of the results that are already there.
let destination_offset = (num_resolved_queries * wgpu::QUERY_SIZE) as u64;
let copy_size = ((num_used_queries - num_resolved_queries) * wgpu::QUERY_SIZE) as u64;
encoder.copy_buffer_to_buffer(
&query_pool.resolve_buffer,
0,
&query_pool.read_buffer,
(num_resolved_queries * wgpu::QUERY_SIZE) as u64,
(num_used_queries * wgpu::QUERY_SIZE) as u64,
destination_offset,
copy_size,
);

query_pool
Expand Down
30 changes: 19 additions & 11 deletions tests/src/multiple_resolves_per_frame.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// Regression test for bug described in https://github.com/Wumpf/wgpu-profiler/issues/79
// Regression test for bug described in
// * https://github.com/Wumpf/wgpu-profiler/issues/79
// * https://github.com/Wumpf/wgpu-profiler/issues/82
#[test]
fn multiple_resolves_per_frame() {
const NUM_SCOPES: usize = 1000;

let (_, device, queue) = super::create_device(
wgpu::Features::TIMESTAMP_QUERY.union(wgpu::Features::TIMESTAMP_QUERY_INSIDE_ENCODERS),
)
Expand All @@ -13,14 +17,14 @@ fn multiple_resolves_per_frame() {
let mut encoder = device.create_command_encoder(&wgpu::CommandEncoderDescriptor::default());

// Resolve call per scope.
{
let _ = profiler.scope("testscope0", &mut encoder, &device);
}
profiler.resolve_queries(&mut encoder);
{
let _ = profiler.scope("testscope1", &mut encoder, &device);
// Do this many times to check for potential buffer overflows as found in
// https://github.com/Wumpf/wgpu-profiler/issues/82
for i in 0..NUM_SCOPES {
{
let _ = profiler.scope(format!("{i}"), &mut encoder, &device);
}
profiler.resolve_queries(&mut encoder);
}
profiler.resolve_queries(&mut encoder);

// And an extra resolve for good measure (this should be a no-op).
profiler.resolve_queries(&mut encoder);
Expand All @@ -31,8 +35,12 @@ fn multiple_resolves_per_frame() {
// Poll to explicitly trigger mapping callbacks.
device.poll(wgpu::Maintain::Wait);

// Frame should now be available.
assert!(profiler
// Frame should now be available and contain all the scopes.
let scopes = profiler
.process_finished_frame(queue.get_timestamp_period())
.is_some());
.unwrap();
assert_eq!(scopes.len(), NUM_SCOPES);
for (i, scope) in scopes.iter().enumerate() {
assert_eq!(scope.label, format!("{i}"));
}
}