From c283a3ea601bc86709da829153da8db331c9201b Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Tue, 29 Sep 2026 17:00:00 +0300 Subject: [PATCH] fix: nescapture overholding locks, nescope pacing and mistakes --- Cargo.lock | 128 ++++++++++-------------- apps/nescapture/src/capture.rs | 173 ++++++++++++++++++++------------- apps/nescapture/src/pacing.rs | 53 ++++++++++ apps/nescapture/src/present.rs | 3 +- apps/nescope/src/handlers.rs | 46 ++++----- apps/nescope/src/main.rs | 54 +++------- apps/nescope/src/state.rs | 106 +++++++------------- 7 files changed, 289 insertions(+), 274 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e0107ec0..005a6e24 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -360,9 +360,9 @@ dependencies = [ [[package]] name = "cc" -version = "1.4.7" +version = "1.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "54413ede23c2daf518f35156dfde027feb2374004d63bd497f983c8db9c0e313" +checksum = "f360145194ee8e21db5ee7f3fcd4fe52210864c75c985dae33218202c8bbe040" dependencies = [ "find-msvc-tools", "shlex 2.0.1", @@ -520,7 +520,7 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0fa961b519f0b462e3a3b4a34b64d119eeaca1d59af726fe450bbba07a9fc0a1" dependencies = [ - "thiserror 2.0.20", + "thiserror 2.0.21", ] [[package]] @@ -790,7 +790,7 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "10d60334b3b2e7c9d91ef8150abfb6fa4c1c39ebbcf4a81c2e346aad939fee3e" dependencies = [ - "thiserror 2.0.20", + "thiserror 2.0.21", ] [[package]] @@ -986,13 +986,12 @@ checksum = "edd0f118536f44f5ccd48bcb8b111bdc3de888b58c74639dfb034a357d0f206d" [[package]] name = "encoding_rs" -version = "0.8.41" +version = "0.8.42" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7b5ef0006ac9ab233c38522f5ae99cae3625151de8f706cacee1cba4b8e2832a" +checksum = "8e985e0451871ad22fb8d2b6b076e2028a502a0d3950998c2c5c0a4f9b5d9679" dependencies = [ "cfg-if", "core_detect", - "multiversion", "multiversion_no_op", "rustversion", "scopeguard", @@ -1063,9 +1062,9 @@ checksum = "64cd1e32ddd350061ae6edb1b082d7c54915b5c672c389143b9a63403a109f24" [[package]] name = "find-msvc-tools" -version = "0.1.13" +version = "0.1.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ef25905e51abafe4dcea6c15fec58c57b601cdbd0ee53d22ea1d3016c587d39b" +checksum = "aedcfb3409746eddb02b9e19ebda1c3394f759a152e48ee875a0844d1b955484" [[package]] name = "flate2" @@ -1460,16 +1459,17 @@ dependencies = [ [[package]] name = "hyper-util" -version = "0.1.20" +version = "0.1.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "96547c2556ec9d12fb1578c4eaf448b04993e7fb79cbaad930a656880a6bdfa0" +checksum = "ddc03d96684f9226b8a787cdb71488417b53ab5ea8fdb1dac946cb9431cc8bff" dependencies = [ - "base64 0.22.1", + "base64 0.23.1", "bytes", "futures-channel", "futures-util", "http", "http-body", + "httparse", "hyper", "ipnet", "libc", @@ -1797,9 +1797,9 @@ dependencies = [ [[package]] name = "iroh-metrics" -version = "1.0.1" +version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "291065721ad7c477b972e581bbc528df031dc8eb5e39fe1ff3300ae5dfb157ef" +checksum = "8ede55536349842337f7cdc63012ec33dc637945f3259c98a0763aac364cdf09" dependencies = [ "iroh-metrics-derive", "itoa", @@ -1952,7 +1952,7 @@ dependencies = [ "jni-sys 0.4.1", "log", "simd_cesu8", - "thiserror 2.0.20", + "thiserror 2.0.21", "walkdir", "windows-link", ] @@ -2000,9 +2000,9 @@ dependencies = [ [[package]] name = "js-sys" -version = "0.3.105" +version = "0.3.106" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce57d20d1ea864ce2ac172ab472d409214f4fd359f0b2a2775abdf522e2af99e" +checksum = "7883d941dae510fb2d978fc3fe018c71c9e2892fd38854de3e8b92c2e5ad9cc5" dependencies = [ "cfg-if", "futures-util", @@ -2201,27 +2201,6 @@ dependencies = [ "windows-sys 0.61.2", ] -[[package]] -name = "multiversion" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4ca4bea16ffc3f443cf7d866912118196bfef4c6a1556ca00f9f9b00bb43f7c" -dependencies = [ - "multiversion-macros", -] - -[[package]] -name = "multiversion-macros" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0d416831a7317ef4b08bee00b69cbbb9c8763da7959a7026244d6266869f9c83" -dependencies = [ - "proc-macro2", - "quote", - "rustversion", - "syn 3.0.6", -] - [[package]] name = "multiversion_no_op" version = "1.0.0" @@ -2532,7 +2511,7 @@ dependencies = [ "log", "netlink-packet-core 0.8.2", "netlink-sys 0.8.8", - "thiserror 2.0.20", + "thiserror 2.0.21", ] [[package]] @@ -2644,7 +2623,7 @@ dependencies = [ "rustc-hash", "rustls", "socket2", - "thiserror 2.0.20", + "thiserror 2.0.21", "tokio", "tokio-stream", "tracing", @@ -2672,7 +2651,7 @@ dependencies = [ "rustls-pki-types", "slab", "sorted-index-buffer", - "thiserror 2.0.20", + "thiserror 2.0.21", "tinyvec", "tracing", "web-time", @@ -2989,7 +2968,7 @@ dependencies = [ "ash", "futures-channel", "futures-core", - "thiserror 2.0.20", + "thiserror 2.0.21", "tracing", ] @@ -3449,9 +3428,9 @@ dependencies = [ [[package]] name = "rustls-platform-verifier" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "26d1e2536ce4f35f4846aa13bff16bd0ff40157cdb14cc056c7b14ba41233ba0" +checksum = "1167586491e2b18b8bfbb293e8180ec17c201c4f076d7cb3070ca964e7598f98" dependencies = [ "core-foundation", "core-foundation-sys", @@ -3470,9 +3449,9 @@ dependencies = [ [[package]] name = "rustls-platform-verifier-android" -version = "0.1.1" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f87165f0995f63a9fbeea62b64d10b4d9d8e78ec6d7d51fb2125fda7bb36788f" +checksum = "eec689c0bc40ff2458a5977b6619cb718087084a18e02a131c599b62d05e1a5f" [[package]] name = "rustls-webpki" @@ -3756,9 +3735,9 @@ checksum = "0c790de23124f9ab44544d7ac05d60440adc586479ce501c1d6d7da3cd8c9cf5" [[package]] name = "smallvec" -version = "1.16.1" +version = "1.16.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ba467056f1b547ed52077911161fc86985becbc60e8e1857c8a144dab0def891" +checksum = "f9395f0f0eee849a9b707b2f06bb92a6a422090e2123bb2ef8e87a0e61892a8e" [[package]] name = "smithay" @@ -3788,7 +3767,7 @@ dependencies = [ "sha2 0.10.9", "smallvec", "tempfile", - "thiserror 2.0.20", + "thiserror 2.0.21", "tracing", "wayland-protocols", "wayland-protocols-misc", @@ -3986,11 +3965,11 @@ dependencies = [ [[package]] name = "thiserror" -version = "2.0.20" +version = "2.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec86235f5fcc2a73650310756d2ac5b138a5780bbbdfae3eeccec992c435ba4f" +checksum = "09e52cb86a36cede5cb101bf8908837b3e4c6e5e59fe7fd85c23fb56200d189e" dependencies = [ - "thiserror-impl 2.0.20", + "thiserror-impl 2.0.21", ] [[package]] @@ -4006,9 +3985,9 @@ dependencies = [ [[package]] name = "thiserror-impl" -version = "2.0.20" +version = "2.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc04cd3e1236dd4a98afca4569f2deb3f120e5422a4023be2cb683f8486292af" +checksum = "fe5197923287db20a58125f0bc85c062f7f2c892de97b18c356f9efb14b28524" dependencies = [ "proc-macro2", "quote", @@ -4101,9 +4080,9 @@ dependencies = [ [[package]] name = "tokio-rustls" -version = "0.26.5" +version = "0.26.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b0c85f2c3ef0b1cd58b36682f4b17aaa995f0e5db534d85692b4903abce21f67" +checksum = "c9cc2678c2cdd569ef8215e2afd7954ada2ae20b4fdd2c5fe6139a3b02d105db" dependencies = [ "rustls", "tokio", @@ -4570,9 +4549,9 @@ dependencies = [ [[package]] name = "wasm-bindgen" -version = "0.2.128" +version = "0.2.129" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aecb87a33d3b0c5e3b7aa46336eaf486cffafbd281b195e4c8b80d50df2351bf" +checksum = "9bb54f33acc68fd454578d9820b0bde1a1a3d17aa17bb7b6595806d02886d409" dependencies = [ "cfg-if", "once_cell", @@ -4583,19 +4562,20 @@ dependencies = [ [[package]] name = "wasm-bindgen-futures" -version = "0.4.78" +version = "0.4.79" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6ef4c5d3d2cdf5c54f4231181768f5510842e350db025faf1f7163b1030ed928" +checksum = "3cbab34de2d982e9b48e18d216d04c4a6f641066ff19ffb699980f591ee3610e" dependencies = [ "js-sys", + "tokio", "wasm-bindgen", ] [[package]] name = "wasm-bindgen-macro" -version = "0.2.128" +version = "0.2.129" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a690d511e3c1a8b3a55e33511e3c2c00c78415cd23650f32b808627f5696b9ed" +checksum = "2e29d0c35b16e224a7eeb5cd2d25e3e1968fbd65604117b44d3b789d00ee8535" dependencies = [ "quote", "wasm-bindgen-macro-support", @@ -4603,9 +4583,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-macro-support" -version = "0.2.128" +version = "0.2.129" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "411e4887f0071ef2d2164a9d5fdf2d20efbef78fccd3a78b0c10a1dc5295e48a" +checksum = "6f501a8bc3719dba86ef8ae4728879c08001bea749eb1333ac5b91e040e2a6b7" dependencies = [ "bumpalo", "proc-macro2", @@ -4616,9 +4596,9 @@ dependencies = [ [[package]] name = "wasm-bindgen-shared" -version = "0.2.128" +version = "0.2.129" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "81941cd78d0c92026c33e5e01312845a4cb1e9af3407f9134b100dd03144103e" +checksum = "23f0c9c52aa7cd7d77769a4cfe2a9adb1b331f489a41d912ce14513d5ab995c6" dependencies = [ "unicode-ident", ] @@ -4735,9 +4715,9 @@ dependencies = [ [[package]] name = "web-sys" -version = "0.3.105" +version = "0.3.106" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9fbddc4a036f00ec4f18c83445bd3115cb306a91da554919a099d9222fe4a7f8" +checksum = "88261b9deccee56594c11a3460c462c41f58d148598fe70ad77070126a68aba4" dependencies = [ "js-sys", "wasm-bindgen", @@ -5186,7 +5166,7 @@ dependencies = [ "futures", "log", "serde", - "thiserror 2.0.20", + "thiserror 2.0.21", "windows", "windows-core", ] @@ -5210,7 +5190,7 @@ dependencies = [ "pharos", "rustc_version", "send_wrapper", - "thiserror 2.0.20", + "thiserror 2.0.21", "wasm-bindgen", "wasm-bindgen-futures", "web-sys", @@ -5290,18 +5270,18 @@ dependencies = [ [[package]] name = "zerocopy" -version = "0.8.57" +version = "0.8.59" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d35102a9f36d089ccae9e4c6802bc118be4487b80aaffc0ab4e0cf5ce92d2873" +checksum = "6df92bf3d9227be3d53173901ddbffac2babc27ae50f397776ffd6dc33f800cb" dependencies = [ "zerocopy-derive", ] [[package]] name = "zerocopy-derive" -version = "0.8.57" +version = "0.8.59" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "146c01f5ab44258da43cf276c74a2763db2ff3969c9c652c3f2de07041d0b2bc" +checksum = "ac4f328cf2f05d084e496c3e9c3f33ed0a183656a16e1fcec4d464d8373aec82" dependencies = [ "proc-macro2", "quote", diff --git a/apps/nescapture/src/capture.rs b/apps/nescapture/src/capture.rs index 1a30aa82..84e64f4e 100644 --- a/apps/nescapture/src/capture.rs +++ b/apps/nescapture/src/capture.rs @@ -821,17 +821,30 @@ pub unsafe fn blit_gpu_time_ns( slot: usize, wait: bool, ) -> Option { - let ring_guard = ds.capture_ring.lock().ok()?; - let ring = ring_guard.as_ref()?; - if ring.timestamp_pool.is_null() { + // `vkGetQueryPoolResults` can synchronize with the GPU on some drivers. + // Copy what is needed out and release the lock before calling it: holding + // `capture_ring` across the driver call stalls the game's present thread + // on the mutex, which is where a regular per-frame hitch would come from. + // + // Also gated on trace: the value feeds only the trace line in + // `stats_sender_thread`, so with trace off this whole call is dead work. + if !log::log_enabled!(log::Level::Trace) { return None; } + let (pool, period) = { + let ring_guard = ds.capture_ring.lock().ok()?; + let ring = ring_guard.as_ref()?; + if ring.timestamp_pool.is_null() { + return None; + } + (ring.timestamp_pool, ring.timestamp_period) + }; let get = ds.fp.get_query_pool_results?; let mut ticks = [0u64; 2]; let result = unsafe { get( ds.raw, - ring.timestamp_pool, + pool, (slot * 2) as u32, 2, std::mem::size_of_val(&ticks), @@ -848,7 +861,7 @@ pub unsafe fn blit_gpu_time_ns( return None; } let elapsed = ticks[1].checked_sub(ticks[0])?; - Some((elapsed as f64 * f64::from(ring.timestamp_period)) as u64) + Some((elapsed as f64 * f64::from(period)) as u64) } /// Record the blit from one swapchain image into one ring slot. @@ -1057,37 +1070,84 @@ pub unsafe fn capture_present_frame( // vkGetDeviceQueue, so there is nothing safe to assume about it. let queue_family = *crate::state::QUEUE_TO_FAMILY.get(&queue.as_raw())?; - let mut ring_guard = ds.capture_ring.lock().ok()?; - if !unsafe { - ensure_capture_ring( - ds, - &mut ring_guard, - ext.width, - ext.height, - fmt, - queue_family, - image_count, + // ── Everything that touches the ring, then the lock is dropped. ─────── + // + // `vkQueueSubmit` below is deliberately outside this scope. On a queue + // the encoder's conversion also submits to, submission serialization is + // a driver-internal lock and it can block for a frame's worth of time. + // Holding `capture_ring` across it stalls the encoder thread, and worse, + // any driver call the encoder makes under that same lock stalls the + // game's present thread on the mutex. Keep the critical section to + // plain memory. + // + // The slot guard lives past the block. A ring rebuild needs + // `capture_slots.all_free()`, so holding it keeps the ring stable for + // as long as the handles copied out are in use. + let (guard, fence, fi, slot_memory, pool, cb, present_wait, blit_timeline, blit_point) = { + let mut ring_guard = ds.capture_ring.lock().ok()?; + if !unsafe { + ensure_capture_ring( + ds, + &mut ring_guard, + ext.width, + ext.height, + fmt, + queue_family, + image_count, + ) + } { + return None; + } + let ring = ring_guard.as_mut()?; + + let present_wait = unsafe { ensure_present_semaphore(ds, ring, image_index) }?; + let guard = ds.capture_slots.try_acquire()?; + let slot_index = guard.index(); + let slot = ring.slots.get(slot_index)?; + let fence = slot.fence; + let fi = slot.image; + let slot_memory = slot.memory; + let pool = ring.command_pool; + + // The recording depends on the source image, the destination image + // and the extent. A swapchain recreated at the same extent keeps the + // ring but replaces the source images, so invalidate here rather than + // trusting every caller to have noticed. + if ring.blit_extent != ext { + ring.blits_recorded.iter_mut().for_each(|r| *r = false); + ring.blit_extent = ext; + } + let blit = crate::state::blit_index(image_index, slot_index, ring.image_count)?; + let cb = *ring.blits.get(blit)?; + if !ring.blits_recorded[blit] { + let ts_pool = ring.timestamp_pool; + if !unsafe { record_blit(ds, cb, si, fi, ext, ts_pool, slot_index) } { + return None; + } + ring.blits_recorded[blit] = true; + } + + let blit_timeline = ring.blit_timeline; + let blit_point = (!blit_timeline.is_null()) + .then(|| pixelforge::TimelinePoint::new(blit_timeline, ring.blit_value + 1)); + + ( + guard, + fence, + fi, + slot_memory, + pool, + cb, + present_wait, + blit_timeline, + blit_point, ) - } { - return None; - } - let ring = ring_guard.as_mut()?; + }; + // ── capture_ring lock released ──────────────────────────────────────── - let present_wait = unsafe { ensure_present_semaphore(ds, ring, image_index) }?; - - // Never blocks: a frame with no free slot is one the encoder has not caught - // up with, and stalling the game's present to wait for it would be worse - // than skipping it. - let guard = ds.capture_slots.try_acquire()?; - let slot_index = guard.index(); - let slot = ring.slots.get(slot_index)?; - let fence = slot.fence; - let fi = slot.image; - let slot_memory = slot.memory; - - // A free slot's fence is already signalled — the encoder side waits on it - // before it ever reads the slot. This covers the paths that abandon a frame - // and return the slot without that wait. + // A free slot's fence is already signalled -- the encoder side waits on + // it before it ever reads the slot. Bounded at 2 ms; the timeout guards + // the abandoned-frame paths, not steady state. unsafe { if (ds.fp.wait_for_fences)(ds.raw, 1, &fence, vk::TRUE, 2_000_000) != vk::Result::SUCCESS { return None; @@ -1095,32 +1155,9 @@ pub unsafe fn capture_present_frame( let _ = (ds.fp.reset_fences)(ds.raw, 1, &fence); } - // The recording depends on the source image, the destination image and the - // extent. A swapchain recreated at the same extent keeps the ring but - // replaces the source images, so invalidate here rather than trusting every - // caller to have noticed. - if ring.blit_extent != ext { - ring.blits_recorded.iter_mut().for_each(|r| *r = false); - ring.blit_extent = ext; - } - let blit = crate::state::blit_index(image_index, slot_index, ring.image_count)?; - let cb = *ring.blits.get(blit)?; - if !ring.blits_recorded[blit] { - let pool = ring.timestamp_pool; - if !unsafe { record_blit(ds, cb, si, fi, ext, pool, slot_index) } { - return None; - } - ring.blits_recorded[blit] = true; - } - let wait_stages = vec![vk::PipelineStageFlags::TRANSFER; app_waits.len()]; - // On a shared device the blit also advances the ring's timeline. A - // timeline signal needs its value given alongside, and the binary - // semaphore next to it a placeholder the driver ignores. - let blit_point = (!ring.blit_timeline.is_null()) - .then(|| pixelforge::TimelinePoint::new(ring.blit_timeline, ring.blit_value + 1)); - let signals = [present_wait, ring.blit_timeline]; + let signals = [present_wait, blit_timeline]; let signal_values = [0, blit_point.map_or(0, |p| p.value)]; let timeline_info = vk::TimelineSemaphoreSubmitInfo { signal_semaphore_value_count: 2, @@ -1152,25 +1189,29 @@ pub unsafe fn capture_present_frame( _marker: std::marker::PhantomData, }; + // No ring lock. `cb` and `fi` are protected by the slot guard, which is + // held. The encoder thread may hold the lock for its own driver calls; + // this call does not touch it. unsafe { if (ds.fp.queue_submit)(queue, 1, &subi, fence) != vk::Result::SUCCESS { log::warn!("capture queue_submit failed — frame skipped"); - // The submit never happened, so nothing waited on the application's - // semaphores and nothing will signal ours. Re-signal the fence by - // hand so the slot is reusable, and let the caller present as the - // application intended. + // The submit never happened, so nothing waited on the + // application's semaphores and nothing will signal ours. let _ = (ds.fp.reset_fences)(ds.raw, 1, &fence); return None; } } - if let Some(p) = blit_point { + // Re-acquire briefly to publish the new timeline value. A store and + // nothing else. + if let Some(p) = blit_point + && let Ok(mut ring_guard) = ds.capture_ring.lock() + && let Some(ring) = ring_guard.as_mut() + { ring.blit_value = p.value; } - // Once per process, and only when asked for: reads this frame back and - // says what range its values are in. See `probe_float_range`. - let pool = ring.command_pool; + // Once per process, and only when asked for. See `probe_float_range`. unsafe { probe_float_range(ds, pool, queue, fi, slot_memory, fence, fmt, ext) }; Some(CaptureSubmission { diff --git a/apps/nescapture/src/pacing.rs b/apps/nescapture/src/pacing.rs index 62f963ec..930dc062 100644 --- a/apps/nescapture/src/pacing.rs +++ b/apps/nescapture/src/pacing.rs @@ -259,6 +259,59 @@ impl FramePacer { } } +/// Sleep until an absolute instant, on the monotonic clock. +/// +/// `std::thread::sleep` takes a *relative* duration; Linux implements it with +/// `nanosleep`, which rounds to a scheduler tick and then wakes the thread +/// whenever the scheduler next runs it. On a box with a game, an encoder, an +/// IPC thread and a compositor event loop all runnable, that "whenever" can +/// be 10-20 ms past the deadline — which reads downstream as a frame that +/// arrived late, once every second or so. +/// +/// `clock_nanosleep` with `TIMER_ABSTIME` gives the kernel the deadline +/// itself and lets it place the wake, which is the whole difference between +/// "sleep for 12 ms" and "wake at this instant." +/// +/// Returns early if the deadline has already passed. Cannot fail for a valid +/// monotonic deadline; errors are ignored rather than retried, because a +/// sleep that woke early once is not a problem the next iteration cannot +/// handle. +pub fn sleep_until(deadline: std::time::Instant) { + let now = std::time::Instant::now(); + if deadline <= now { + return; + } + let remaining = deadline - now; + + // Instant is CLOCK_MONOTONIC-based on Linux, so the deadline converts by + // adding to the current monotonic time. There is no public Instant → + // timespec, hence the round trip. + let mut ts = libc::timespec { + tv_sec: 0, + tv_nsec: 0, + }; + if unsafe { libc::clock_gettime(libc::CLOCK_MONOTONIC, &mut ts) } != 0 { + // Should not happen, but if it did, falling back to the relative + // sleep is strictly better than not sleeping at all. + std::thread::sleep(remaining); + return; + } + ts.tv_sec += remaining.as_secs() as libc::time_t; + ts.tv_nsec += remaining.subsec_nanos() as libc::c_long; + if ts.tv_nsec >= 1_000_000_000 { + ts.tv_sec += 1; + ts.tv_nsec -= 1_000_000_000; + } + unsafe { + libc::clock_nanosleep( + libc::CLOCK_MONOTONIC, + libc::TIMER_ABSTIME, + &ts, + std::ptr::null_mut(), + ); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/apps/nescapture/src/present.rs b/apps/nescapture/src/present.rs index 9366d8d7..c615d4e6 100644 --- a/apps/nescapture/src/present.rs +++ b/apps/nescapture/src/present.rs @@ -136,7 +136,8 @@ fn finish(ds: &crate::state::DeviceState, entered: std::time::Instant, down: std }; if !held.is_zero() { note_present(ds, PresentStep::Holding); - std::thread::sleep(held); + // Absolute deadline, not a duration + crate::pacing::sleep_until(worked + held); } let now = std::time::Instant::now(); diff --git a/apps/nescope/src/handlers.rs b/apps/nescope/src/handlers.rs index 135e8776..08415e58 100644 --- a/apps/nescope/src/handlers.rs +++ b/apps/nescope/src/handlers.rs @@ -7,6 +7,7 @@ //! - All delegate macros use smithay::backend::allocator::dmabuf::Dmabuf; +use smithay::backend::renderer::utils::on_commit_buffer_handler; use smithay::desktop::Window; use smithay::input::pointer::{CursorImageStatus, PointerHandle}; use smithay::input::{Seat, SeatHandler, SeatState}; @@ -78,44 +79,43 @@ impl CompositorHandler for NescopeState { } fn commit(&mut self, surface: &WlSurface) { + on_commit_buffer_handler::(surface); + self.hdr.commit(surface); if is_sync_subsurface(surface) { return; } - // Hold a reference to the committed buffer — delays wl_buffer.release - // until the next frame tick, starving the swapchain of images in FIFO mode. - smithay::wayland::compositor::with_states(surface, |states| { - let mut cached = states - .cached_state - .get::(); - let attrs = cached.current(); - if let Some(smithay::wayland::compositor::BufferAssignment::NewBuffer(ref wl_buf)) = - attrs.buffer - { - self.held_buffer = Some(wl_buf.clone()); + // A commit can land on a subsurface; walk up to the root of the + // surface tree to find the window it belongs to. Pacing the whole + // tree from the root keeps a client's sub-surfaces on the same + // frame as its main surface, which is what the client is asking + // for when it calls wl_surface.frame on any of them. + let root = { + let mut s = surface.clone(); + while let Some(parent) = smithay::wayland::compositor::get_parent(&s) { + s = parent; } - }); + s + }; - // Notify the window of the commit so it can refresh its cached state. - if let Some(window) = self + let Some(window) = self .space .elements() .find(|w| { w.toplevel() - .map(|t| t.wl_surface() == surface) + .map(|t| t.wl_surface() == &root) .unwrap_or(false) }) .cloned() - { - window.on_commit(); - // A game frame, which is what the stats claim to report. Counted - // here rather than on the frame-callback tick: that tick fires - // whether anything was drawn or not, so counting it reported the - // compositor's own cadence back as the game's rate. - self.game_frame_count += 1; - } + else { + return; + }; + + window.on_commit(); + self.game_frame_count += 1; + self.on_surface_committed(&root, &window); } fn destroyed(&mut self, surface: &WlSurface) { diff --git a/apps/nescope/src/main.rs b/apps/nescope/src/main.rs index 072a2500..0d1a4785 100644 --- a/apps/nescope/src/main.rs +++ b/apps/nescope/src/main.rs @@ -111,35 +111,9 @@ struct Args { height: u32, /// Virtual output refresh rate, as advertised to clients. - /// - /// **Advertised only — this does not pace anything.** It is what a game - /// reads as its monitor's refresh rate, so it should be the rate the - /// session actually sends at: a game with V-Sync on will lock to it, and - /// one that reads the mode to build a settings list will offer it. - /// - /// Pacing is `--frame-callback-hz`, and the two used to be this one value. - /// That made an honest advertisement and a non-binding cadence mutually - /// exclusive, which is why the default sat at 60 while sessions asked for - /// 120. #[arg(long, default_value = "60", env = "NESCOPE_FPS")] fps: u32, - /// How often `wl_surface.frame` callbacks are sent, in hertz. - /// - /// This is the only rate that can throttle a client, and only a FIFO one: - /// `IMMEDIATE` and `MAILBOX` swapchains ignore these callbacks entirely. - /// It is therefore not a frame limiter — it cannot hold a game whose - /// V-Sync is off, which is every game whose player turned it off. That job - /// belongs to the capture layer, which sees every present and can hold the - /// application whatever its swapchain does. - /// - /// So the default is set high enough never to bind, and the compositor - /// stops being a second opinion on the frame rate. The cost is the timer - /// itself: a wakeup per tick, each sending callbacks to the surfaces in - /// the space. Lower it if that shows up on a small box. - #[arg(long, default_value = "1000", env = "NESCOPE_FRAME_CALLBACK_HZ")] - frame_callback_hz: u32, - /// Enable HDR colour management (`wp_color_manager_v1`). #[arg( long, @@ -441,26 +415,26 @@ fn main() { } } - // ── Frame-callback timer ────────────────────────────────────────────── - // Sends wl_surface.frame done events, releases the held buffer and posts - // presentation feedback. + // ── Input dispatch timer ──────────────────────────────────────────────── + // Just input and cursor. High rate so a 1000 Hz mouse isn't coalesced; + // cursor piggybacks on the same tick because it is change-gated + // internally and costs nothing when idle. // - // Deliberately *not* `--fps`. This cadence only ever throttles a FIFO - // client, so using it as a frame limiter caps the games that opted into - // V-Sync and does nothing at all to the ones that did not — which is the - // wrong way round, and it capped them at 60 while sessions asked for 120. - // The capture layer holds the game instead, and this runs fast enough to - // stay out of the way. - let frame_interval = Duration::from_micros(1_000_000 / args.frame_callback_hz.max(1) as u64); + // There is deliberately no frame-callback timer. Frame callbacks and + // presentation feedback are driven by commits — see + // NescopeState::on_surface_committed. A timer at any rate would pace + // the client against something other than its own submit loop, which + // is what produces the periodic hitches. + let tick_interval = Duration::from_millis(1); loop_handle - .insert_source(Timer::from_duration(frame_interval), move |_, _, data| { + .insert_source(Timer::from_duration(tick_interval), move |_, _, data| { if let Some(ref mut li) = data.libinput { libinput_backend::dispatch_libinput(li, &mut data.state); } - data.state.on_frame_tick(); - calloop::timer::TimeoutAction::ToDuration(frame_interval) + data.state.send_cursor_update(); + calloop::timer::TimeoutAction::ToDuration(tick_interval) }) - .expect("Failed to register frame timer"); + .expect("Failed to register input timer"); // ── CalloopData ─────────────────────────────────────────────────────── let socket_name_for_cleanup = args.socket.clone(); diff --git a/apps/nescope/src/state.rs b/apps/nescope/src/state.rs index 3363b43a..9d9fa61e 100644 --- a/apps/nescope/src/state.rs +++ b/apps/nescope/src/state.rs @@ -733,86 +733,57 @@ impl NescopeState { } // ----------------------------------------------------------------------- - // Frame callbacks — driven by the frame-callback timer in main.rs + // Commit-driven frame signals + // + // Called from CompositorHandler::commit for every non-sync commit that + // belongs to a mapped window. Fires the wl_surface.frame callbacks the + // client asked for and resolves the wp_presentation_feedback for the + // frame that just arrived. + // + // NOT driven by a timer. A frame callback means "the compositor is ready + // for your next frame"; for a headless capture compositor that is true + // the moment the commit lands, because there is no renderer and no + // scan-out between us and the client. A timer at any rate decouples the + // client's submit loop from the signal it is pacing itself with — too + // early for a slow client (over-submits), too late for a fast one + // (throttles), and when the two rates are close but not equal the phase + // slips and produces periodic hitches. The client's own rate is the + // correct rate; that is the whole point of the callback. // ----------------------------------------------------------------------- - /// Called from the calloop timer at `--frame-callback-hz`. - /// - /// Note what this does *not* count: the game's frames. This runs whether or - /// not anything was drawn, so counting ticks here reported the timer's own - /// rate as the game's — true only while the two were the same number, which - /// they no longer are. `game_frame_count` is incremented on commit. - pub fn on_frame_tick(&mut self) { + pub(crate) fn on_surface_committed(&mut self, root: &WlSurface, window: &Window) { let output = self.output.clone(); let now = self.clock.now(); - // 1. Release the held buffer → frees a swapchain image for the game. - self.held_buffer.take(); + // Frame callbacks for the whole tree. `None` throttle: no minimum + // interval — the client decides its own cadence, we answer every + // frame it hands us. + send_frames_surface_tree(root, &output, now, None, |_, _| Some(output.clone())); - // 2. Presentation feedback — every frame committed since the last tick - // is reported presented, on the one output there is. - // - // Not filtered by primary scan-out output: that is recorded by a - // renderer, and nothing here renders, so the filter matched no - // surface ever. Feedback then resolved only as `discarded`, when the - // next commit superseded it -- which never happens for a client that - // waits for its last present before drawing the next. A Vulkan - // client with present-wait under FIFO does exactly that: Control on - // VKD3D-Proton froze on leaving its title screen, GPU idle, the game - // still running behind a stream that no longer moved. - let mut output_presentation_feedback = OutputPresentationFeedback::new(&output); - let on_output = - |_: &WlSurface, _: &smithay::wayland::compositor::SurfaceData| Some(output.clone()); - for window in self.space.elements().cloned().collect::>() { - window.take_presentation_feedback( - &mut output_presentation_feedback, - on_output, - |_, _| wp_presentation_feedback::Kind::Vsync, - ); - } - output_presentation_feedback.presented( + // Presentation feedback for the same frame, resolved as `presented` + // immediately. `discarded` is the wrong resolution here: it means + // "the next frame superseded this one before it reached the + // display," which is not what happened — this frame is the one the + // capture layer will see, and the client that waits on it (Vulkan + // FIFO present, VKD3D-Proton, DXVK) is waiting precisely for this. + let mut feedback = OutputPresentationFeedback::new(&output); + window.take_presentation_feedback( + &mut feedback, + |_, _| Some(output.clone()), + |_, _| wp_presentation_feedback::Kind::Vsync, + ); + feedback.presented( now, output .current_mode() - .map(|mode| Refresh::fixed(Duration::from_secs_f64(1_000f64 / mode.refresh as f64))) + .map(|m| Refresh::fixed(Duration::from_secs_f64(1_000.0 / m.refresh as f64))) .unwrap_or(Refresh::Unknown), 0, wp_presentation_feedback::Kind::Vsync, ); - - // 3. Frame callbacks — tell the game it can present the next frame. - for window in self.space.elements().cloned().collect::>() { - if let Some(surface) = window.wl_surface() { - send_frames_surface_tree(&*surface, &output, now, Some(Duration::ZERO), |_, _| { - Some(output.clone()) - }); - } - } - - // 4. Send periodic stats over IPC - let now = std::time::Instant::now(); - if now.duration_since(self.last_stats_time) >= std::time::Duration::from_secs(1) { - let fps = self.game_frame_count.min(255) as u8; - let count = self.game_frame_count as u32; - self.game_frame_count = 0; - self.last_stats_time = now; - - if let Some(ref mut ipc) = self.ipc_write { - use std::io::Write; - let mut buf = Vec::with_capacity(6); - nesprotocol::stats::encode_nescope_stats(&mut buf, fps, count); - let len = buf.len() as u16; - let _ = ipc.write_all(&len.to_le_bytes()); - let _ = ipc.write_all(&buf); - let _ = ipc.flush(); - } - } - - // 5. Send cursor position update over IPC (if changed). - self.send_cursor_update(); } - fn send_cursor_update(&mut self) { + pub(crate) fn send_cursor_update(&mut self) { use std::io::Write; let ipc = match self.ipc_write.as_mut() { Some(s) => s, @@ -879,11 +850,6 @@ impl NescopeState { let _ = ipc.write_all(&buf); let _ = ipc.flush(); - tracing::debug!( - "cursor: sent update pos=({:.0},{:.0}) status={status}", - x, - y - ); self.last_sent_cursor_pos = self.cursor_position; self.last_sent_cursor_status = status; }