From 7d0fb710a676f49fca65d3ef7995616eff3d5429 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Mon, 17 Aug 2026 13:13:17 +0200 Subject: [PATCH] Bound a readback by time, so a full-resolution export can finish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exporting a real 20 MP CR2 failed every time with "readback did not complete", while the copy itself was perfectly healthy. The bound was 100,000 non-blocking polls. That sounds generous and is not: a `Poll` that finds nothing returns immediately, so the loop spent its entire budget in a few milliseconds. Small transfers — the histogram's 4 KB, a viewport-sized frame — happened to land inside it. An 80 MB frame never could. It is a deadline now, thirty seconds, which is the only thing the bound was ever for: catching a lost device that will never deliver the callback. A one-millisecond pause after the first sixty-four spins stops the loop saturating a core for the length of the copy, while keeping a small transfer as immediate as it was. Found by developing /home/dtourolle/Downloads/_MG_8596.CR2 through the export example: 5472×3648 renders in 127 ms and writes all five formats. Co-Authored-By: Claude Opus 5 --- core/dr-gpu/src/readback.rs | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/core/dr-gpu/src/readback.rs b/core/dr-gpu/src/readback.rs index af40d17..892b704 100644 --- a/core/dr-gpu/src/readback.rs +++ b/core/dr-gpu/src/readback.rs @@ -15,7 +15,28 @@ use crate::{GpuContext, GpuError}; /// surfacing the error. Set far above any plausible completion — the copies /// this waits on are milliseconds — so it is reached only when something is /// wrong. -const POLL_LIMIT: u32 = 100_000; +/// How long a readback may take before it is called failed. +/// +/// **A deadline, not an iteration count, and the difference was a real bug.** +/// This was 100,000 non-blocking polls, which sounds generous and is not: a +/// `Poll` that finds nothing returns immediately, so the loop burned through +/// the whole budget in a few milliseconds. Small transfers — the histogram's +/// 4 KB, a viewport-sized frame — happened to complete inside it. A +/// full-resolution export did not: 5472×3648 is 80 MB, and every attempt +/// failed with "readback did not complete" while the copy was still perfectly +/// healthy. +/// +/// Generous, because the legitimate worst case is a large export on a slow +/// integrated GPU, and the only thing this bound exists to catch is a lost +/// device that will never deliver the callback at all. +const READBACK_DEADLINE: std::time::Duration = std::time::Duration::from_secs(30); + +/// How long to wait between polls once the first few have found nothing. +/// +/// Without it this is a busy spin that saturates a core for the duration of +/// the copy. A millisecond is far below the transfer times involved and keeps +/// the thread available to the scheduler. +const POLL_PAUSE: std::time::Duration = std::time::Duration::from_millis(1); /// Drive the device until a `map_async` callback lands. /// @@ -34,7 +55,9 @@ pub(crate) fn await_mapping( rx: &std::sync::mpsc::Receiver>, ) -> Result<(), GpuError> { let mut mapped = None; - for _ in 0..POLL_LIMIT { + let deadline = std::time::Instant::now() + READBACK_DEADLINE; + let mut spins = 0u32; + while std::time::Instant::now() < deadline { // A poll error is a lost device, which is exactly the case the bounded // spin exists to escape — returning here reports it immediately rather // than spinning out the full limit first. @@ -46,11 +69,26 @@ pub(crate) fn await_mapping( mapped = Some(r); break; } - Err(std::sync::mpsc::TryRecvError::Empty) => continue, + Err(std::sync::mpsc::TryRecvError::Empty) => { + // The first handful of polls run flat out, so a small + // transfer — the histogram's, most of all — still returns + // without ever sleeping. Only a copy that is genuinely going + // to take a while pays the pause. + spins += 1; + if spins > 64 { + std::thread::sleep(POLL_PAUSE); + } + continue; + } Err(e) => return Err(GpuError::Readback(e.to_string())), } } mapped - .ok_or_else(|| GpuError::Readback("readback did not complete".into()))? + .ok_or_else(|| { + GpuError::Readback(format!( + "readback did not complete within {}s", + READBACK_DEADLINE.as_secs() + )) + })? .map_err(|e| GpuError::Readback(e.to_string())) }