diff --git a/Cargo.lock b/Cargo.lock index f654272..19b8f22 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1757,6 +1757,7 @@ dependencies = [ "swash", "wasm-bindgen", "web-sys", + "wgpu", ] [[package]] @@ -2019,28 +2020,6 @@ dependencies = [ "slab", ] -[[package]] -name = "gbm" -version = "0.18.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce852e998d3ca5e4a97014fb31c940dc5ef344ec7d364984525fd11e8a547e6a" -dependencies = [ - "bitflags 2.13.1", - "drm", - "drm-fourcc", - "gbm-sys", - "libc", -] - -[[package]] -name = "gbm-sys" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c13a5f2acc785d8fb6bf6b7ab6bfb0ef5dad4f4d97e8e70bb8e470722312f76f" -dependencies = [ - "libc", -] - [[package]] name = "generic-array" version = "0.14.7" @@ -2500,14 +2479,13 @@ dependencies = [ "calloop 0.14.4", "cfg_aliases", "drm", - "gbm", - "glutin", "i-slint-common", "i-slint-core", "i-slint-renderer-femtovg", + "i-slint-renderer-skia", "input", + "memmap2", "nix", - "raw-window-handle", "xkbcommon", ] @@ -2526,6 +2504,7 @@ dependencies = [ "i-slint-core", "i-slint-core-macros", "i-slint-renderer-femtovg", + "i-slint-renderer-skia", ] [[package]] @@ -2667,6 +2646,7 @@ dependencies = [ "wasm-bindgen", "web-sys", "web-time", + "wgpu", "windows", ] @@ -2701,6 +2681,7 @@ dependencies = [ "rgb", "wasm-bindgen", "web-sys", + "wgpu", ] [[package]] @@ -2709,6 +2690,7 @@ version = "1.17.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7b6eed7f3f0a9a3d3ca6e8b9d4ca233371d989351fdb2a7ab88ec368b99e7b57" dependencies = [ + "ash", "bytemuck", "cfg-if", "cfg_aliases", @@ -2734,9 +2716,12 @@ dependencies = [ "scoped-tls-hkt", "skia-safe", "softbuffer", + "spin_on", "unicode-segmentation", "vtable", + "wgpu", "windows", + "windows-core", "write-fonts", ] @@ -5808,6 +5793,7 @@ dependencies = [ "slint-macros", "unicode-segmentation", "vtable", + "wgpu", ] [[package]] @@ -5971,6 +5957,7 @@ checksum = "aac18da81ebbf05109ab275b157c22a653bb3c12cf884450179942f81bcbf6c3" dependencies = [ "as-raw-xcb-connection", "bytemuck", + "drm", "fastrand", "js-sys", "memmap2", diff --git a/apps/darkroom-android/Cargo.toml b/apps/darkroom-android/Cargo.toml index 28e7d7c..fc6c416 100644 --- a/apps/darkroom-android/Cargo.toml +++ b/apps/darkroom-android/Cargo.toml @@ -27,8 +27,8 @@ log.workspace = true android_logger = "0.15" [features] -# Mirrors darkroom-desktop: the CPU readback path stays on until spike S1 -# lands zero-copy. On Adreno this is the same wrong path as on desktop, only -# with less memory bandwidth to absorb it (ARCH §6.1). -default = ["readback"] -readback = ["dr-ui/readback"] +# Mirrors darkroom-desktop: the CPU readback path is gone since S1 landed +# zero-copy. It mattered more here than on desktop — the same wrong path with +# far less memory bandwidth to absorb it (ARCH §6.1) — but it is untested on a +# device, since S1 was verified on desktop only. +default = [] diff --git a/apps/darkroom-desktop/Cargo.toml b/apps/darkroom-desktop/Cargo.toml index a6c2cec..238b7f6 100644 --- a/apps/darkroom-desktop/Cargo.toml +++ b/apps/darkroom-desktop/Cargo.toml @@ -12,6 +12,4 @@ env_logger.workspace = true log.workspace = true [features] -default = ["readback"] -# Temporary CPU upload path until spike S1 lands zero-copy adoption. -readback = ["dr-ui/readback"] +default = [] diff --git a/core/dr-gpu/Cargo.toml b/core/dr-gpu/Cargo.toml index 7cd6618..86be0e3 100644 --- a/core/dr-gpu/Cargo.toml +++ b/core/dr-gpu/Cargo.toml @@ -26,7 +26,9 @@ required-features = ["readback"] [[example]] name = "develop" -required-features = ["readback"] +# No `readback` needed since S1: this writes a file, so it goes through +# `export_pixels`, which is ungated precisely because an export is not the +# round-trip AC-8 forbids. [features] default = [] diff --git a/core/dr-gpu/examples/develop.rs b/core/dr-gpu/examples/develop.rs index 2f65762..cb38e9d 100644 --- a/core/dr-gpu/examples/develop.rs +++ b/core/dr-gpu/examples/develop.rs @@ -5,7 +5,7 @@ //! isolation; this proves they compose into an image a person would accept. //! //! ```sh -//! cargo run -p dr-gpu --example develop --features readback -- IMG.CR2 out.ppm +//! cargo run -p dr-gpu --example develop -- IMG.CR2 out.ppm //! ``` //! //! PPM because it needs no encoder dependency and every image viewer reads @@ -145,7 +145,10 @@ fn main() { adjust.cached_pipelines() ); - let (pixels, pw, ph) = adjust.read_output().expect("readback"); + // `export_pixels`, because that is honestly what this is: the frame is + // going into a PPM, not onto a screen. See the note on that method for + // why the two readbacks were never the same thing (AC-8). + let (pixels, pw, ph) = adjust.export_pixels().expect("readback"); // Sanity: an all-black or all-white result means something upstream // failed silently, and it is far easier to see here than in a viewer. diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index 39c3c22..062b9a1 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -38,9 +38,9 @@ const RESERVED_FIELDS: usize = dr_pipeline::RESERVED_UNIFORM_FIELDS; /// surfacing the error. Set far above any plausible completion — the copy this /// waits on is milliseconds — so it is reached only when something is wrong. /// -/// Ungated along with `export_pixels`: an export reads pixels back in a -/// shipping build, and the bound that stops a lost device hanging the app -/// applies at least as much there as it does to the display bridge. +/// Ungated along with [`AdjustPass::export_pixels`], the one readback that +/// survives S1: an export reads pixels back in a shipping build, and a lost +/// device mid-export must surface as an error rather than a hung interface. const READBACK_POLL_LIMIT: u32 = 100_000; /// Runs composed operation chains against demosaiced images. @@ -50,8 +50,27 @@ pub struct AdjustPass { pipeline_layout: wgpu::PipelineLayout, /// Compiled pipelines by structure hash (ARCH §5.6). cache: HashMap, - /// Output texture, reallocated only when the size changes. - target: Option, + /// TRACES: FR-DSP-1 | AC-8 + /// Output textures, written alternately, each reallocated only when the + /// size changes. + /// + /// **Two, and the second one is not an optimisation — it is what makes the + /// zero-copy path visible.** Since S1 the compositor is handed this + /// texture rather than a copy of its pixels, and Slint decides whether to + /// repaint by comparing the image property against its previous value. Two + /// images wrapping the *same* `wgpu::Texture` compare equal, so a pass + /// that always wrote one texture would recompute every frame on the GPU + /// and never once be asked to show it. Alternating makes each frame a + /// genuinely different value, which is the only thing that makes it a + /// different picture as far as the property system is concerned. + /// + /// It also settles the question of whether the compositor is still + /// sampling last frame while this frame's dispatch overwrites it. Both go + /// through one queue, so submission order already answers that — but not + /// having to rely on it is worth a texture. + targets: [Option; 2], + /// Which of [`Self::targets`] the last render wrote. + current: usize, } struct Target { @@ -117,7 +136,8 @@ impl AdjustPass { bind_group_layout, pipeline_layout, cache: HashMap::new(), - target: None, + targets: [None, None], + current: 0, } } @@ -176,13 +196,20 @@ impl AdjustPass { .expect("just inserted")) } - /// Ensure the output texture matches the requested size. + /// Move to the other output texture and make sure it is the right size. + /// + /// The rotation is unconditional; the reallocation is not. Steady-state + /// rendering at one viewport size therefore allocates nothing and simply + /// ping-pongs between two textures — see [`Self::targets`] for why there + /// are two. A resize reallocates whichever one comes up next, so the two + /// converge on the new size over two frames rather than in one lump. fn ensure_target(&mut self, width: u32, height: u32) { - let matches = self - .target + self.current ^= 1; + let slot = &mut self.targets[self.current]; + if slot .as_ref() - .is_some_and(|t| t.width == width && t.height == height); - if matches { + .is_some_and(|t| t.width == width && t.height == height) + { return; } @@ -197,13 +224,24 @@ impl AdjustPass { sample_count: 1, dimension: wgpu::TextureDimension::D2, format: Self::FORMAT, + // STORAGE_BINDING to write from compute, TEXTURE_BINDING so the + // compositor can sample it, COPY_SRC for `export_pixels`. + // + // RENDER_ATTACHMENT is never used by this pass and is required + // anyway: Slint rejects an imported texture that lacks it + // (`TextureImportError::InvalidUsage`), because a compositor + // handed a texture has to assume it may need to draw into it. The + // format is likewise not a free choice — `Rgba8Unorm` and + // `Rgba8UnormSrgb` are the only two the import accepts, which is + // why `FORMAT` is what it is. usage: wgpu::TextureUsages::STORAGE_BINDING | wgpu::TextureUsages::TEXTURE_BINDING + | wgpu::TextureUsages::RENDER_ATTACHMENT | wgpu::TextureUsages::COPY_SRC, view_formats: &[], }); let view = texture.create_view(&Default::default()); - self.target = Some(Target { + self.targets[self.current] = Some(Target { texture, view, width, @@ -262,7 +300,7 @@ impl AdjustPass { .cache .get(&shader.structure_hash) .expect("compiled above"); - let target = self.target.as_ref().expect("ensured above"); + let target = self.targets[self.current].as_ref().expect("ensured above"); let bind_group = self .ctx @@ -303,7 +341,10 @@ impl AdjustPass { } self.ctx.queue.submit(Some(enc.finish())); - Ok(&self.target.as_ref().expect("ensured above").texture) + Ok(&self.targets[self.current] + .as_ref() + .expect("ensured above") + .texture) } /// How many distinct pipelines are compiled. Exposed for tests asserting @@ -312,48 +353,37 @@ impl AdjustPass { self.cache.len() } + /// The texture the last render wrote, if there has been one. pub fn output(&self) -> Option<&wgpu::Texture> { - self.target.as_ref().map(|t| &t.texture) + self.targets[self.current].as_ref().map(|t| &t.texture) } - /// Copy the output to the CPU as tightly packed RGBA8. - /// - /// **A temporary bridge, not the display path.** ARCH §6.1 forbids this - /// round-trip in production and AC-8 asserts it does not happen; it - /// exists only because Slint's texture-import path is unwired until - /// spike S1. Measured cost at 4K is ~7 ms against a 0.28 ms compute pass - /// — 96% of the frame — so this must go, and the `readback` feature gate - /// keeps it out of a shipping build. - #[cfg(any(test, feature = "readback"))] - pub fn read_output(&self) -> Result<(Vec, u32, u32), GpuError> { - self.copy_output() - } - - /// TRACES: FR-EXP-9 + /// TRACES: FR-EXP-9 | AC-8 /// Copy the output to the CPU **for export**. /// - /// The same transfer as [`Self::read_output`] and deliberately not the - /// same method, because the two are opposites in intent and only one of - /// them is a defect. + /// This method had a twin, `read_output`, which performed exactly the same + /// transfer for the display path. Spike S1 deleted the twin and left this + /// one, and the difference between them is worth writing down because it + /// is the whole of AC-8. /// - /// Reading pixels back to display them is what ARCH §6.1 forbids and AC-8 - /// asserts against: the compositor could have sampled that texture where - /// it stood, and the round-trip costs 96% of the frame at 4K. Reading them - /// back to *encode a JPEG* is not a shortcut around anything — a file is - /// made of bytes on the CPU, and there is no path to one that does not - /// pass through here. + /// Reading pixels back to *display* them is what ARCH §6.1 forbids: the + /// compositor could have sampled that texture where it stood, and the + /// round-trip cost 96% of the frame at 4K — ~7 ms against a 0.28 ms + /// compute pass. There is now no method that does it, which is a stronger + /// guarantee than a feature gate: the display readback cannot be called + /// back into existence by turning something on. /// - /// So this is ungated where `read_output` is behind a feature: an export - /// must work in a shipping build, and the gate exists to keep the display - /// bridge out of one. Keeping them separate also means the instrumentation - /// AC-8 calls for can count display readbacks without counting exports. + /// Reading them back to *encode a file* is not a shortcut around anything. + /// A JPEG is made of bytes on the CPU and there is no path to one that + /// does not pass through here, so this is ungated and belongs in a + /// shipping build. pub fn export_pixels(&self) -> Result<(Vec, u32, u32), GpuError> { self.copy_output() } - /// The transfer itself, shared by both readers above. + /// The transfer itself. fn copy_output(&self) -> Result<(Vec, u32, u32), GpuError> { - let Some(target) = self.target.as_ref() else { + let Some(target) = self.targets[self.current].as_ref() else { return Err(GpuError::Readback("nothing rendered yet".into())); }; let (w, h) = (target.width, target.height); @@ -1141,6 +1171,66 @@ mod tests { assert_eq!(read_centre(&ctx, t)[3], 255); } + /// TRACES: FR-DSP-1 | AC-8 + #[test] + fn the_output_is_importable_by_a_compositor() { + // Every condition Slint checks before it will adopt a texture + // (`slint::wgpu_29`: `TextureImportError`). They are asserted here, + // in the crate that owns the descriptor, because failing them does not + // fail a build or a shader — it fails at runtime, on the frame the + // image is handed over, and only where there is a screen to hand it + // to. Nothing else in the test suite would notice. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = grey_image(&ctx, 4000); + let shader = EditGraph::default_chain().compose(); + let t = pass.render(&img, &shader, 16, 16).expect("render"); + + assert!( + matches!( + t.format(), + wgpu::TextureFormat::Rgba8Unorm | wgpu::TextureFormat::Rgba8UnormSrgb + ), + "import accepts only the two 8-bit RGBA formats, not {:?}", + t.format() + ); + assert!( + t.usage().contains(wgpu::TextureUsages::TEXTURE_BINDING), + "the compositor has to sample it" + ); + assert!( + t.usage().contains(wgpu::TextureUsages::RENDER_ATTACHMENT), + "Slint requires this even though the adjust pass never uses it" + ); + } + + /// TRACES: FR-DSP-1 | AC-8 + #[test] + fn consecutive_frames_are_different_textures() { + // Not a detail: the compositor is handed this texture rather than a + // copy of its pixels, and Slint repaints only when the image property + // *changes*. Two images over one texture compare equal, so writing the + // same texture every frame would leave a slider moving the pixels on + // the GPU and nothing at all on screen — the frame would be correct + // and invisible, which is the worst kind of wrong. + // + // No display is needed to catch it, because the equality Slint tests + // is the equality asserted here. + let Some(ctx) = ctx() else { return }; + let mut pass = AdjustPass::new(&ctx); + let img = grey_image(&ctx, 4000); + let shader = EditGraph::default_chain().compose(); + + let first = pass.render(&img, &shader, 16, 16).expect("render").clone(); + let second = pass.render(&img, &shader, 16, 16).expect("render").clone(); + assert_ne!(first, second, "the compositor cannot tell these two apart"); + + // And back again, so the alternation is a rotation between two rather + // than an allocation per frame — which at 4K would be 33 MB a frame. + let third = pass.render(&img, &shader, 16, 16).expect("render").clone(); + assert_eq!(first, third, "a third texture was allocated"); + } + #[test] fn the_output_resizes_with_the_viewport() { let Some(ctx) = ctx() else { return }; diff --git a/core/dr-gpu/src/lib.rs b/core/dr-gpu/src/lib.rs index fcc204a..b1f9e7e 100644 --- a/core/dr-gpu/src/lib.rs +++ b/core/dr-gpu/src/lib.rs @@ -6,6 +6,12 @@ //! //! Deliberately free of UI dependencies (ARCH §6.5a). The texture is handed //! out as a `wgpu::Texture`; who composites it is not this crate's concern. +//! +//! That independence is why [`GpuContext::new_shared`] hands back the raw +//! instance and adapter rather than talking to a compositor itself: the +//! compositor will only sample a texture that came from the device *it* draws +//! with, so somebody has to make one device for both — but it does not have to +//! be this crate, and this crate must not know who it is. use std::sync::Arc; @@ -33,19 +39,72 @@ pub struct GpuContext { adapter_info: wgpu::AdapterInfo, } +/// TRACES: FR-DSP-1 | AC-8 +/// One device, opened so that a compositor can be made to share it. +/// +/// The texture the adjust pass writes only reaches the screen without a copy +/// if the compositor is drawing with the *same* `wgpu::Device` — two devices +/// are two address spaces, and a texture from one is not a texture the other +/// can sample. So the device cannot be an implementation detail of either +/// side; it has to be made once and handed to both. +/// +/// [`Self::ctx`] is what the compute passes want. The instance and adapter are +/// what a compositor wants in order to adopt the same setup — Slint's +/// `WGPUConfiguration::Manual` asks for all four pieces — and they are handed +/// out raw rather than wrapped, because naming Slint here would put a UI +/// dependency in the one crate that must not have one (ARCH §6.5a). +pub struct SharedGpu { + /// The context every compute pass in this crate runs on. + pub ctx: GpuContext, + /// The instance the compositor will create its window surface from. + pub instance: wgpu::Instance, + /// The adapter [`Self::ctx`]'s device came from. + pub adapter: wgpu::Adapter, +} + impl GpuContext { /// Create a headless context — no surface, no window. /// - /// Used by tests and by the Slint path, which supplies its own surface. + /// Used by tests, by the examples, and by anything that only needs to + /// compute. A context opened this way cannot be shared with a compositor: + /// see [`Self::new_shared`] for that, and for why the difference matters. pub async fn new_headless() -> Result { + // GL is allowed alongside Vulkan here and nowhere else: a machine with + // no Vulkan loader should still run the tests, and a headless context + // never has to produce a window surface — which is precisely the thing + // the GL backend cannot do from an instance opened without a display + // handle. + Self::open(wgpu::Backends::VULKAN | wgpu::Backends::GL) + .await + .map(|shared| shared.ctx) + } + + /// TRACES: FR-DSP-1 | AC-8 + /// Open a device intended to be shared with the compositor. + /// + /// Vulkan only, unlike [`Self::new_headless`]. The caller will hand the + /// instance to a compositor that has to create a *window surface* from it, + /// and wgpu's GL backend reaches its display through EGL at instance + /// creation — an instance opened without a display handle, which is the + /// only kind available before a window exists, cannot then produce a GL + /// surface. Vulkan takes the window handle at surface creation instead, so + /// it is the only backend this order of operations permits. + /// + /// A machine with no Vulkan therefore gets no shared device, and the + /// caller is expected to carry on without the develop path rather than + /// refuse to start. + pub async fn new_shared() -> Result { + // Vulkan on both targets (D1), and here it is not merely the + // preference — see above. + Self::open(wgpu::Backends::VULKAN).await + } + + async fn open(backends: wgpu::Backends) -> Result { // `new_without_display_handle` rather than a struct literal: the // descriptor carries a boxed display handle and so has no `Default`, - // and a headless context is precisely the case with no display to - // hand it. + // and there is no window yet to take one from in either case. let mut descriptor = wgpu::InstanceDescriptor::new_without_display_handle(); - // Vulkan on both targets (D1). GL is allowed as a fallback so a - // machine without a Vulkan loader still runs the tests. - descriptor.backends = wgpu::Backends::VULKAN | wgpu::Backends::GL; + descriptor.backends = backends; let instance = wgpu::Instance::new(descriptor); let adapter = instance @@ -76,7 +135,14 @@ impl GpuContext { // in compute shaders are required, and the downlevel tier // does not guarantee them. This is effectively our GPU // floor (NFR-COMPAT-1). - required_limits: wgpu::Limits::default(), + // + // `using_resolution` raises only the texture-dimension limits, + // to whatever this adapter actually offers. That matters once + // a compositor shares this device: the default ceiling is + // 8192, and a swapchain image for a large or scaled display + // can exceed it — a limit we chose for our own compute passes + // would otherwise silently cap somebody else's window. + required_limits: wgpu::Limits::default().using_resolution(adapter.limits()), memory_hints: wgpu::MemoryHints::Performance, // Nothing behind a feature flag wgpu itself calls unstable — // the pipeline is ordinary compute and storage textures. @@ -88,10 +154,14 @@ impl GpuContext { .await .map_err(|e| GpuError::DeviceRequest(e.to_string()))?; - Ok(Self { - device: Arc::new(device), - queue: Arc::new(queue), - adapter_info, + Ok(SharedGpu { + ctx: Self { + device: Arc::new(device), + queue: Arc::new(queue), + adapter_info, + }, + instance, + adapter, }) } @@ -259,8 +329,16 @@ impl RenderTarget { // STORAGE_BINDING to write from compute; TEXTURE_BINDING so the // compositor can sample it. COPY_SRC exists only for tests — // production never reads this back (ARCH §6.1). + // + // RENDER_ATTACHMENT is not something this pass ever uses. It is + // there because Slint refuses to import a texture without it + // (`TextureImportError::InvalidUsage`), the compositor having to + // assume it may need to draw into what it was given. Declaring an + // unused capability costs an allocation flag and buys the whole + // zero-copy path, so it is a cheap price for AC-8. usage: wgpu::TextureUsages::STORAGE_BINDING | wgpu::TextureUsages::TEXTURE_BINDING + | wgpu::TextureUsages::RENDER_ATTACHMENT | wgpu::TextureUsages::COPY_SRC, view_formats: &[], }); diff --git a/core/dr-gpu/src/segment.rs b/core/dr-gpu/src/segment.rs index 8488c16..e3a5953 100644 --- a/core/dr-gpu/src/segment.rs +++ b/core/dr-gpu/src/segment.rs @@ -329,12 +329,19 @@ impl SegmentPass { /// The result of one segmentation: a basin label per pixel, on the GPU. pub struct Segmentation { + /// Only [`Self::read_field`] reads this, so a build without `readback` + /// carries it unread. That is now the ordinary build: dr-ui used to turn + /// the feature on for the whole workspace and stopped when S1 removed the + /// display readback, which is what made the field look dead. + #[cfg_attr(not(any(test, feature = "readback")), allow(dead_code))] ctx: GpuContext, width: u32, height: u32, /// Per pixel, the linear index of its basin root. Sparse — compacted by /// [`crate::hierarchy::RegionField::from_roots`]. labels: wgpu::Buffer, + /// As with `ctx` above: read only by [`Self::read_field`]. + #[cfg_attr(not(any(test, feature = "readback")), allow(dead_code))] gradient: wgpu::Buffer, } diff --git a/ui/dr-ui/Cargo.toml b/ui/dr-ui/Cargo.toml index 0119909..8382119 100644 --- a/ui/dr-ui/Cargo.toml +++ b/ui/dr-ui/Cargo.toml @@ -7,10 +7,11 @@ license.workspace = true [dependencies] dr-types.workspace = true -# The readback feature is on because Slint's texture-import path is unwired -# until spike S1; the develop view has no other way to reach the screen. It -# must come off when S1 lands (ARCH §6.1, AC-8). -dr-gpu = { workspace = true, features = ["readback"] } +# No `readback`. S1 wired Slint's texture import, so the develop view hands +# the compositor the texture itself and there is no display round-trip left to +# gate (ARCH §6.1, AC-8). The export path reads pixels back through +# `export_pixels`, which is ungated and always was. +dr-gpu.workspace = true dr-decode.workspace = true serde_json.workspace = true tokio.workspace = true @@ -29,7 +30,34 @@ rusqlite.workspace = true # android-activity on Android, and enabling both makes the backend selector # pick at random. So the backend features live on the target-specific # dependencies below rather than here. -slint = { workspace = true, features = ["compat-1-2", "renderer-femtovg"] } +# +# `renderer-femtovg-wgpu` rather than `renderer-femtovg`: the latter is +# FemtoVG over OpenGL, and a compositor drawing through GL cannot be handed a +# `wgpu::Texture`. Importing one requires Slint itself to be rendering with +# wgpu, and this is the FemtoVG backend that does (ARCH §6.1, spike S1). +# +# `unstable-wgpu-29` is the other half: the renderer feature makes Slint draw +# with wgpu, and this one exposes the API to say so — `BackendSelector:: +# require_wgpu_29` and `Image::try_from(wgpu::Texture)`. Unstable is Slint's +# word for it; the surface is small and the alternative is the 7 ms round-trip. +# +# `renderer-femtovg` is *not* kept alongside as a fallback, though it would +# still compile. Slint's winit backend prefers the wgpu FemtoVG renderer +# whenever both are built, so the GL one would only ever be reached by someone +# setting `SLINT_BACKEND=winit-femtovg` — and on that path an imported texture +# is not drawn at all. FemtoVG-over-GL has no branch for a `wgpu::Texture`, so +# it falls through to "render this image to a buffer", gets nothing back, and +# draws nothing. A blank canvas with no error is a far worse failure than the +# one below, so the fallback is removed rather than left as a trap. +# +# Consequence worth stating plainly: the desktop app now needs a working wgpu +# adapter to open a window at all. Slint refuses a CPU adapter for this +# renderer unless `SLINT_WGPU_CPU` is set in the environment. +slint = { workspace = true, features = [ + "compat-1-2", + "renderer-femtovg-wgpu", + "unstable-wgpu-29", +] } wgpu.workspace = true anyhow.workspace = true # `SettingsError` distinguishes an io failure from a malformed file, which the @@ -63,8 +91,6 @@ serde_norway.workspace = true [features] default = [] -# Temporary CPU readback path; see dr-ui docs and spike S1. -readback = ["dr-gpu/readback"] # Debug convenience: re-read style.yaml at startup so a palette can be tuned # without rebuilding. Costs the constant-folding of every token, so it stays # off by default and has no business in a release build. diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index f03f4d3..4428cf2 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -502,15 +502,25 @@ impl DevelopSession { Some((op.id, param.id)) } + /// TRACES: FR-DSP-1 | AC-8 /// Render at the requested display size and hand back a Slint image. /// /// Renders at *viewport* resolution rather than sensor resolution, which /// is what keeps slider interaction inside the frame budget on a 24 MP /// file (FR-DSP-1). /// - /// The readback at the end is the temporary bridge documented on - /// `AdjustPass::read_output`: ARCH §6.1 forbids it, and spike S1 removes - /// it by importing the texture into Slint directly. + /// **The image is the texture, not a copy of it.** This used to end in a + /// `read_output` into a `SharedPixelBuffer` — the GPU→CPU→GPU round-trip + /// ARCH §6.1 forbids and AC-8 asserts against, measured at ~7 ms at 4K + /// against a 0.28 ms compute pass. Spike S1 replaced it with + /// `slint::Image::try_from`, which wraps the texture where it already is. + /// The `clone` below is a refcount on the wgpu handle, not on the pixels. + /// + /// This only works because the compositor is drawing with the same device + /// the pass wrote with; see `shared_gpu` in the crate root for how that is + /// arranged, and note that nothing here can detect it having gone wrong — + /// a texture from a foreign device is a runtime fault on a real screen, + /// which is why the arrangement is made once at startup and never again. pub fn render(&mut self, width: u32, height: u32) -> Result { // Fit the render to the viewport while preserving aspect, so the // pass does no work on pixels the view will letterbox away. @@ -523,14 +533,17 @@ impl DevelopSession { let (w, h) = fit(fw, fh, width.max(1), height.max(1)); let shader = self.graph.compose(); - self.adjust + let texture = self + .adjust .render(&self.demosaiced, &shader, w, h) .map_err(|e| e.to_string())?; - let (pixels, rw, rh) = self.adjust.read_output().map_err(|e| e.to_string())?; - let buffer = - slint::SharedPixelBuffer::::clone_from_slice(&pixels, rw, rh); - Ok(slint::Image::from_rgba8(buffer)) + // The import is fallible on format and usage only, and both are fixed + // in `AdjustPass`'s texture descriptor — so a failure here is a + // descriptor that drifted, not anything the caller did. Say that, + // rather than surfacing "InvalidUsage" to a photographer. + slint::Image::try_from(texture.clone()) + .map_err(|e| format!("the render target is not importable by the compositor: {e}")) } /// Render the *whole* frame for the crop overlay to be drawn over. @@ -592,13 +605,17 @@ impl DevelopSession { /// screen-sized file. This renders the framed output size instead, so the /// export is the full-quality path FR-EXP-9 requires. /// - /// The readback here is `export_pixels`, not the display bridge: a file - /// is made of bytes on the CPU and there is no path to one that avoids - /// the transfer. See the note on that method for why the two are separate. + /// This reads pixels back and [`Self::render`] does not, and that is the + /// whole distinction AC-8 draws: a file is made of bytes on the CPU and + /// there is no path to one that avoids the transfer, whereas a frame on + /// screen had no business making the trip. See `AdjustPass::export_pixels` + /// for the longer version. /// - /// Leaves the pass holding a full-resolution target, so the caller should - /// expect the next display render to reallocate. Cheaper than keeping a - /// second pass alive for the exports a session rarely performs. + /// Leaves one of the pass's two targets at full resolution; it is dropped + /// and reallocated on the second display render after this, since the + /// other target still holds a viewport-sized texture and comes up first. + /// Cheaper than keeping a second pass alive for the exports a session + /// rarely performs. pub fn render_for_export(&mut self) -> Result { let (sw, sh) = self.demosaiced.size(); let (w, h) = self.graph.output_size(sw, sh); @@ -996,6 +1013,145 @@ mod tests { use super::*; use dr_pipeline::EditGraph; + /// TRACES: FR-DSP-1 | AC-8 + /// Copy a displayed frame back to the CPU, for assertions and nothing else. + /// + /// The library has no such function on purpose: S1 removed the display + /// readback, and AC-8 is the assertion that it stayed removed. A test that + /// wants to look at the pixels therefore has to do the copy itself, which + /// is exactly the right shape — the round-trip lives in the test binary + /// and cannot be reached from a shipping one. + /// + /// Doubles as the proof: this only compiles because the image *is* a wgpu + /// texture. Hand it a `SharedPixelBuffer`-backed image and it panics. + fn read_back(ctx: &GpuContext, image: &slint::Image) -> Vec { + let texture = image + .to_wgpu_29_texture() + .expect("the develop canvas must be a GPU texture, not a pixel buffer"); + let (w, h) = (texture.width(), texture.height()); + + // Buffer rows must be aligned to COPY_BYTES_PER_ROW_ALIGNMENT. + let unpadded = w * 4; + let align = wgpu::COPY_BYTES_PER_ROW_ALIGNMENT; + let padded = unpadded.div_ceil(align) * align; + + let buf = ctx.device.create_buffer(&wgpu::BufferDescriptor { + label: Some("test-readback"), + size: u64::from(padded * h), + usage: wgpu::BufferUsages::COPY_DST | wgpu::BufferUsages::MAP_READ, + mapped_at_creation: false, + }); + + let mut enc = ctx.device.create_command_encoder(&Default::default()); + enc.copy_texture_to_buffer( + wgpu::TexelCopyTextureInfo { + texture: &texture, + mip_level: 0, + origin: wgpu::Origin3d::ZERO, + aspect: wgpu::TextureAspect::All, + }, + wgpu::TexelCopyBufferInfo { + buffer: &buf, + layout: wgpu::TexelCopyBufferLayout { + offset: 0, + bytes_per_row: Some(padded), + rows_per_image: Some(h), + }, + }, + wgpu::Extent3d { + width: w, + height: h, + depth_or_array_layers: 1, + }, + ); + ctx.queue.submit(Some(enc.finish())); + + let slice = buf.slice(..); + let (tx, rx) = std::sync::mpsc::channel(); + slice.map_async(wgpu::MapMode::Read, move |r| { + let _ = tx.send(r); + }); + ctx.device + .poll(wgpu::PollType::wait_indefinitely()) + .expect("poll"); + rx.recv().expect("map").expect("map"); + + let data = slice.get_mapped_range(); + let mut out = Vec::with_capacity((unpadded * h) as usize); + for row in 0..h { + let start = (row * padded) as usize; + out.extend_from_slice(&data[start..start + unpadded as usize]); + } + drop(data); + buf.unmap(); + out + } + + /// TRACES: FR-DSP-1 | AC-8 + #[test] + fn the_displayed_frame_is_a_texture_and_not_a_pixel_buffer() { + // The acceptance criterion itself, asserted from the side that would + // notice it regressing. `to_rgba8` returning `Some` would mean the + // frame had come back through system memory to be looked at, which is + // the ~7 ms per frame at 4K that ARCH §6.1 forbids; `to_wgpu_29_texture` + // returning `Some` means the compositor got the texture where it lay. + // + // Note this passes without a display: the import is a wrapper, and it + // is the *compositor* adopting the device that needs a screen. What + // cannot be proved here is that the picture arrives; what can be + // proved is that no copy was made on the way. + let Ok(ctx) = pollster::block_on(dr_gpu::GpuContext::new_headless()) else { + log::warn!("no GPU adapter; skipping"); + return; + }; + + let rgba = vec![128u8; 32 * 32 * 4]; + let mut session = + DevelopSession::open_rgb(&ctx, &rgba, 32, 32, dr_types::Orientation::NORMAL) + .expect("session"); + let frame = session.render(32, 32).expect("render"); + + assert!( + frame.to_rgba8().is_none(), + "the canvas has CPU pixels, so something copied them there" + ); + let texture = frame + .to_wgpu_29_texture() + .expect("the canvas is neither a texture nor a pixel buffer"); + assert_eq!((texture.width(), texture.height()), (32, 32)); + } + + /// TRACES: FR-DSP-1 | AC-8 + #[test] + fn consecutive_frames_look_different_to_the_property_system() { + // The catch that comes free with handing over a texture instead of a + // buffer. Slint repaints when the image property *changes*, and it + // decides that with `PartialEq` — which for two images over one + // `wgpu::Texture` says "unchanged". A pass that reused a single target + // would therefore render every slider move correctly and show none of + // them. + // + // `AdjustPass` alternates between two targets to prevent it. This + // asserts the consequence in the terms Slint actually uses, so it + // would still catch the regression if the mechanism were replaced. + let Ok(ctx) = pollster::block_on(dr_gpu::GpuContext::new_headless()) else { + log::warn!("no GPU adapter; skipping"); + return; + }; + + let rgba = vec![128u8; 32 * 32 * 4]; + let mut session = + DevelopSession::open_rgb(&ctx, &rgba, 32, 32, dr_types::Orientation::NORMAL) + .expect("session"); + + let first = session.render(32, 32).expect("first render"); + let second = session.render(32, 32).expect("second render"); + assert_ne!( + first, second, + "the canvas property would not change, so the frame would never be shown" + ); + } + /// The whole scroll-to-zoom path, end to end, in the order the user drives /// it: show the image fitted, *then* turn the wheel. /// @@ -1032,12 +1188,14 @@ mod tests { assert!(session.is_zoomed(), "the session did not register the zoom"); let zoomed = session.render(64, 64).expect("zoomed render"); - let before = fitted.to_rgba8().expect("fitted pixels"); - let after = zoomed.to_rgba8().expect("zoomed pixels"); + // Both images are still readable here because consecutive frames go to + // alternating textures; see `AdjustPass::targets`. Holding two frames + // at once would be meaningless against a single reused target. + let before = read_back(&ctx, &fitted); + let after = read_back(&ctx, &zoomed); let differing = before - .as_bytes() .iter() - .zip(after.as_bytes().iter()) + .zip(after.iter()) .filter(|(a, b)| a != b) .count(); diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index d7acf76..0e5b54a 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -3,12 +3,17 @@ //! A viewer with a develop panel: open a folder of RAW files, decode and //! demosaic on the GPU, and adjust. //! -//! **Read before assuming A1 is proven.** Slint's public API for adopting an -//! externally created wgpu texture is not wired up here; this build uploads -//! through `SharedPixelBuffer`, which *is* a CPU round-trip — explicitly the -//! thing ARCH §6.1 forbids in production. Spike S1 replaces it. Until then A1 -//! is unvalidated, and the develop path pays a readback per frame that the -//! finished one will not. +//! **The develop frame never leaves the GPU** (ARCH §6.1, AC-8). Spike S1 +//! wired Slint's texture import: [`shared_gpu`] opens one wgpu device and +//! gives it to *both* the compute passes and Slint's renderer, and +//! `DevelopSession::render` then hands the compositor the very texture the +//! adjust pass wrote. What used to be a readback and an upload per frame is +//! now a refcount. +//! +//! `SharedPixelBuffer` still appears in this file and in the library grid, and +//! that is not a relapse: an embedded JPEG preview and a thumbnail are decoded +//! on the CPU and have no texture to hand over. AC-8 is about pixels that were +//! *computed on the GPU* travelling to the CPU and back to be looked at. //! //! **The develop panel is generated, not written.** [`develop`] asks the //! pipeline what parameters it has and builds a control per answer; no code @@ -576,6 +581,64 @@ enum PointsUpdate { Unchanged, } +/// TRACES: FR-DSP-1 | AC-8 +/// Open the one wgpu device the compute passes and the compositor share. +/// +/// **This is the whole of the zero-copy display path, and it is four lines of +/// configuration.** A `wgpu::Texture` belongs to the device that allocated it; +/// handing one to a compositor drawing on a *different* device is meaningless, +/// and the two would have to meet through system memory — which is the round +/// trip ARCH §6.1 forbids. So there is exactly one device, made here, before +/// anything else needs it. +/// +/// **Called before the window exists, and it must be.** `BackendSelector` +/// installs the Slint platform, and Slint installs a default one the first +/// time a window is created; selecting afterwards is too late. That is why the +/// GPU is opened at the top of [`run`] rather than beside the other +/// controllers, where it used to sit. +/// +/// `None` means develop is unavailable and the viewer falls back to embedded +/// previews — the same degradation as a machine with no adapter at all. +fn shared_gpu() -> Option { + let shared = match pollster::block_on(dr_gpu::GpuContext::new_shared()) { + Ok(shared) => shared, + Err(e) => { + log::warn!("no shareable GPU: {e}"); + return None; + } + }; + let dr_gpu::SharedGpu { + ctx, + instance, + adapter, + } = shared; + + // `Manual` is the variant that means "render with these, do not open your + // own". The two clones are of wgpu handles, which are refcounts over the + // one device and the one queue — not copies of either. + let configuration = slint::wgpu_29::WGPUConfiguration::Manual { + instance, + adapter, + device: (*ctx.device).clone(), + queue: (*ctx.queue).clone(), + }; + + if let Err(e) = slint::BackendSelector::new() + .require_wgpu_29(configuration) + .select() + { + // Dropping the context rather than keeping it: Slint has fallen back + // to a renderer that did not adopt our device, so every texture this + // context produces is one the compositor cannot sample. A disabled + // develop panel is a visible, explicable failure; a texture handed + // across devices is undefined behaviour on a good day. + log::warn!("Slint would not adopt the GPU device, develop disabled: {e}"); + return None; + } + + Some(ctx) +} + /// TRACES: M-13 | M-14 /// Build and run the viewer. pub fn run(paths: Vec) -> Result<()> { @@ -587,7 +650,21 @@ pub fn run(paths: Vec) -> Result<()> { let entries = Rc::new(RefCell::new(collect(&paths))); log::info!("{} image(s) to browse", entries.borrow().len()); + // Before the window, and it has to be: this selects the Slint backend, and + // creating a window selects one for us. See `shared_gpu`. The device is + // shared by demosaic, the adjust pass and the compositor; without one the + // app still browses through the preview path, just without develop. + let gpu = shared_gpu(); + let window = AppWindow::new()?; + match &gpu { + Some(ctx) => { + log::info!("adapter: {} ({:?})", ctx.adapter_name(), ctx.backend()); + window.set_adapter(ctx.adapter_name().into()); + window.set_backend(format!("{:?}", ctx.backend()).to_uppercase().into()); + } + None => window.set_backend("NO GPU".into()), + } // Every background job reports here, and this draws the bar across the top // of the shell and fills the settings page's list. Built before the @@ -913,22 +990,6 @@ pub fn run(paths: Vec) -> Result<()> { }); } - // The device is shared by demosaic and the adjust pass. Without one the - // app still browses through the preview path, just without develop. - let gpu = match pollster::block_on(dr_gpu::GpuContext::new_headless()) { - Ok(ctx) => { - log::info!("adapter: {} ({:?})", ctx.adapter_name(), ctx.backend()); - window.set_adapter(ctx.adapter_name().into()); - window.set_backend(format!("{:?}", ctx.backend()).to_uppercase().into()); - Some(ctx) - } - Err(e) => { - log::warn!("no GPU adapter: {e}"); - window.set_backend("NO GPU".into()); - None - } - }; - window.set_total(entries.borrow().len() as i32); let index = Rc::new(RefCell::new(0usize)); // The current develop session, if the file yielded sensor data. @@ -965,9 +1026,11 @@ pub fn run(paths: Vec) -> Result<()> { // **Half resolution while the gesture is still moving.** // - // The adjust pass and the readback both scale with pixel count, so - // halving each edge is roughly a quarter of the work — the - // difference between keeping up with a drag and lagging behind it. + // The adjust pass scales with pixel count, so halving each edge is + // roughly a quarter of the work — the difference between keeping + // up with a drag and lagging behind it. Less dramatic since S1 + // removed the readback that scaled the same way and cost far more, + // but a dispatch is still not free at 4K. // A draft frame is visible for one gesture and is replaced by a // full-resolution one the moment motion stops, so the cost is a // little softness exactly while the image is moving too fast to @@ -1013,7 +1076,8 @@ pub fn run(paths: Vec) -> Result<()> { // **Rendering is decoupled from input, and this is why.** // - // A render is a blocking GPU round-trip (see `AdjustPass::read_output`). + // A render used to be a blocking GPU round-trip — S1 removed the block, + // but not the reason for this, so read it as history that still applies. // Running one straight from a `moved` handler put that stall *inside* the // gesture: touch events arrive far faster than a render completes, so the // input queue backed up, positions arrived stale, and Android — seeing the