Show the develop frame itself, instead of a photocopy of it
The oldest open item in the project (ARCH §6.1, spike S1, AC-8). Every frame
in develop was read off the GPU into a `SharedPixelBuffer` and handed back to
Slint to upload again: ~7 ms at 4K against a 0.28 ms compute pass, 96% of the
frame spent carrying pixels to the CPU and back so they could be drawn where
they already were.
Slint 1.17 will adopt a `wgpu::Texture` directly, and the whole of what that
needs is arrangement rather than code.
**One device, made before the window.** A texture belongs to the device that
allocated it, so the compute passes and the compositor cannot each open their
own. `GpuContext::new_shared` opens one and hands back the instance and
adapter alongside it; `dr_ui::shared_gpu` gives all four to
`BackendSelector::require_wgpu_29(WGPUConfiguration::Manual { .. })`. That
call has to come before the first window, because creating one selects a
backend for you — which is why the GPU is now opened at the top of `run`
rather than two hundred lines down beside the other controllers.
dr-gpu still names no UI type. It hands out raw wgpu and does not ask who is
compositing (ARCH §6.5a).
**Vulkan only on the shared path**, where headless keeps its GL fallback.
wgpu's GL backend reaches its display through EGL at instance creation, and
before a window exists there is no display handle to give it — so a GL
instance cannot later produce the window surface Slint needs from it. A
machine with no Vulkan gets no shared device and browses without develop,
which is the same degradation as no adapter at all.
**`renderer-femtovg` becomes `renderer-femtovg-wgpu`.** The old one is FemtoVG
over OpenGL and cannot be handed a wgpu texture at all. It is not kept
alongside as a fallback: FemtoVG-over-GL has no branch for an imported
texture, falls through to "render this image into a buffer", gets nothing, and
draws nothing — a blank canvas with no error, which is worse than the failure
it would be papering over. The consequence is stated plainly in the manifest:
the desktop app now needs a working wgpu adapter to open a window.
**Two output textures, not one, and this is the part that is not obvious.**
Slint repaints when the image property *changes*, and it decides that with
`PartialEq` — which for two images over the same `wgpu::Texture` says
"unchanged". A pass that reused a single target would have rendered every
slider move correctly on the GPU and shown none of them: right, and invisible.
`AdjustPass` alternates between two targets, so consecutive frames are
genuinely different values. It also settles the read-while-write question that
one queue was already answering.
`RENDER_ATTACHMENT` is added to both render targets. Neither pass uses it;
Slint rejects an imported texture without it, on the reasoning that a
compositor handed a texture may need to draw into it.
**`AdjustPass::read_output` is deleted rather than gated.** It and
`export_pixels` were the same transfer under two names, and the comments
explaining why they were separate are the point of the whole criterion:
reading pixels back to *display* them is the defect, reading them back to
*encode a file* is the only way a file is made. The display twin is now gone
outright, which is stronger than a feature flag — it cannot be turned back on.
`export_pixels` is untouched and still ungated. The `readback` feature comes
off dr-ui, darkroom-desktop and darkroom-android; it stays in dr-gpu, where it
still gates `RenderTarget::read_pixels` and the segmentation field readback.
`examples/develop` moves to `export_pixels`, which is honest — it writes a
PPM — and so no longer needs the feature.
Four tests, each named for what it protects and each of which fails without a
screen if the property it guards breaks:
- the adjust target satisfies every condition Slint's import checks, asserted
in the crate that owns the descriptor, because a descriptor that drifts
fails at runtime on a real display and nothing else would notice;
- consecutive renders are different textures, and the third is the first
again, so the alternation is a rotation and not an allocation per frame;
- the develop canvas has no CPU pixel buffer and does have a wgpu texture —
AC-8 itself, in the terms Slint uses;
- consecutive frames compare unequal as `slint::Image`, which is the property
the repaint actually depends on.
The zoom test's readback moves into the test module. It has to: there is no
library function that copies a displayed frame to the CPU any more, and that
is the point — the round-trip now exists in the test binary and nowhere a
shipping build can reach.
**What is not proven.** No GUI was run. What is verified is that the texture
satisfies the import contract, that the import succeeds, that the canvas is a
texture rather than a buffer, and that consecutive frames are distinguishable.
What is unverified is everything that needs a display: that Slint's FemtoVG
wgpu renderer adopts the Manual configuration on a real surface, that the
picture appears the right way up and the right colour, and the frame timing
that motivated the whole exercise. Android is untouched by testing — the
android backend routes a WGPU29 request to Skia, whose wgpu surface does
handle imported textures, but that is read from the source, not observed.
56 dr-gpu tests and 255 dr-ui tests pass, clippy clean under `-D warnings`,
fmt clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
+176
-18
@@ -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<slint::Image, String> {
|
||||
// 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::<slint::Rgba8Pixel>::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<dr_export::Frame, String> {
|
||||
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<u8> {
|
||||
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();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user