Conversation
|
I tested this and it works, and gave it a quick look over and it seems reasonable. |
|
|
||
| // Wait for all GPU work — submissions *and* any pending presentations — | ||
| // to complete before releasing resources. | ||
| match unsafe { self.raw.wait_for_idle() } { |
There was a problem hiding this comment.
No timeout here may be a problem in some cases?
|
@cwfitzgerald Can you have copilot review this |
There was a problem hiding this comment.
Pull request overview
Reworks surface presentation so that presenting is a queue operation (instead of being driven by SurfaceTexture/surface detail), adds backend support for blocking until presents complete, and fixes Vulkan correctness/UB around presenting unused/uninitialized surface textures and dropping queues with in-flight presents.
Changes:
- Move presentation to
Queue::present, removingSurfaceTexture::presentand backendSurfaceOutputDetailInterface::present. - Add
hal::Queue::wait_for_idleacross backends and use it to safely wait for present completion (including on queue drop). - Add core-side logic to clear/transition acquired-but-unused surface textures before present, and track present lifetimes until completion.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wgpu/src/dispatch.rs | Adds QueueInterface::present and removes surface-output-detail present hook. |
| wgpu/src/backend/wgpu_core.rs | Routes presentation through CoreQueue::present; removes now-unused error sink from surface output detail. |
| wgpu/src/backend/webgpu.rs | Implements no-op QueueInterface::present for web; removes output-detail present hook. |
| wgpu/src/api/surface_texture.rs | Removes SurfaceTexture::present API; relies on Drop discard when not presented. |
| wgpu/src/api/surface.rs | Updates presentation docs to reference Queue::present. |
| wgpu/src/api/queue.rs | Introduces Queue::present(SurfaceTexture) -> SubmissionIndex API. |
| wgpu-hal/src/lib.rs | Adds Queue::wait_for_idle to hal trait to support waiting for presents. |
| wgpu-hal/src/dynamic/queue.rs | Plumbs wait_for_idle through the dynamic queue abstraction. |
| wgpu-hal/src/vulkan/mod.rs | Implements wait_for_idle via vkQueueWaitIdle. |
| wgpu-hal/src/metal/mod.rs | Implements wait_for_idle by committing an empty command buffer and waiting for completion. |
| wgpu-hal/src/dx12/mod.rs | Implements wait_for_idle with a fence signal + wait. |
| wgpu-hal/src/gles/queue.rs | Implements wait_for_idle via gl.finish(). |
| wgpu-hal/src/noop/mod.rs | Implements wait_for_idle as a no-op. |
| wgpu-core/src/present.rs | Moves present logic onto Queue, returns a SubmissionIndex, and tracks presented textures for safe lifetime. |
| wgpu-core/src/device/queue.rs | Adds waiting-for-present logic, queue-drop idle wait, and “unused surface texture” prepare/mini-submit path. |
| wgpu-core/src/device/life.rs | Tracks pending present textures and releases them once completion conditions are met. |
| wgpu-core/src/device/resource.rs | Adds maintain-time handling intended to resolve pending presents on waits. |
| wgpu-core/src/device/trace.rs | Extends trace Action::Present to include a submission index. |
| wgpu-core/src/device/trace/record.rs | Updates trace record conversion for the new Present action shape. |
| player/src/lib.rs | Updates pattern match for the new traced Present action signature. |
| player/src/bin/play.rs | Updates replay logic to present via queue/surface and handle new trace action signature. |
| examples/standalone/custom_backend/src/custom.rs | Extends custom backend queue trait implementation with present. |
| examples/standalone/02_hello_window/src/main.rs | Migrates example to queue.present(surface_texture). |
| examples/features/src/framework.rs | Migrates example framework to queue.present(frame). |
| examples/features/src/hello_triangle/mod.rs | Migrates example to queue.present(frame). |
| examples/features/src/hello_windows/mod.rs | Migrates example to queue.present(frame). |
| examples/features/src/uniform_values/mod.rs | Migrates example to queue.present(frame). |
| examples/bug-repro/01_texture_atomic_bug/src/main.rs | Migrates repro to queue.present(frame). |
| examples/bug-repro/02_present_bugs/src/main.rs | Adds new repro for presenting unused textures and dropping queue after present. |
| examples/bug-repro/02_present_bugs/Cargo.toml | Adds new repro crate manifest. |
| Cargo.lock | Adds lockfile entries for the new repro crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(present_index) = queue.lock_life().oldest_pending_present_index() { | ||
| if present_index <= wait_target { | ||
| if let Err(e) = queue.wait_for_present(present_index) { | ||
| let hal_error: WaitIdleError = e.into(); | ||
| return (user_closures, Err(hal_error)); | ||
| } | ||
| queue_empty = queue.lock_life().queue_empty(); | ||
| } |
| // Assign a fresh submission index to this present so it is distinct | ||
| // from any prior real queue submission. This means: | ||
| // | ||
| // - Waiting for the *submission* index (N) will not accidentally | ||
| // trigger present resolution — the present is at N+1, which is | ||
| // strictly after N. | ||
| // - Waiting for the *present* index (N+1) will correctly block | ||
| // until the GPU is done with the presentation. | ||
| // | ||
| // We also update `last_successful_submission_index` so that the normal | ||
| // `Device::maintain` wait-validity check does not reject a poll for | ||
| // this index. | ||
| let present_index = { | ||
| let mut indices = device.command_indices.write(); | ||
| indices.active_submission_index += 1; | ||
| indices.active_submission_index | ||
| }; | ||
|
|
||
| match result { | ||
| Ok(()) => { | ||
| device | ||
| .last_successful_submission_index | ||
| .store(present_index, Ordering::Release); | ||
| self.lock_life().track_present(present_index, texture); | ||
| Ok((Status::Good, present_index)) |
wgpu/src/api/surface.rs
Outdated
| /// In order to present the [`SurfaceTexture`] returned by this method, | ||
| /// first a [`Queue::submit`] needs to be done with some work rendering to this texture. | ||
| /// Then [`SurfaceTexture::present`] needs to be called. | ||
| /// Then [`Queue::present`] needs to be called. |
| /// A presentation is complete once a real queue submission with an index | ||
| /// strictly greater than `present_index` has finished executing on the GPU. | ||
| /// If such a submission has already been issued, this waits for its fence. | ||
| /// Otherwise — no subsequent submission has been made — the whole queue is | ||
| /// drained via [`wait_for_idle`][hal::Queue::wait_for_idle]. | ||
| pub(crate) fn wait_for_present( | ||
| &self, | ||
| present_index: SubmissionIndex, | ||
| ) -> Result<(), DeviceError> { | ||
| let last_successful = self | ||
| .device | ||
| .last_successful_submission_index | ||
| .load(Ordering::Acquire); | ||
|
|
||
| if last_successful > present_index { | ||
| // A submission made after the present has completed (or is in | ||
| // flight); wait for the fence to reach that index. | ||
| let fence = self.device.fence.read(); | ||
| unsafe { | ||
| self.device | ||
| .raw() | ||
| .wait(fence.as_ref(), last_successful, None) | ||
| .map(|_| ()) | ||
| .map_err(|e| self.device.handle_hal_error(e)) | ||
| } | ||
| } else { | ||
| // No submission has been made after the present yet; drain the | ||
| // entire queue so we know the present is complete. | ||
| unsafe { self.raw().wait_for_idle() }.map_err(|e| self.device.handle_hal_error(e)) | ||
| } |
| } else { | ||
| // No submission has been made after the present yet; drain the | ||
| // entire queue so we know the present is complete. | ||
| unsafe { self.raw().wait_for_idle() }.map_err(|e| self.device.handle_hal_error(e)) |
|
@teoxoy I assigned this to you, if you can't take it, feel free to unassign yourself. |
725134e to
34745e8
Compare
|
@teoxoy I just sent this to a matrix account but you have 2 and I'm not sure if I sent it to the right one, plsu thats probably not the right place to discuss. Anyway, what do you think about making Surface::get_current_texture be a queue operation instead? I think it can potentially be blocking and depends on the queue timeline (a semaphore). Also we should perhaps consider adding a method to wait for the surface texture to be ready/query if its ready, or even make it an async operation. P.S. the same probably goes for |
|
IIRC |
Connections
Closes #9109
CC @teoxoy
Description
One question: I think surface texture acquisition should also be a queue operation since it may end up waiting for previous presents/whatever. Do you agree?
This was primarily written by Claude, though I looked over everything, tested it, and I generally guided Claude throughout this knowing the issues myself.
Testing
Manual test
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.