Export the photograph, not the canvas
Zooming the develop view changed the exported file. `Framing::view` is kept out of the sidecar, out of `is_active` and out of `output_size` precisely so that it cannot — but those exclusions keep it out of the *edit*, and an export is a *render*. `visible_rect` deliberately folds the view into the single rect the fused shader's prologue samples, so `render_for_export` inherited it: at 4:1 it wrote the middle of the frame, magnified to fill the file at the full output size, with the detail kernels scaled four times over because `render_scale` folds the view in as well. `render_thumbnail` did the same to the grid. `render_uncropped` already suspends the view for this exact reason, so the fix is its pattern: one `render_the_file` that both file-producing paths go through, composing inside the suspension since the view reaches the shader as a uniform baked at composition time. Restored whatever happens — leaving the graph un-zoomed after a failed export would throw away where the photographer was looking. Nothing caught it because the guard checked the wrong things. `zooming_does_not_change_the_exported_image` asserted the output size and the crop; both held perfectly throughout. Renamed to `zooming_does_not_change_the_size_or_the_crop`, which is what it tests, and the pixels are now guarded where pixels exist. The new test uses a ramp rather than quadrants deliberately: a four-quadrant frame is self-similar under a centred zoom, and the first version of this test passed against the bug because of it. Traces FR-EXP-9, which asks for the full-quality pipeline "regardless of what the display was showing". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -348,9 +348,19 @@ pub struct Framing {
|
|||||||
/// Which part of the framed image the viewport is looking at.
|
/// Which part of the framed image the viewport is looking at.
|
||||||
///
|
///
|
||||||
/// **Not an edit.** Zooming changes what you are inspecting, never what
|
/// **Not an edit.** Zooming changes what you are inspecting, never what
|
||||||
/// the file becomes: it is excluded from [`Self::is_active`], from the
|
/// the file becomes, so it is excluded from [`Self::is_active`], from the
|
||||||
/// structure hash, and from the sidecar, so a zoomed view exports exactly
|
/// structure hash, and from the sidecar.
|
||||||
/// as an unzoomed one does.
|
///
|
||||||
|
/// **That is not on its own enough to make an export ignore it**, and
|
||||||
|
/// this doc comment used to claim it was. The exclusions keep the view out
|
||||||
|
/// of the *edit* — out of what is saved, out of the output size, out of
|
||||||
|
/// the crop. They cannot keep it out of a *render*, because
|
||||||
|
/// [`Self::visible_rect`] deliberately folds it into the one rect the
|
||||||
|
/// shader samples. Anything composing this framing and rendering it gets
|
||||||
|
/// the zoom; a caller that wants the photograph rather than the canvas
|
||||||
|
/// has to suspend the view first, as `DevelopSession::render_the_file`
|
||||||
|
/// and `render_uncropped` both do. Exporting at 4:1 wrote the middle of
|
||||||
|
/// the frame, magnified, until it did.
|
||||||
///
|
///
|
||||||
/// It lives here rather than in the UI because it composes with the crop
|
/// It lives here rather than in the UI because it composes with the crop
|
||||||
/// in the same normalised space — nesting one rect inside the other is a
|
/// in the same normalised space — nesting one rect inside the other is a
|
||||||
@@ -1252,10 +1262,18 @@ mod tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn zooming_does_not_change_the_exported_image() {
|
fn zooming_does_not_change_the_size_or_the_crop() {
|
||||||
// The property that makes zoom a viewing tool rather than an edit: it
|
// The property that makes zoom a viewing tool rather than an edit: it
|
||||||
// must not reach the output size or the crop. If it did, exporting
|
// must not reach the output size or the crop.
|
||||||
// while zoomed would write the zoomed view.
|
//
|
||||||
|
// **Renamed, because the old name promised more than the body checks
|
||||||
|
// and the gap was where a real bug lived.** "Does not change the
|
||||||
|
// exported image" was read as a guarantee about pixels; it is a
|
||||||
|
// guarantee about two numbers. Both held perfectly while
|
||||||
|
// `render_for_export` was writing the zoomed view at full size,
|
||||||
|
// because the view reaches the render through `visible_rect` and
|
||||||
|
// never through either of these. The pixels are guarded where pixels
|
||||||
|
// exist — `dr-ui`'s `export_ignores_the_viewport`.
|
||||||
//
|
//
|
||||||
// The structure key is deliberately not asserted here — see
|
// The structure key is deliberately not asserted here — see
|
||||||
// `zooming_from_neutral_changes_the_structure_key` for why it must
|
// `zooming_from_neutral_changes_the_structure_key` for why it must
|
||||||
|
|||||||
+21
-21
File diff suppressed because one or more lines are too long
+45
-8
@@ -3371,13 +3371,53 @@ impl DevelopSession {
|
|||||||
let (sw, sh) = self.demosaiced.size();
|
let (sw, sh) = self.demosaiced.size();
|
||||||
let (w, h) = self.graph.output_size(sw, sh);
|
let (w, h) = self.graph.output_size(sw, sh);
|
||||||
|
|
||||||
let shader = self.graph.compose_for(space);
|
let (pixels, rw, rh) = self.render_the_file(w, h, space)?;
|
||||||
self.render_with_masks(&shader, w, h, space)?;
|
|
||||||
|
|
||||||
let (pixels, rw, rh) = self.adjust.export_pixels().map_err(|e| e.to_string())?;
|
|
||||||
dr_export::Frame::in_space(rw, rh, pixels, space).map_err(|e| e.to_string())
|
dr_export::Frame::in_space(rw, rh, pixels, space).map_err(|e| e.to_string())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// TRACES: FR-EXP-9 | FR-CAT-9
|
||||||
|
/// Render the *photograph*, with the viewport suspended, and read it back.
|
||||||
|
///
|
||||||
|
/// **The one thing separating a file from a frame on screen**, and the
|
||||||
|
/// reason both file-producing paths go through here rather than composing
|
||||||
|
/// for themselves. [`Framing::view`](../dr_pipeline/framing/struct.Framing.html#method.view)
|
||||||
|
/// is not an edit — it is kept out of the sidecar, out of `is_active` and
|
||||||
|
/// out of `output_size` precisely so that zooming cannot change what the
|
||||||
|
/// file becomes. But it is folded into `visible_rect`, which is the rect
|
||||||
|
/// the fused shader's prologue samples, so a path that composes the graph
|
||||||
|
/// and renders it inherits the zoom whether or not it wanted it. Exporting
|
||||||
|
/// at 4:1 wrote the middle of the frame magnified to fill the file, at the
|
||||||
|
/// full output size, with the detail kernels scaled four times over —
|
||||||
|
/// silently, since every dimension the old guard checked still held.
|
||||||
|
///
|
||||||
|
/// Suspended rather than refused: an export is a thing the photographer
|
||||||
|
/// asks for *while* inspecting a highlight at 4×, and demanding they zoom
|
||||||
|
/// out first would be answering a question nobody asked.
|
||||||
|
///
|
||||||
|
/// Restored whatever happens, for the reason [`Self::render_uncropped`]
|
||||||
|
/// restores it: leaving the graph un-zoomed after a failed export would
|
||||||
|
/// throw away where the photographer was looking.
|
||||||
|
fn render_the_file(
|
||||||
|
&mut self,
|
||||||
|
w: u32,
|
||||||
|
h: u32,
|
||||||
|
space: dr_types::ColourSpace,
|
||||||
|
) -> Result<(Vec<u8>, u32, u32), String> {
|
||||||
|
let saved_view = self.graph.framing().view();
|
||||||
|
self.graph.framing_mut().set_view(CropRect::default());
|
||||||
|
|
||||||
|
// Composed *inside* the suspension: the view reaches the shader as a
|
||||||
|
// uniform baked at composition, so composing before this point would
|
||||||
|
// restore the framing and export the zoom anyway.
|
||||||
|
let shader = self.graph.compose_for(space);
|
||||||
|
let rendered = self.render_with_masks(&shader, w, h, space);
|
||||||
|
|
||||||
|
self.graph.framing_mut().set_view(saved_view);
|
||||||
|
rendered?;
|
||||||
|
|
||||||
|
self.adjust.export_pixels().map_err(|e| e.to_string())
|
||||||
|
}
|
||||||
|
|
||||||
/// TRACES: FR-CAT-9
|
/// TRACES: FR-CAT-9
|
||||||
/// Render this edit small, for the grid's thumbnail.
|
/// Render this edit small, for the grid's thumbnail.
|
||||||
///
|
///
|
||||||
@@ -3594,10 +3634,7 @@ impl DevelopSession {
|
|||||||
let (fw, fh) = self.graph.output_size(sw, sh);
|
let (fw, fh) = self.graph.output_size(sw, sh);
|
||||||
let (w, h) = fit(fw, fh, edge.max(1), edge.max(1));
|
let (w, h) = fit(fw, fh, edge.max(1), edge.max(1));
|
||||||
|
|
||||||
let shader = self.graph.compose_for(dr_types::ColourSpace::Srgb);
|
let (pixels, rw, rh) = self.render_the_file(w, h, dr_types::ColourSpace::Srgb)?;
|
||||||
self.render_with_masks(&shader, w, h, dr_types::ColourSpace::Srgb)?;
|
|
||||||
|
|
||||||
let (pixels, rw, rh) = self.adjust.export_pixels().map_err(|e| e.to_string())?;
|
|
||||||
Ok((rw, rh, pixels))
|
Ok((rw, rh, pixels))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,123 @@
|
|||||||
|
//! TRACES: FR-EXP-9 | FR-CAT-9
|
||||||
|
//! Does an export render what the file is, or what the canvas is showing?
|
||||||
|
//!
|
||||||
|
//! FR-EXP-9 says an export uses the full-quality pipeline "regardless of what
|
||||||
|
//! the display was showing", and `Framing::view` is documented as "not an
|
||||||
|
//! edit" — kept out of the sidecar, out of `is_active`, out of the output
|
||||||
|
//! size, "so a zoomed view exports exactly as an unzoomed one does".
|
||||||
|
//!
|
||||||
|
//! It is folded into `visible_rect()` all the same, and that is the rect the
|
||||||
|
//! fused shader's prologue samples. So anything that composes the graph and
|
||||||
|
//! renders it inherits the zoom whether or not it wanted it.
|
||||||
|
//! `DevelopSession::render_uncropped` saves and restores the view for exactly
|
||||||
|
//! this reason; `render_for_export` and `render_thumbnail` do not.
|
||||||
|
//!
|
||||||
|
//! The framing crate's own guard, `zooming_does_not_change_the_exported
|
||||||
|
//! _image`, asserts only that the output *size* and the crop are untouched.
|
||||||
|
//! Both survive; the pixels do not, which is why this test is at the session
|
||||||
|
//! level where a render actually happens.
|
||||||
|
|
||||||
|
use dr_gpu::GpuContext;
|
||||||
|
use dr_types::{ColourSpace, Orientation};
|
||||||
|
use dr_ui::DevelopSession;
|
||||||
|
|
||||||
|
fn headless() -> Option<GpuContext> {
|
||||||
|
pollster::block_on(GpuContext::new_headless()).ok()
|
||||||
|
}
|
||||||
|
|
||||||
|
/// A ramp, not a pattern of quadrants.
|
||||||
|
///
|
||||||
|
/// Worth stating because the first version of this test used quadrants and
|
||||||
|
/// passed: a four-quadrant frame is self-similar under a centred zoom — the
|
||||||
|
/// middle quarter of it is four quadrants again — so it cannot tell a zoomed
|
||||||
|
/// render from an unzoomed one. A monotonic ramp can. The centre quarter of
|
||||||
|
/// it spans a narrow band of values, magnified across the whole target.
|
||||||
|
fn ramp(w: u32, h: u32) -> Vec<u8> {
|
||||||
|
let mut rgba = Vec::with_capacity((w * h * 4) as usize);
|
||||||
|
for y in 0..h {
|
||||||
|
for x in 0..w {
|
||||||
|
rgba.extend_from_slice(&[
|
||||||
|
(x * 255 / w.max(1)) as u8,
|
||||||
|
(y * 255 / h.max(1)) as u8,
|
||||||
|
// A fine stripe, so a magnification shows as well as a shift.
|
||||||
|
if x % 4 < 2 { 200 } else { 40 },
|
||||||
|
255,
|
||||||
|
]);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
rgba
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The span of the red channel across the top row — the ramp's full sweep on
|
||||||
|
/// an unzoomed frame, and a narrow slice of it on a zoomed one.
|
||||||
|
///
|
||||||
|
/// Compared instead of the buffers themselves because a failure that prints
|
||||||
|
/// 16 KB of pixels says nothing a reader can act on, and this says which part
|
||||||
|
/// of the photograph came out.
|
||||||
|
fn top_row_span(rgba: &[u8], width: u32) -> (u8, u8) {
|
||||||
|
let row = &rgba[..(width as usize) * 4];
|
||||||
|
let reds = row.iter().step_by(4);
|
||||||
|
(
|
||||||
|
reds.clone().copied().min().unwrap_or(0),
|
||||||
|
reds.copied().max().unwrap_or(0),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn an_export_is_the_same_frame_whether_or_not_the_canvas_is_zoomed() {
|
||||||
|
let Some(ctx) = headless() else { return };
|
||||||
|
let mut session = DevelopSession::open_rgb(&ctx, &ramp(64, 64), 64, 64, Orientation::NORMAL)
|
||||||
|
.expect("session");
|
||||||
|
|
||||||
|
let unzoomed = session
|
||||||
|
.render_for_export(ColourSpace::Srgb)
|
||||||
|
.expect("export");
|
||||||
|
let before = top_row_span(&unzoomed.rgba, unzoomed.width);
|
||||||
|
|
||||||
|
// What a scroll wheel over the middle of the canvas does.
|
||||||
|
session.zoom_about(4.0, 0.5, 0.5);
|
||||||
|
assert!(session.is_zoomed(), "the session did not take the zoom");
|
||||||
|
|
||||||
|
let zoomed = session
|
||||||
|
.render_for_export(ColourSpace::Srgb)
|
||||||
|
.expect("export");
|
||||||
|
let after = top_row_span(&zoomed.rgba, zoomed.width);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
(unzoomed.width, unzoomed.height),
|
||||||
|
(zoomed.width, zoomed.height),
|
||||||
|
"the export changed size with the zoom"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
before, after,
|
||||||
|
"the export followed the viewport: unzoomed it swept the ramp {before:?}, \
|
||||||
|
zoomed it wrote {after:?} — the middle of the frame, magnified to fill the file"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
unzoomed.rgba, zoomed.rgba,
|
||||||
|
"the exported pixels changed when only the viewport moved"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_thumbnail_is_the_same_frame_whether_or_not_the_canvas_is_zoomed() {
|
||||||
|
let Some(ctx) = headless() else { return };
|
||||||
|
let mut session = DevelopSession::open_rgb(&ctx, &ramp(64, 64), 64, 64, Orientation::NORMAL)
|
||||||
|
.expect("session");
|
||||||
|
|
||||||
|
let (w, _, unzoomed) = session.render_thumbnail(32).expect("thumbnail");
|
||||||
|
let before = top_row_span(&unzoomed, w);
|
||||||
|
|
||||||
|
session.zoom_about(4.0, 0.5, 0.5);
|
||||||
|
let (_, _, zoomed) = session.render_thumbnail(32).expect("thumbnail");
|
||||||
|
let after = top_row_span(&zoomed, w);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
before, after,
|
||||||
|
"the grid's thumbnail followed the viewport: {before:?} unzoomed against {after:?} zoomed"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
unzoomed, zoomed,
|
||||||
|
"the thumbnail's pixels moved with the view"
|
||||||
|
);
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user