Conversation
## Summary - Add a new `BrightroomParametric` SwiftPM product/target for platform-neutral parametric editing features. - Move Feature documents, registry-backed definitions, graph compilation, image rendering, video rendering, and Metal-backed mask kernels into the new module. - Keep the `EditingStack` bridge inside `BrightroomEngine` so legacy `EditingStack.Edit.Filters` can convert into parametric feature pipelines without making the new module depend on Engine/UI. - Add SwiftUI image/video playgrounds plus a lightweight macOS `ParametricMacDemo` target for interactive checks. ## Notes `Package.swift` now declares macOS support so the new `BrightroomParametric` product can build for macOS. This PR does not make the UIKit-backed `BrightroomEngine` or `BrightroomUI` surfaces a full macOS port. ## Validation - `plutil -lint Dev/Brightroom.xcodeproj/project.pbxproj` - `xcodebuild -project Dev/Brightroom.xcodeproj -scheme ParametricMacDemo -destination 'platform=macOS' -derivedDataPath build/CodexDerivedData build` - `xcodebuild -project Dev/Brightroom.xcodeproj -scheme BrightroomEngineTests -destination 'platform=iOS Simulator,id=05718949-3329-4852-9F7A-FA1649441821' -destination-timeout 60 -derivedDataPath build/CodexDerivedData -only-testing:BrightroomEngineTests/ParametricFeatureTreeTests test` - `xcodebuild -project Dev/Brightroom.xcodeproj -scheme SwiftUIDemo -destination 'platform=iOS Simulator,id=05718949-3329-4852-9F7A-FA1649441821' -destination-timeout 60 -derivedDataPath build/CodexDerivedData build`
muukii
marked this pull request as draft
June 10, 2026 11:20
## Summary
- Remove the load-time path that eagerly decoded the editing source into
an `MTLTexture` via `MTKTextureLoader`; collapse `CGImage._makeCIImage`
down to `CIImage(cgImage:).oriented()`.
- Delete `EditingStack.Options.usesMTLTextureForEditingImage` and its
plumbing (`makeMTLTexture` / `supportsImage` / `MTLImageCreationError`,
the now-unused `mtlDevice`, and the `MetalKit` import).
- Add `EditingImageWarmUp`: render the source once on a background queue
before publishing `loadedState`, warming Core Image's pipeline.
## Why
In the v5 canvas, `EditingCanvasMTKView` re-renders the source `CIImage`
into its own viewport texture every frame (`viewportSourceImage`). The
source's backing — `MTLTexture` vs `CGImage` — is therefore irrelevant
to display, so the load-time texture conversion is unnecessary. It only
carried downsides:
- `MTKTextureLoader` quantizes to **8-bit**, defeating the P3/EDR pixel
format `MetalImageView` configures.
- **16-bit images were never supported** and silently fell back to the
`CGImage` path, so behavior was already split in two.
- `CIImage(mtlTexture:)` required a **y-flip** that has been a recurring
source of orientation bugs (e.g. flipped export masks).
This direction also matches the Core Image guidance in WWDC 2026 Session
305 ("Enhance RAW image processing with Core Image"): use a per-view
Metal-backed `CIContext` rendering into an `MTKView`, and do not
pre-convert the source into a texture yourself.
## Preload (warm-up)
Dropping the texture path moves the GPU upload — previously done
off-thread by `MTKTextureLoader` during load — plus Core Image's
one-time pipeline-state compilation onto the first main-thread
`draw(in:)`, which visibly stalls the first frame. `EditingImageWarmUp`
addresses this:
- Renders the source downscaled to 256px through a dedicated
process-wide `CIContext(mtlDevice:)` (downscaling forces the whole
source to be sampled → full upload + pipeline compilation), with a tiny
read-back.
- Runs on a background queue in `handleImageLoaded` **before**
`loadedState` is assigned. Since `isLoading == (loadedState == nil)`,
the loading spinner stays up until warm-up finishes, so the canvas only
draws once the pipeline is hot.
- Never blocks the main thread. No new public API.
## Test plan
- [x] `xcodebuild -scheme BrightroomUI` build succeeds
- [x] `BrightroomEngineTests` pass with 0 failures (incl.
orientation/render suites: `RendererOrientationTests`,
`LocalAdjustmentMaskOrientationTests`, `RendererDeviceEquivalenceTests`)
- [ ] On-device verification of the first-load hitch (not yet done)
## Notes
- `Options.usesMTLTextureForEditingImage` was public, so its removal is
technically a breaking change. v5 already carries larger breaking
changes (e.g. PixelEditor removal), so this is in scope.
- The warm-up gates presentation: the spinner is slightly longer in
exchange for a hitch-free first frame.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
- EditingStack.Edit stores only [EditingFeature]; undo/redo over document snapshots - PixelEditor removed; PhotosCrop is the single built-in UI - BrightroomParametric: JSON/registry model deleted; Swift-native value tree with protocol-dispatched evaluation; ParametricDocumentCodec with typed versioned migration, encode-side boundary enforcement, and formatVersion - Core Image kernels moved to ParametricKernels.metal (precompiled metallib first, runtime-source fallback for pure SwiftPM) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the legacy Filtering/AnyFilter/Filter* layer with the BrightroomParametric vocabulary across the engine and UI. - EditingStack.Edit features carry parametric payloads: .effects(EffectPipeline) / .localAdjustment(LocalAdjustmentFeature) / .crop(EditingCrop). FeatureID is the feature identity; the UUID glue is gone. - Renderer evaluates .effects/.localAdjustment operations, and preview and export share one composition path (pinned by EditingPreviewExportParityTests). Local-adjustment effect failures now surface on export (throwing) while the preview degrades to identity. Disabled brush leaves select nothing in both paths, matching the parametric compiler. - Canvas/CropView/PhotosCrop speak EffectPipeline / BrushMaskStroke / FeatureID. PhotosCrop owns effect ordering (PhotosCropEffectOrder); the blur seed is the scale-invariant GaussianBlurFeature(value: 40); presets are PresetFeature; ColorCubeLoader returns ColorCubeFeature. - Delete the legacy filter layer: 15 Filter* files, Filtering/AnyFilter, EditingStackParametricBridge, ColorCubeStorage, and the previewFilterPresets / previewScale machinery. - Tests: port the suite to parametric types; add preview/export parity, brush-falloff parity, visual-evidence, and per-path performance benchmarks (111 passing). - Docs: refresh AGENTS.md guidance and add docs/performance-notes.md with profiling results and improvement opportunities. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SwiftUIImagePreviewView (and the internal _ImagePreviewView / _PreviewImageView) was only referenced by demo code. Remove it together with the demos that used it: ImagePreviewDemoView in RenderingDemoView, the DemoFilterView custom-effects screen, and their ContentView entry points. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y disk export
Engine/UI checkpoint for the v5 parametric editing work. Builds (engine, UI, SwiftUI demo) and 136 BrightroomEngineTests pass.
Document & crop model:
- Delete EditingCrop and EditingFeature; EditingStack.Edit now holds an
EditingDocument directly and crop is a parametric CropFeature (y-up rect,
QuarterTurn rotation, straighten). Add CropGeometry helper and rebase the
RenderCrop math on CropFeature. BrightroomUI edits crop through a y-down
CropEditingState/CropRotation working model with a single shared y-flip/snap.
Rendering:
- Single rendering path: every export compiles the document to one CIImage
recipe via ParametricImageRenderer. Remove the CoreGraphics fast path
(renderOnlyCropping / axisAlignedCropOnlyFeature / pixelCropRect) and the
Rendered.Engine distinction.
- BrightRoomImageRenderer.render is now a single async throws -> Rendered.
Output (in-memory bitmap vs file) is chosen via Options.output; Rendered hides
the disk/memory backing (cgImage/uiImage get throws, fileURL, thumbnail).
Large-image memory:
- Fix the editing-canvas local-adjustment blur OOM on huge images (e.g. 12000^2
'Nasa'): evaluate effects + the local effect at the downsampled ~2560 editing
source then upscale, instead of blurring the full-canvas image. The blur
radius is a fraction of image extent, so the upscaled preview matches the
full-resolution export.
- Bounded-memory disk export (Output.file): CIImageStreamingFileWriter renders
the recipe strip-by-strip into an mmap'd temp file and encodes lazily through
a direct CGDataProvider (no full-resolution CGImage, no CGImageDestination
over the whole bitmap). Replaces write{JPEG,HEIF}Representation, which
materialized the full bitmap in RAM. (On-device peak-memory verification still
pending.)
Brush mask:
- Share one brush falloff (BrushStampSharedSource) across the export/preview
CIKernel and the live Metal shader; the live canvas renders committed + active
strokes in one real-time pass.
Tests/docs:
- Add CropGeometry, BrushStampFalloff, BrushMaskRasterizerParity, LargeMaskedExport,
MaskedPreviewExportScaleConsistency, and DiskExport suites; migrate renderer
tests to the async API. Track remaining follow-ups in docs/editing-engine-todo.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Set the package to swift-tools-version 6.3 with swiftLanguageModes [.v6] and resolve the data-race-safety diagnostics across all three library modules (0 errors; demo and tests build green). Key changes: - Lock-guarded global CIContext caches -> nonisolated(unsafe). - Add Sendable to renderer value types (Options/Rendered/Output/Resolution/RenderingDevice) and CropViewFeatureFocus; ImageSource -> @unchecked Sendable. - PresetStorage, UIKit layout/frame-rate helpers, and the display-link holder -> @mainactor; view teardown uses isolated deinit. - BrightRoomImageRenderer.render runs off-actor (@Concurrent) over a Sendable snapshot, with a shared synchronous core kept for benchmarks; makeRenderer returns a sending value. - Remove the now-unused CIImageDisplaying protocol; add SwiftUIMetalImageView wrappers. Remaining warnings: EditingStack/ImageProvider background-queue self-captures (isolation-model follow-up) and pre-existing iOS deprecations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Fixes the editing canvas clamping **Display-P3 to sRGB** (export was already correct — this was a **preview-only WYSIWYG defect**, not data loss), and makes the canvas wide-gamut end to end. **Root cause:** the canvas ran a closed sRGB/8-bit loop. One color constant tagged every Metal↔Core Image hop as sRGB, and all color textures + the drawable were `.bgra8Unorm`, so the very first source bake hard-clipped out-of-sRGB-gamut P3 chroma; every downstream stage re-clipped. ## Changes **Four-boundary color contract** (`EditingCanvasImageProcessing`), replacing the single `colorSpace`: - `workingColorSpace` == `intermediateColorSpace` = `extendedLinearDisplayP3` — the **same** space writes AND reads back every intermediate texture, so each hop round-trips losslessly through a float texture. - `drawableColorSpace` = `displayP3`, kept identical to `CAMetalLayer.colorspace` → a single linear→display conversion, no double color management. - `maskColorSpace` = `sRGB` (unchanged) — the mask is a [0,1] selection field, so the brush feather fed to `CIBlendWithAlphaMask` is behavior-preserving. **Canvas** (`EditingCanvasMTKView`): - CIContext: `workingColorSpace` = extendedLinearDisplayP3, `workingFormat` = `.RGBAh`. - Color intermediates (source/base/adjusted) → `.rgba16Float`; drawable → `.bgr10a2Unorm` (10-bit P3 SDR). - Layer colorspace set in `init` and re-asserted in `didMoveToWindow` (MTKView rebuilds its drawable from `colorPixelFormat`). Both canvas surfaces share the init, so CropView's instance is fixed too. **Export** (`ParametricExportRenderer`): always process through an **extended-linear Display-P3 working space + half-float (`.RGBAh`) working buffer**, so out-of-sRGB / out-of-range chroma survives the multi-filter chain. The previous setup (CI's default sRGB-primary working space, 8-bit) clipped edited wide-gamut sources mid-chain. This is the processing precision only; the **output** stays `options.workingFormat` (8-bit, source-space-tagged, for SDR delivery). The export working space now matches the canvas, so canvas and export agree. Measured — an Adobe RGB green `(0,1,0)` (outside both sRGB and P3) edited (blur) then exported back to Adobe RGB: | working space / format | exported (R,G,B) | |---|---| | default sRGB-primary, 8-bit (old) | `(144, 255, 60)` — badly contaminated | | default sRGB-primary, RGBAh | `(4, 255, 60)` | | **extended-linear Display-P3, RGBAh (this PR)** | **`(0, 255, 0)` — faithful** | On the editing canvas, out-of-P3 source colors (Adobe RGB greens/cyans) are clamped to the P3 gamut boundary at the final drawable write — correct and unavoidable, since a P3 panel cannot display colors outside its gamut; the internal pipeline preserves them up to that point. **Test + demo:** `EditingCanvasColorContractTests` renders P3 red through the new vs old contract — both encoded to Display-P3 — and asserts the new path stays saturated while the old desaturates (runs on the simulator: it tests color **math**, not the panel, which can't show wide gamut). Added an "Insta Logo" PhotosCrop demo entry using the existing Display-P3 `Asset.instaLogo` sample. ## Verification - BrightroomUI + SwiftUIDemo build clean; **137 engine tests pass** (135 prior + 2 new). - **On real P3 hardware:** P3 now renders correctly in the editing canvas and matches export (confirmed via the PhotosCrop "Insta Logo" sample). ## Not included (follow-up) - **EDR/HDR:** float drawable + `wantsExtendedDynamicRangeContent` + per-frame `currentEDRHeadroom` tone-mapping, plus HDR gain-map ingest. The float intermediates + extended-linear working space introduced here make that mostly a layer-config + one view-node addition. ## Note - The `.bgr10a2Unorm` drawable has only 2-bit alpha; verified on-device that transparent-edge quality is fine. Flipping the single `drawablePixelFormat` constant to `.rgba16Float` (full alpha, also the EDR format) is the escape hatch if it ever matters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Migrates the `BrightroomEngineTests` suite from **XCTest** to the **Swift Testing** framework (`import Testing`). 27 test files are converted; one performance file is intentionally left on XCTest (see below). `XCTestCase` classes become `struct` / `final class` suites, test methods gain `@Test` with sentence-case raw identifiers, and assertions move to `#expect` / `#require`. ## Required project change The `BrightroomEngineTests` target was typed as `com.apple.product-type.bundle.**ui-testing**` — a hostless logic-test bundle that had been mistyped. Swift Testing is **not available in UI-testing bundles** (`import Testing` fails with `Unable to resolve module dependency: '_Testing_Unavailable'`), so the product type is changed to `com.apple.product-type.bundle.**unit-test**`. No host app / `TEST_HOST` is involved, so this is a no-op for how the logic tests run. ## Notable conversions - **`expectation` / `wait`** (`LoadingTests`) → `withCheckedContinuation` (kept `@MainActor` for the StateGraph observation). - **`XCTSkip`** for a missing Metal device → `@Test(.enabled(if: MTLCreateSystemDefaultDevice() != nil))` + `#require`. - **`XCTAttachment(image:)`** → `Attachment.record(uiImage.pngData()!, named:)`. - **`file:`/`line:` forwarding helper** → `sourceLocation: SourceLocation = SourceLocation(fileID:filePath:line:column:)` default arg (no underscored API). - **`continueAfterFailure = false`** → `try #require` throughout that suite. - **`tearDown` cleanup** → `deinit` (`DiskExportTests` kept as `final class`). - **`XCTAssertThrowsError`** → `#expect(throws:)`, capturing the value where the error is inspected. ## Kept on XCTest (deliberate) `EnginePerformanceWorkloadTests` uses `measure(metrics:options:)` with `XCTClockMetric` / `XCTCPUMetric` as CPU-instruction regression guards. Swift Testing has no performance-measurement equivalent, so this file stays on XCTest. A single test bundle runs both frameworks. ## Verification - `xcodebuild ... build-for-testing` — **succeeds, 0 errors**. - `xcodebuild ... test-without-building` — **all 134 tests in 31 suites pass** (mixed XCTest + Swift Testing), including the rebased wide-gamut `EditingCanvasColorContractTests`. Rebased on top of the latest `v5` (wide-gamut PR #298); the new `EditingCanvasColorContractTests` added there is migrated here as well. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pth (#300) ## What Fixes two visible defects in blur-mask painting (PhotosCrop), plus a related export-path safety change. ### 1. See-through — the unblurred image bled through the blur The brush default `opacity` was **0.9**, and the mask accumulates with **max** (live = Metal `.max` blend; export = `CIBlendKernel.componentMax`). So opacity acts as a *hard ceiling* — overlapping/repeated strokes can never exceed the single-stamp peak. `CIBlendWithAlphaMask` then composited `blur·0.9 + original·0.1`, leaving ~10% of the sharp original visible even at the stroke core. **Fix:** default `opacity` 0.9 → **1.0** (`hardness` 0.72 still feathers the edge). The color-space / premultiplied-alpha theories were investigated and **ruled out**: the mask, base, and adjusted images are all sRGB (`EditingCanvasImageProcessing.colorSpace`), and `CIBlendWithAlphaMask` blends using only the alpha channel, which is color-space independent. ### 2. Visible stamp seams along a stroke Default `spacing` 0.18 placed stamps `0.36·radius` apart — wider than the hardness‑0.72 falloff ring of `(1−0.72)·radius = 0.28·radius` — so the feathered edge scalloped between stamps. **Fix:** default `spacing` 0.18 → **0.05** (`~0.10·radius`, about ⅓ of the ring). Preview and export share the **same stored stamp list** (commit performs no decimation; export rasterizes stored stamps 1:1), so this densifies both — as requested. ### 3. Export safety — bound the mask Core Image graph depth `FeatureGraphCompiler.render(_:BrushMask)` folded each stamp into the mask with `componentMax` **linearly**, making the CI graph depth O(n stamps). Denser stamps would risk slow compiles / stack overflow on long strokes. **Fix:** reduce stamps in a **balanced tree** (`reduceComponentMax`, O(log n) depth). `componentMax` is associative and commutative and `componentMax(transparent, x) == x`, so the result is identical. ## Why Direct response to the report: blur mask compositing looked wrong (original faintly showing through) and stamp joints were visible. Both trace to brush defaults; the export reducer change keeps the denser default safe. ## Reviewer notes - Behavior change is limited to **new** strokes' defaults; existing committed strokes render from their stored stamps unchanged. - `EditingCanvasStrokeRecord.init(brushMaskStroke:)` spacing is a display-only placeholder (`BrushMaskStroke` doesn't persist spacing); updated to match the authoring default. - Base branch is **v5** (this branch sits on the v5 line, not `main`). ## Verification - `BrightroomUI` builds (iOS Simulator). - Tests pass: `BrushMaskRasterizerParityTests`, `CommittedMaskParametricParityTests`, `LargeMaskedExportTests`, `LocalAdjustmentRenderingTests` (the balanced-tree reduction preserves mask parity). - `opacity 1.0` / `spacing 0.05` are values best confirmed visually; `spacing` can be lowered further if seams persist (export cost is now O(log n)). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…esult screen (#301) ## Summary Adds a reusable **FeatureTree viewer** to `BrightroomParametric` (as a debug component) and uses it on the SwiftUI Demo's **PhotosCrop export result screen** to show the parametric `EditingDocument` that produced the render. The viewer is expressed with `List` + `DisclosureGroup`, as requested. ## BrightroomParametric New `Sources/BrightroomParametric/Debug/FeatureTreeView.swift`, guarded by `#if canImport(SwiftUI)` so the otherwise SwiftUI-free module keeps building. - `FeatureTreeView(document:)` — standalone `List`-based screen. - `FeatureTreeOutline(document:)` — embeddable rows for an existing `List`/`Form`/`Section` (used by the demo). - `FeatureTreeDescriptor` / `FeatureTreeNode` — the tree builder and display model (reusable for tests/other hosts). Design: - Structural containers (`EffectPipelineFeature`, `PresetFeature`, `LocalAdjustmentFeature`, `MaskTree`/`MaskNode`) expand into child nodes explicitly. - Scalar parameters are read via `Mirror` reflection, so **new leaf features show up automatically** without touching the viewer. `CGRect` / `ParametricRGBAColor` / `GaussianBlurRadius` / `QuarterTurn` etc. get dedicated formatting. - Each node shows an icon, title, enabled state, a one-line summary, and (when expanded) `name`(left)/monospaced `value`(right) parameter rows plus child nodes. Top level is expanded by default; nested structure stays collapsed. ## Demo - `RenderedResultView` is now a `List` with a "Result" section (image + metadata) and a "Feature Tree" section (`FeatureTreeOutline`). - `DemoPhotosCropView` snapshots `stack.loadedState?.currentEdit.document` on done and passes it to the result screen. ## Verification Built `SwiftUIDemo` (Xcode 27) and verified on iPhone 17 Pro Simulator: opened PhotosCrop, applied the *Vivid* preset + blur brush strokes, tapped Done, and confirmed the result screen shows the Feature Tree: - **Effects** (`1 effect`) → child **Preset (Vivid)** - **Local Adjustment** (`alpha`) → **Mask → Brush** (Strokes 3 / Stamps 61) - **Crop** (`Crop Rect (…) · Rotation 0°`) with parameter rows DisclosureGroup expand/collapse, icons, layout, and long-ID wrapping all render correctly. No visual bugs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary Fixes PhotosCrop rendering as a black canvas on iOS Simulator while keeping the 10-bit Display-P3 drawable path on real devices. ## Root Cause The editing canvas drawable pixel format was changed to `.bgr10a2Unorm` for the wide-gamut Display-P3 canvas path. That works on device, but recent iOS Simulator runtimes can create the MTKView and still present the drawable as black. ## Changes - Use `.bgra8Unorm` for the editing canvas drawable on Simulator builds. - Keep `.bgr10a2Unorm` for device builds. - Add a regression test that locks the intended platform-specific drawable format while preserving the existing wide-gamut color math tests. ## Verification - Reproduced the black PhotosCrop canvas in SwiftUIDemo on iPhone 17 Pro, iOS 26.5 Simulator before the fix. - Verified the same PhotosCrop Horizontal screen renders the image after the fix. - Ran `BrightroomEngineTests/EditingCanvasColorContractTests`: 3 passed. - Built and launched `SwiftUIDemo` successfully on the same Simulator.
## Summary - Move parametric brush Core Image kernels from copied `.metal.txt` source to build-compiled `.metal` sources. - Move the live brush-mask render shader into BrightroomParametric so it is compiled into the same `default.metallib`. - Share brush falloff through `BrushStampFalloff.metalh`, loaded by both the Core Image kernel and live Metal render shader. - Load kernels via `CIColorKernel(functionName:fromMetalLibraryData:)` and live render functions via `MTLDevice.makeLibrary(URL:)`. ## Why The previous resource-copy/runtime-compile setup existed to avoid missing `brushStampAlpha` during Metal compilation. Now the shared falloff is a real Metal header included by both compiled sources, so the package can ship compiled Metal instead of text resources. ## Validation - `BrightroomUI` simulator build via XcodeBuildMCP: succeeded. - `BrightroomEngineTests` targeted tests via XcodeBuildMCP: 4 passed. - `BrushStampFalloffTests` - `BrushMaskRasterizerParityTests` - Confirmed build output contains `Brightroom_BrightroomParametric.bundle/default.metallib` and build log runs `CompileMetalFile` + `MetalLink` for the new `.metal` sources.
## Summary - Add a finite maximum crop zoom based on a minimum authored crop-output side length. - Clamp programmatic crop zoom to the scroll view's configured zoom range before calculating content offset. - Add focused tests for the crop zoom policy on large and small images. ## Root Cause Crop editing allowed an unbounded zoom scale, so users could author a final crop output that was only a few pixels wide or tall. Blur masking uses that final crop output as the tool canvas; once the crop output became extremely small, the tool surface entered huge zoom-scale space and viewport-sized brushes resolved into sub-pixel image-space strokes. ## Validation - `BrightroomEngineTests/CropEditingZoomScaleTests` via XcodeBuildMCP: 2 tests passed - `git diff --check`
## What Each filter in the PhotosCrop **Filters** strip now renders a live thumbnail of the photo with that filter applied (Photos/Instagram-style), replacing the text-only pills — including an "Original" swatch. ## How - **New `PhotosCropFilterThumbnail.swift`** — a `@concurrent` single-swatch renderer on a dedicated wide-gamut `CIContext` whose working/output color spaces match the editing canvas (`extendedLinearDisplayP3` → `displayP3`), so a swatch and the live canvas resolve the same colors. It center-square-crops the session thumbnail, never upscales past the source, and threads a source-relative `radiusReferenceExtent` so host-supplied blur/sharpen presets preview at the correct strength. - **Per-cell lazy rendering** — each `PhotosCropFilterChip` renders its own swatch in a `.task` into its `@State`. A `LazyHStack` defers off-screen first renders and retains created chips, so scrolling never re-renders. No shared cache or stringly-typed dictionary. - **Selected chip auto-centers** via `.scrollPosition(id:anchor:)` on selection change (clamps at the ends, so Original / the last filter rest at the edge). - **Selection ring** uses a dark shadow halo so it stays legible on bright swatches (Original / Mono / Noir on light photos). - Swatches preview the **filter only** — not the user's manual Adjust edits, blur mask, or current crop — matching the Photos/Instagram strip. ## Notes - Uses the iOS 17 `scrollPosition(id:anchor:)` modifier (not deprecated). The iOS 18 `ScrollPosition` struct would need `if #available` since the deployment floor is iOS 17. ## Verification - BrightroomUI builds clean in Swift 6 language mode. - Verified on the iOS Simulator: all swatches render distinctly (Mono/Noir desaturated, Warm/Cool shifted, Fade washed, Vivid/Dramatic as expected), off-screen chips render lazily when scrolled into view, the selection ring is visible on bright swatches, and selecting a chip scrolls it to center. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary `EditingCanvasMode.preview` was a dead compatibility alias of `.renderedEditPreview`. This removes it and collapses its `switch` arms into `.renderedEditPreview`. ## Why An audit of all four `EditingCanvasMode` cases found that `.preview`: - Was documented in-source as a *"Compatibility spelling for `renderedEditPreview`"* and was introduced in the **same** commit as `.renderedEditPreview` (it is not an older, migrated-away-from name). - Is **never constructed anywhere** — production, demo, or tests. `git grep` for any producer (`= .preview`, `return .preview`, `mode: .preview`, `EditingCanvasMode.preview`) returns nothing. - Appears **only** as a passive `switch` co-arm, always paired with `.renderedEditPreview` and handled identically, in every `switch` over the enum: - `EditingCanvasPublicTypes.swift` — `localEffect`, `activeLocalEffect`, `defaultInteractionMode` - `EditingCanvasRenderImageFactory.swift` — `makeRenderImages`, `makeCropOutputRenderImages` So the case carried no distinct behavior and could never be reached. ## Behavioral impact None. No first-party code path ever produced `.preview`, and it was rendered byte-for-byte identically to `.renderedEditPreview`. The other three cases (`.viewportBase`, `.localAdjustment`, `.renderedEditPreview`) are unchanged and remain in use. ## API note This removes a `public` enum case, so it is technically source-breaking for any external caller that explicitly referenced `EditingCanvasMode.preview`. There are no such references inside this repository. ## Verification - `git grep` confirms zero remaining `.preview` references in `Sources/` and `Dev/`. - `xcodebuild -scheme BrightroomUI -destination 'generic/platform=iOS Simulator'` → **BUILD SUCCEEDED** (Swift 6 mode). ## Follow-up (not in this PR) `.renderedEditPreview` itself is only selected by `CropView.CanvasRenderPlan` when 2+ local-adjustment layers are simultaneously active — a state the built-in PhotosCrop UI cannot currently produce. It (and `CanvasRenderPlan`, already marked `// TODO: Consider to remove this`) is a candidate for removal/replacement once the live canvas composites multiple local-adjustment layers; left out of this PR to keep the change a pure, behavior-preserving cleanup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Problem In Crop mode, holding a pinch still (fingers down, no movement) would snap the scroll view back to the recorded crop after ~0.8s, making it look like the crop was committed mid-gesture. ## Cause `cropSurface.onDidZoom` schedules `updateCropLayout()` on a trailing `debounce` (interval 0.8s). `updateCropLayout()` → `updateScrollContainerView()` → `customZoom(to: crop.zoomExtent())` resets the scroll view to `state.proposedCrop`, which is **not** updated mid-pinch (`record()` only runs on settle). While the user moves their fingers, continuous `scrollViewDidZoom` events keep resetting the 0.8s debounce. The moment the user **holds still with fingers down**, events stop firing and the debounce fires mid-gesture, snapping the view. The sibling `onDidScroll` debounce already guards against this for drags with an `isTracking == false` check, but `onDidZoom` had no equivalent guard — and both closures share the same `debounce` instance, so whether the snap happens depends on which event was scheduled last. ## Fix Guard the `onDidZoom` settle layout with `cropSurface.isInteractiveZoomGestureActive == false` (pinch recognizer not in `.began`/`.changed`), deferring the reflow until the pinch actually ends. The crop **recording** path (`record()` via the `scrollViewSettleDebounce` → `didSettleScrollViewAdjustment`) already checks `isContentOffsetResting` (`isZooming == false`, etc.), so commit semantics are unchanged — this only stops the premature layout snap. ## Verification - `BrightroomUI` builds (iPhone 17 Pro, iOS 26.5). - Confirmed in SwiftUIDemo PhotosCrop: pinching and holding still no longer auto-snaps the crop. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The crop surface shows its pixels on a Metal canvas that is repositioned each frame to follow the transparent zooming platter, rather than being zoomed by the scroll view directly, so the bounce-back animation depends on per-frame following of the scroll view's layers. Two issues broke it: - makeCropDisplayViewport read model-layer values whenever the zoom was "interaction active", lumping the active pinch together with the post-release bounce-back. During the bounce-back UIKit animates the presentation layer while the model has already jumped to the clamped value, so model reads snapped the canvas. Read presentation layers during the bounce-back (new isInteractiveZoomDriving) and keep model reads only while the pinch actively drives the zoom. - updateCurrentEditingStackDisplay's document-follow guard checked isZooming but not isZoomBouncing/isDecelerating, so a SwiftUI re-render during the bounce could re-load the document crop and customZoom(animated: false) back, cancelling the bounce. Treat bounce/decelerate as live interaction, matching isContentOffsetResting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This reverts commit e45c4fc.
…ent bake (#308) ## Summary Two performance fixes for slow CropView rotation, found and verified with on-device Instruments Time Profiler traces. Both are behind pixel-equivalence tests (the editing canvas renders black on the Simulator, so correctness is guarded via CIContext read-back, and the felt win was confirmed on device). ### 1. Make the editing source GPU-resident (adaptive bit depth) The editing source was a CPU-backed `CIImage(cgImage:)`, so Core Image re-uploaded its bitmap CPU→GPU on **every** canvas render. During rotation the viewport cache invalidates every frame, so the trace showed `CIMetalTextureSetBytes` / `replaceRegion:withBytes:` consuming ~40% of all main-thread time. `EditingSourcePreparation.makeGPUResidentSource` uploads the oriented source into a persistent private `MTLTexture` once at load (background queue) and returns a texture-backed `CIImage`. The texture depth **adapts to the source**: `rgba8Unorm` for an 8-bit source (the common case — lossless, half the memory), `rgba16Float` for >8-bit / HDR. This re-does what #297 removed, but avoids that PR's downsides (`MTKTextureLoader` 8-bit quantization / no 16-bit / y-flip) by rendering through a `CIContext` in the source's own color space — the `CIContext.render(to:)` / `CIImage(mtlTexture:)` round-trip is flip-free. `EditingImageWarmUp` is deleted (building the texture warms the pipeline). ### 2. Bake the local-effect layer once per generation With the upload gone, the trace's remaining per-frame cost was Core Image re-evaluating the blur-heavy `adjusted` graph every frame (`create_intermediate` / `GetSurfaceFromCache` — IOSurface allocation under a barrier lock). The effect content is invariant during a gesture; only the viewport moves. `EditingCanvasContentBake.bake` renders the `adjusted` layer into a capped (2560) texture once per render-images generation; the composite path resamples it per frame. The cache is invalidated only on `setRenderImages` / `setViewportCachedSourceEnabled`, never on viewport changes. `base` (global pointwise effects on the now-GPU-resident source) stays a live graph — cheap to re-evaluate, and not baking it halves the held texture memory. ## Measured (device, normalized ms per wall-second) | Main-thread cost | before | after | | --- | --- | --- | | Canvas draw (`MTKView.draw`) | 102 | 26 | | CI graph eval (`tile_node_graph`) | 70 | 3 | | IOSurface alloc + barrier lock | 58 | 0 | | Per-frame source upload | (eliminated in step 1) | 0 | `CA::Transaction::commit` dropped 158 → 90. The new dominant rotation cost is UIKit layout (`updateUIViewController` → `updateCropLayout`), tracked as a follow-up. ## Memory Adaptive source depth + baking only `adjusted` keep the footprint bounded. For an 8-bit source: plain crop/rotate holds just the ~26MB source texture (≈ the pre-change CGImage footprint); a blur-mask gesture adds one ~52MB `adjusted` bake. The `adjusted` bake stays `rgba16Float` deliberately — it is the extended-linear-Display-P3 intermediate, where 8-bit linear bands and can't carry exposure overshoot / wide gamut. ## Test plan - [x] `xcodebuild -scheme BrightroomUI` builds. - [x] `EditingSourceTextureTests` — texture-backed source is pixel-equivalent to `CIImage(cgImage:).oriented()` across 8 orientations × {8,16}-bit (16 cases). - [x] `EditingCanvasContentBakeTests` — bake is a faithful drop-in (offset extent, asymmetric content, within float tolerance; cap respected). - [x] Regression-green: ColorContract, CropSurfaceBlurRenderPath, CommittedMaskParametricParity, LocalAdjustmentRendering/MaskOrientation, EditingPreviewExportParity, MaskedPreviewExportScaleConsistency, RendererOrientation. - [ ] Device re-trace to confirm keeping `base` un-baked did not re-introduce per-frame cost (expected fine — pointwise on a GPU texture). ## Follow-ups (docs/editing-engine-todo.md item 7) - UIKit per-frame layout during the angle drag is now the dominant rotation cost. - Remaining bake-memory levers (resolution cap, confirmed-SDR formats) if needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Summary - Make the crop Metal canvas follow UIScrollView zoom bounce-back by sampling presentation-layer geometry after the pinch ends. - Apply the same presentation-layer viewport tracking to the Tool/Mask surface so the mask canvas follows min-zoom shrink/bounce without drifting toward the top-left. - Defer document-crop reload while scroll/zoom settling is still active, avoiding a non-animated `customZoom` snap during bounce-back. ## Root Cause The viewport display link was already running, but `makeCropDisplayViewport()` disabled presentation-layer reads for the full zoom interaction, including the post-release bounce. During bounce-back UIKit animates the presentation layer while model values are already clamped, so the canvas snapped instead of following the visible image. Tool/Mask mode also computed its viewport from model `contentOffset`/`contentInset`/`zoomScale`, which lost the presentation-time centering during min-zoom shrink. ## Validation - `git diff --check` - Xcode 27 `SwiftUIDemo` launched on `iPhone 17 Pro (27.0)` simulator from the cwd worktree; build completed with no errors. Launch session: `77a4d4780`.
## Summary Fixes a regression where, after changing the zoom in **Crop** mode and switching to **Blur** (tool/mask) mode, the crop output rendered **small and pinned to the top-left**. Toggling the Crop/Blur tabs again corrected it; zooming the other way produced the inverse misplacement. ## Root cause `makeToolCropDisplayViewport` derives the Metal canvas placement from **presentation-layer** conversions (`currentLayerRect(... usesPresentationLayers:)`), gated only on `toolSurface.isInteractiveZoomGestureActive == false`. This was introduced in #309 to make the mask canvas follow the post-release zoom bounce. The Crop→Blur switch applies the tool viewport **once, synchronously**, in the same runloop turn that `updateToolScrollGeometry` just reconfigured the freshly-un-hidden tool scroll view (non-animated `setZoomScale`, `centerContentInViewport`, etc.): `setFeatureFocus` → `applySurfaceMode(syncsToolViewportFromCrop: true)` → `updateToolScrollGeometry` (model set synchronously) → `updateToolCropDisplayViewport()` (one-shot, no display link). At that instant no pinch is active, so `usesPresentationLayers == true`, but Core Animation has not yet committed the new geometry to the presentation layers — they still hold the **previous** Blur session's geometry. The one-shot samples that stale transform → small + top-left placement, and because no display link runs afterward, the stale frame sticks until the next mode switch (the "switch back and forth fixes it" symptom). `#308` (GPU-resident source / per-generation content bake), originally suspected, is **not** involved: it never touches `CropView.swift`, and `EditingCanvasContentBake` maps the bake back to the original `extent`, so it changes layer *content*, never viewport *placement*. ## Fix Presentation-layer reads are only meaningful while the viewport display link is actively re-sampling an in-flight bounce. Gate them on whether that link is running: ```swift let usesPresentationLayers = toolSurface.viewportRendering.isRunning && toolSurface.isInteractiveZoomGestureActive == false ``` This flips **only** the link-idle one-shot paths (i.e. the mode switch) back to the model layers, which the synchronous reconfigure already made authoritative. Every display-link-driven path (interactive pinch, post-release bounce, deceleration) is unchanged, so #309's bounce tracking still works. ## Scope / notes - Scoped to the **tool** path only. `makeCropDisplayViewport` has the same presentation-read pattern, but the crop path is not a reported repro and its animator-driven rotation computes the viewport inside a `UIViewPropertyAnimator` block (where presentation reads are intended), so it is intentionally left untouched. - The tool path's animator case early-returns (the tool surface is hidden in Crop mode), so the gate is safe there. ## Test plan - [x] `xcodebuild -scheme BrightroomUI -destination 'platform=iOS Simulator,name=iPhone 17 Pro'` builds. - [x] Verified on device: Crop zoom change → switch to Blur now displays the crop output correctly (no small/top-left misplacement, no need to toggle tabs). - [ ] Editing canvas renders black on the Simulator, so visual confirmation is device-only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Summary - Add Blur to the PhotosCrop Adjust parameter list. - Write Blur edits into the global-effects node as `GaussianBlurFeature(value:)`. - Add focused tests for blur insertion, neutral removal, and PhotosCrop effect ordering. ## Why `GaussianBlurFeature` already existed in the parametric layer and PhotosCrop effect ordering, but PhotosCrop's Adjust UI did not expose a global Blur parameter. This makes Gaussian blur available as a first-class Feature adjustment in that UI. ## Validation - BrightroomUI simulator build succeeded via XcodeBuildMCP. - `BrightroomEngineTests/PhotosCropAdjustmentParameterTests` passed: 3 tests.
## Summary - Introduce `PhotosCropEditingModel` as the PhotosCrop policy layer over the generic FeatureTree document. - Add `CropViewDocument` so CropView edits through a document boundary instead of depending on `EditingStack` directly. - Move undo/redo checkpoint storage behind `EditingHistory` and expose clearer stack APIs: `commitCurrentEditIfNeeded`, `undo`, `redo`, `revertCurrentEdit`, and `removeAllHistory`. - Remove legacy crop-specific stack helpers and update demos, docs, and render tests to use FeatureTree-oriented mutation paths. ## Validation - `BrightroomEngineTests`: 152 passed on iPhone 17 simulator. - `BrightroomUI` simulator build: succeeded. - `SwiftUIDemo` simulator build: succeeded. - `git diff --check`: passed. Note: `SwiftUIDemo` still reports existing iOS 17 `onChange(of:perform:)` deprecation warnings unrelated to this change.
The Metal canvas tracked the scroll view by sampling presentation layers from a display link, which is always a frame out of phase with the post-release zoom bounce (the bounce is a CA presentation animation the model has already settled past, so there is no per-frame hook to chase). Instead, at bounce start reparent the canvas into the zooming view with a 1/zoomScale compensating transform so UIKit's own bounce spring carries it in exact phase; the display link only polls for settle. Render the ride target from settled model geometry and reparent without animation so the handoff is flash-free. Also fixes two interaction bugs surfaced by this path: - 2→1 finger: the onDidZoom settle debounce ran updateCropLayout()'s customZoom against the not-yet-recorded proposedCrop while one finger still panned, reverting the zoom. Guard it on isTracking==false to match the onDidScroll debounce (and the comment's stated intent). - Tool surface right-edge clip during the bounce: the tool viewport has no overscan, so at fit the content sits flush against the canvas edge and the offset spring shifts it past the edge. Add a symmetric ride overscan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
During a pinch on the Tool surface the content tracked slightly off and snapped to the reconciled position at release. Root cause: the tool rewrote contentSize and contentInset on every scrollViewDidZoom tick, so UIKit derived each interactive tick's contentOffset against one-tick-stale geometry. The crop surface never mutates its scroll model mid-gesture, which is why it was immune. Align the tool with the crop surface's design: the centering inset is now a static value computed once per layout in updateToolScrollGeometry from the fit geometry, with the fitted crop-output box playing the role of the crop surface's guide (the crop frame is the tool viewport, per docs/vision-of-editing.md). The fit box shares the content's aspect ratio, so at minimum zoom the valid offset range degenerates to the centered point in both axes and the content rests centered with no per-tick correction. Zoomed in, panning clamps at the fit box edges just as crop clamps at the guide; a below-fit pinch rubber-bands and bounces back onto the same centered point on a constant scroll model. centerContentInViewport and synchronizeZoomedContentSize are deleted; zoom ticks now run only the canvas chase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # Package.swift
Scroll model: - Center the tool content with a letterbox inset reconciled only at layout passes and quiet gesture boundaries. settleToolScrollInsetIfNeeded also repairs UIKit's stale contentSize after a below-minimum pinch (measured: contentSize kept the pre-pinch zoomed value while zoomScale rested at minimum) and glides any out-of-range offset back into the covering range. Fixes the photo coming to rest displaced or outside the viewport after two-finger gestures. - Keep alwaysBounce disabled (a two-finger parallel drag at fit no longer carries the photo away) but leave `bounces` at UIKit's default so a below-minimum pinch shrinks around the pinch instead of pinning to the fit box's top-left corner. - Honour isZoomEnabled == false on the tool surface, publish display state before setZoomScale (which emits scrollViewDidZoom synchronously), hoist the degenerate-frame guard above all writes, and make the zoom-view sizing well-defined (reset transform, single frame write). Canvas viewport: - makeToolCropDisplayViewport now always derives from model layer geometry. The previous viewportRendering.isRunning-gated presentation sampling flipped its geometry source on the tick after a pinch ended, producing a measured one-frame flash (content rendered small and shifted for exactly one frame, once per zoom-in pinch; zero after this change). Pinches read live model values per delegate tick, the zoom bounce is carried by the bounce ride, and one-shot applies run against the synchronously reconfigured model. - Mirror the crop surface's interactive-pinch handling: apply synchronously from delegate ticks during a pinch and keep the display-link chase out of the way; drop the tool-only escape hatch in the tick guard. - End the zoom-bounce ride when the tool surface deactivates so a mode switch mid-bounce cannot strand the canvas inside the zooming view. Remove dead code: onDidEndScrollingAnimation plumbing (never fired; no animated scroll API calls exist), CanvasSurface.isZoomBouncing, the presentation-derived viewport zoomScale quotient, no-op layer.mask writes, and the unused clipsToGuide reset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_EditingCanvasView was the development vehicle for the Metal editing canvas (born in #289); CropView embedded _EditingCanvasMTKView directly in #295 and has been the production path since. The standalone view, its SwiftUI wrapper, and their sandbox hosts only existed on v5 and were never released, so nothing external can depend on them. - Delete _EditingCanvasView, SwiftUIEditingCanvasView, and the internal scroll/attachment/viewport views only they used. - Delete the demo hosts (Editing Canvas Crop Probe, Metal Brush Sandbox incl. the NASA variant) and their menu entries. - Trim scaffold-only public types (EditingCanvasInteractionMode, EditingCanvasMetrics, EditingCanvasMode.defaultInteractionMode) and fix doc comments referencing the deleted types. The production path is untouched: EditingCanvasStrokeCommitPipeline, EditingCanvasMode/Brush/StrokeSmoothing, _EditingCanvasMTKView, the drawing recognizer, render factory, and geometry all stay. This also retires the scaffold's known zoom-dependent contentInset defect (written per scrollViewDidZoom tick) along with the component. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ahead of repeatable crops, two behaviors that held only because the canonical document puts the final crop last are now declared semantics (docs/vision-of-editing.md "Decided Semantics"): - Mask coordinates are current-Feature-domain: a mask is authored in and evaluated against the input domain of its owning Feature. Masks are frame-anchored, not content-anchored — an upstream crop change shifts the content under the mask and no restoration is attempted. The compiler's existing rasterization into the current base extent is the correct implementation of this, so no code change was needed. - Value-form radii (Gaussian blur, sharpen, unsharp mask) resolve against the render pass's CHAIN-ENTRY extent. FeatureGraphCompiler now resolves the reference once at entry instead of letting each recipe fall back to whatever intermediate it receives, so a mid-chain crop cannot re-base the radius and editing a crop can never retroactively change a committed effect's strength. The basis is per render pass, so the downsampled preview and the full-resolution export stay consistent. Wire the reference explicitly through every evaluation path: the engine preview reduction (engineRender grew a radiusReferenceExtent parameter) and all EditingCanvasRenderImageFactory sites — including two that were silently wrong for any proportional radius (effects applied to the viewport slice and to the cropped output resolved against those intermediates instead of the chain entry). New test pins the compiler-level contract: [crop, blur(value)] equals crop followed by the entry-diagonal absolute radius, and does not equal the cropped-extent radius. Geometry-anchored effects (vignette) stay current-domain by design; only the radius basis is entry-based. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
) ## Problem In `CropView.layoutSubviews`, the outside-overlay container was positioned with: ```swift guideOutsideContainerView.center = center ``` `guideOutsideContainerView` is added via `addSubview(...)`, so it is a direct subview of `CropView` and its `center` is expressed in `CropView`'s own bounds space. But `self.center` is expressed in the **superview's** coordinate space. The two are being mixed. ## Impact Behavior-neutral today. Every in-repo host lays `CropView` out at origin `.zero`, where `bounds.midX == center.x` and `bounds.midY == center.y`, so the two expressions coincide and nothing moves. It only changes — correctly — for a host that positions `CropView` at a non-zero origin, e.g. inlined below a header. Such a host would today see the entire outside-overlay plane displaced by that origin. ## Fix Use the view's own bounds center, which is the pattern already established a few lines below in the same file for `surfaceHost.platterView`: ```swift surfaceHost.platterView.center = .init(x: self.bounds.midX, y: self.bounds.midY) ``` While in the same block, the hardcoded `UIScreen.main.bounds` sizing is replaced with `window?.screen.bounds ?? UIScreen.main.bounds`, so the overlay is sized from the screen the view actually lives on. This mirrors the existing `window?.screen.scale ?? UIScreen.main.scale` fallback used elsewhere in this file. ## Scope Strictly limited to this one block in `layoutSubviews` — 6 insertions, 3 deletions in a single file. CropView's scroll behavior is untouched, and no other `UIScreen.main` site in the file was changed. ## Verification `xcodebuild -project Dev/Brightroom.xcodeproj -scheme SwiftUIDemo -destination 'generic/platform=iOS Simulator' build` → **BUILD SUCCEEDED** 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
) Two independent correctness fixes in the image-loading path. ## 1. `imageProviderSubscription` data race (`EditingStack.swift`) **What.** `start()` wrote `imageProviderSubscription` on the caller's thread; the load completion nils the same unsynchronized `private var` from `backgroundQueue`. The subscription is now handed off through the serial background queue, and the publish block is guarded with `loadedState == nil`. **Mechanism.** `start()` is documented as callable from a background thread. When the `ImageProvider` is already loaded, the graph-tracking `onChange` fires *synchronously* inside `withGraphTracking` — before the assignment line is reached — and enqueues `handleImageLoaded` on `backgroundQueue`. So two threads write the same variable with no happens-before edge between them. If the nil-out won that race, the assignment would re-install the observation instead of releasing it, and a later provider emission could rebuild `loadedState` on top of edits already in flight. **Why this shape.** Hopping through the serial queue gives FIFO ordering: the install is enqueued before the completion's nil-out can run, so the nil-out always lands last and the subscription is released rather than re-installed. The queue becomes the single owner of the variable's lifecycle, which is what the added comment states. The `loadedState == nil` guard closes the same hole from the other side: a duplicate emission can never rebuild loaded state over in-flight edits, independently of subscription timing. The practical window here is narrow — this is a memory-model correctness fix rather than a response to a reproduced user-facing failure. The GCD flow is deliberately left as-is; restructuring it into Tasks is planned separately. ## 2. Remote metadata failure was misclassified (`ImageProvider.swift`) This one **is** user-visible. In `init(editableRemoteURLRequest:)`, a `makeImageMetadata` failure appended to `loadingNonFatalErrors` and returned early, skipping the `editableImage` assignment. `loadedImage` then stayed nil forever: the editor spun in its loading state indefinitely while hosts observing `loadingFatalErrors` were never notified — a silent hang with no way to surface an error. The two sibling guards immediately above it (`failedToCreateCGDataProvider`, `failedToCreateCGImageSource`) already use `loadingFatalErrors`, and the local-file initializer throws `failedToGetImageSize` for the same condition. Changing this one append to `loadingFatalErrors` makes remote-load failures surface to the host instead of spinning forever. ## Verification - `xcodebuild -scheme SwiftUIDemo -destination 'generic/platform=iOS Simulator' build` — **BUILD SUCCEEDED** - `BrightroomEngineTests` full suite on a clean iPhone 17 Pro simulator, `-parallel-testing-enabled NO` — **153 tests in 36 suites passed**, no failures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…window (#320) ## Problem `_EditingCanvasMTKView.startLiveDisplayLinkIfNeeded` creates a `CADisplayLink` targeting `self` and adds it to the main run loop: ```swift let displayLink = CADisplayLink(target: self, selector: #selector(liveDisplayLinkDidTick(_:))) displayLink.add(to: .main, forMode: .common) ``` The run loop retains the link and the link retains its target, so while a stroke is live the canvas view is held alive by that chain. The existing `isolated deinit { stopLiveDisplayLink() }` cannot save it — deinit is exactly what the cycle prevents from running. `CropView.removeCanvasView()` tears the canvas down without cancelling the stroke, and the drawing gesture recognizer lives on the container rather than on the MTKView, so teardown can land mid-stroke (a canvas-size change, or a focus switch while a touch is down). The result is an orphaned MTKView plus its `rgba16Float` texture caches, with the display link still firing every frame for the life of the run loop. The per-frame work is mostly a no-op, but both the callback and the leaked textures persist. ## Fix Handle it inside the MTKView: on `didMoveToWindow` with `window == nil`, cancel the active stroke and stop the live display link, so `removeFromSuperview` always severs the retain chain and `deinit` becomes reachable. - Reuses the existing `cancelActiveStroke()` path that the gesture flow's `cancelStroke()` already calls — no parallel cancel path. - `stopLiveDisplayLink()` is restated after it so the lifecycle guarantee of this branch does not rest on stroke bookkeeping being refactored carefully later. The call is a no-op once the link is already nil. - The `window != nil` branch (colorspace re-assert, preferred frame-rate update) is unchanged, so being *added* to a window behaves exactly as before. - Direct calls, no actor hops: the class inherits `MTKView`'s main-actor isolation and `didMoveToWindow` is already on the main thread. ## Stroke survival across a detach Checked, as the fix would otherwise be a behavior change: nothing depends on a stroke outliving a brief window detach. - `CropView.removeCanvasView()` does `canvasView?.removeFromSuperview(); canvasView = nil`, and `ensureCanvasView` constructs a brand-new `_EditingCanvasMTKView` — a detach is always terminal for a given canvas instance, never a detach/re-attach round trip. - The only other `window` references in the file are `window?.screen` frame-rate reads. So no gating was needed beyond the `window == nil` check. `setNeedsDisplay()` inside `cancelActiveStroke()` is inert during teardown: the view is configured `enableSetNeedsDisplay = true` / `isPaused = true`, an off-window layer never displays, and `renderViewportImage()` guards on `currentDrawable` anyway. ## Scope Confined to `EditingCanvasMTKView.swift` (+16 lines, lifecycle path only). `CropView` is untouched — its surfaces are behaviorally frozen. No hot render path is modified, given this file's history of one-frame-flash regressions. ## Verification - `SwiftUIDemo` builds for `generic/platform=iOS Simulator`: **BUILD SUCCEEDED** - `BrightroomEngineTests` on a clean private simulator (iPhone 17 Pro, iOS 27.0, `-parallel-testing-enabled NO`): **153 tests in 36 suites passed**, including the canvas-related suites `EditingCanvasColorContractTests`, `EditingCanvasContentBakeTests`, and `BrushMaskRasterizerParityTests`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Four documentation contracts in this repo state the opposite of what the code does, and two sliders are unlabeled for VoiceOver. This corrects the comments and adds the labels. **No rendering, resize, or evaluation behavior changes.** ## 1. Editing-source resolution is a short-side target, not a longest-side cap `ImageTool.makeResizedCGImage(from:maxPixelSizeHint:)` computes `largestSide * hint / smallestSide`, and returns early without downsampling when `smallestSide < hint`. So with the engine's 2560: | source | loaded as | | --- | --- | | 4032x3024 | 3413x2560 | | 6000x2000 | 6000x2000 (untouched) | Three docs claimed a longest-side cap, and a fourth (`EditingCanvasImageProcessing.contentBakeMaxPixelSize`) declared itself "visually lossless" on the strength of a `MUST stay equal` parity with it. That bake cap *is* a real longest-side cap, so the two resolutions coincide only for square content; for anything else the bake resamples somewhat below the editing source. The comments now describe the actual relation at all four sites (`ImageTool`, `EditingStack`, `ImageSource`, `EditingCanvasGeometry`). Whether this *should* be a true longest-side cap - which would bound the memory of extreme aspect ratios but reduce preview detail on ordinary photos - is an owner decision, so it is flagged as an open question in the corrected comment rather than changed silently. ## 2. Radius basis docs and a named accessor `SharpenFeature.radius`, `UnsharpMaskFeature.radius`, and `GaussianBlurRadius.editingStackFilterValue` still said "resolved against the current extent", the pre-decision behavior. fea9619 pinned the opposite: a proportional radius resolves against the render pass's chain-entry extent, so a mid-chain crop can never re-base a committed effect. The docs now say that. The `context.radiusReferenceExtent ?? image.extent` motif repeated by the three recipes becomes `FeatureEvaluationContext.radiusBasis(for:)`, whose doc carries the subtle part - the fallback is correct *only* when the input is the chain entry - so a fourth diagonal-based effect cannot copy the `??` without meeting the rule. The accessor's body is that same expression, so the change is behavior-neutral by construction. ## 3. HighlightShadowTintFeature docs overstated the recipe The properties were documented as "composited over highlight/shadow regions", but the recipe source-over composites both colors across the entire extent with no luminance or region weighting. This is faithful parity with the legacy `FilterHighlightShadowTint` (verified against its pre-parametric source), so the recipe is deliberately left alone and the comment now states the behavior and why it must not be "fixed" - stored documents render against it. ## 4. Accessibility labels for two sliders `BrightroomSteppedSliderControl` is an adjustable accessibility element with an `accessibilityValue` but no label of its own. The rotation slider gets one from its host; the Adjust-mode value slider and the blur brush-size slider did not, so VoiceOver announced a bare number - and in Adjust mode without saying which of the nine parameters it moves. Both now follow the local rotation-slider pattern, with the Adjust slider labeled by `selection.title`. ## Verification `xcodebuild -scheme SwiftUIDemo -destination 'generic/platform=iOS Simulator'` - **BUILD SUCCEEDED**. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#322) Three related defects in `SwiftUICropView`'s SwiftUI update contract, all confined to `Sources/BrightroomUI/Shared/Components/Crop/SwiftUICropView.swift`. `CropView.swift` is untouched. ## 1. State written during view update `updateUIView` wrapped the binding applications in the coordinator's `applySwiftUIInputs` guard, but called `setMaskingBrush`, `setCanvasStrokeSmoothing`, `setFeatureFocus` and `updateCurrentDocumentDisplay` **outside** it. Two of those reach `CropView.emitStateSnapshot()` synchronously: - `setFeatureFocus` ends with `updateCurrentDocumentDisplay()` - `updateCurrentDocumentDisplay` -> `load(crop:)` -> `setProposedCrop` -> `updateProposedCrop` -> `emitStateSnapshot()` The emitted snapshot lands in the coordinator's `handleStateSnapshot`. With `isApplyingSwiftUIInputs` false, it takes the immediate `syncInputs(snapshot)` branch and writes `rotationInput`, `adjustmentAngleInput` and `croppingAspectRatioInput` — SwiftUI bindings — from inside `updateUIView`. That is the classic **"Modifying state during view update; this will cause undefined behavior"**, triggered on any focus change or `imageSize` change. All four calls now run inside the existing guard, so the guard's deferral path (`pendingInputSyncSnapshot` + a follow-up main-actor hop) applies. `makeUIView` already wrapped `loadCurrentDocumentState` in the same guard, which is the contract this restores. `setMaskingBrush` and `setCanvasStrokeSmoothing` cannot emit today. They moved inside anyway so the rule is uniform — "everything that pushes SwiftUI inputs into CropView goes through the guard" — and a future emitting setter is safe by default rather than by accident. ## 2. A new `CropViewDocument` on every body evaluation Both public initializers did `self.document = CropViewDocument(editingStack: editingStack)`. `SwiftUICropView` is a struct, so every body evaluation of the host built a new document — each owning a fresh `EditingCanvasStrokeCommitPipeline` whose `layerID` is session state. The representable then ignored it: `makeUIView` captured document N and `updateUIView` never re-binds (`CropView` stores `private let document`). So the mounted canvas kept the first document while `body`'s `document.snapshot` check and `.onAppear { document.start() }` operated on the newest one. The document is now resolved through a private `DocumentSource` enum and a `@State`-held box that creates it once and reuses it for the view's lifetime, recreating only when the `EditingStack` identity changes. Mutating a class held in `@State` does not invalidate the view, so populating it from `body` is not itself a state write during update. The representable's identity is also tied to the document (`.id(ObjectIdentifier(document))`), so if the host ever swaps `EditingStack` the canvas is rebuilt against the new document instead of silently staying bound to the old one. With a stable document — the normal case, and always the case for PhotosCrop — this is a no-op. **Public API is source-compatible.** No initializer signature changed. Rather than the reviewer's suggested route of promoting `init(document:)` to public, the stored `private let document` became a `private let documentSource: DocumentSource` with cases for "created from a stack" and "owned by the host". The host-owned path that PhotosCrop uses via `PhotosCropEditingModel.cropViewDocument` behaves exactly as before. PhotosCrop had already worked around this bug by holding the document in its model ("The stable crop-canvas document for this PhotosCrop editing session"), which is the stability contract the public initializers were violating. ## 3. Inputs now documented as fixed-at-creation Three host-settable inputs are consumed only in `makeUIView` and silently never update, while sitting in the same parameter list as inputs that do live-update (`isGuideInteractionEnabled`, `areAnimationsEnabled`, `stateHandler`): - **`contentInset`** — architecturally frozen: `CropView` stores it as `private let contentInset: UIEdgeInsets`, so it cannot be updated without changing `CropView`, which is out of scope here. - **`cropInsideOverlay`** / **`cropOutsideOverlay`** — installed only in `makeUIView`, so a closure capturing changing host state keeps rendering the stale capture. All three are now documented as fixed-at-creation on both public initializers and on the stored properties, and the `makeUIView` call site carries a note on why `updateUIView` deliberately does not re-apply them. Live overlays would need an explicit change token rather than unconditionally re-installing `AnyView` closures every update; that is left for whenever a host actually needs it. ## Verification `cd Dev && xcodebuild -project Brightroom.xcodeproj -scheme SwiftUIDemo -destination 'generic/platform=iOS Simulator' build` — **BUILD SUCCEEDED**, zero warnings. This also compiles `BrightroomUI` and the demo hosts that consume the representable, which is the compile-time check of source compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
## Problem The brush-mask Metal render pipeline was constructed **twice**: | | Live path | Test path | |---|---|---| | Owner | `_EditingCanvasMTKView` (inline, drives live painting) | `BrushMaskMetalRasterizer` (off-screen, single stamp pass) | | Library load | `makeBrushMaskShaderLibrary` | `makeBrushMaskShaderLibrary` | | Pipeline descriptor | `makeBrushMaskPipeline` | `makeBrushMaskPipeline` | | Uniform layout | `EditingCanvasBrushStampUniforms` | `BrushMaskMetalRasterizer.BrushStampUniforms` | | Per-stamp encoding | inline `setVertexBytes`/`setFragmentBytes`/`drawPrimitives` | inline `setVertexBytes`/`setFragmentBytes`/`drawPrimitives` | The two copies were kept equal only by doc comments asserting it — *"exactly as `_EditingCanvasMTKView.makeBrushMaskPipeline` does"*, *"replicated field-for-field"*. That made `BrushMaskRasterizerParityTests` weaker than it looks. The rasterizer exists to prove the **live** pipeline matches the parametric Core Image kernel (`FeatureGraphCompiler.renderMask`), but because it rebuilt its own copy, the test exercised the copy. If the live view's blend op, pixel format, or uniform layout drifted, the test would have stayed green. ## Change Extract one internal `BrushMaskPipeline` factory (new file, same directory) holding the single definition of: - `makeLibrary(device:)` — the BrightroomParametric metallib - `make(device:library:)` / `make(device:)` — the pipeline descriptor and blend state - `StampUniforms` — the uniform layout - `encodeStamp(_:into:)` — the per-stamp bind + draw - `stampUniformsBufferIndex` — named, matching `[[buffer(0)]]` in the shader Both paths now call it. `_EditingCanvasMTKView` keeps its `private typealias BrushStampUniforms`, retargeted at the shared type, so its call sites are otherwise untouched. ## Why share (rather than tolerate the duplication) These two constructions **must** evolve together — it is the entire reason the second copy exists, and a test already enshrines that requirement: - Both target the same shader functions (`brushStampVertex` / `brushStampFragment`) in the same metallib. - The uniform layout is **ABI** against the MSL `BrushStampUniforms` struct in `BrushMaskRenderShader.metal`. A field change on one side silently reinterprets bytes on the other. - The `.max` blend state is what makes the live rasterization agree with the parametric mask's `CIBlendKernel.componentMax` accumulation — the property the parity test pins. Comment-enforced equality across files is exactly the convention that breaks quietly. After this change the parity test covers the live path's construction *by construction*. ## Divergence found between the two copies **None.** The copies were already effect-identical, and every axis was checked before unifying: - Pixel format: `.rgba8Unorm` on both (and both render targets are `.rgba8Unorm`). - Blend state: `isBlendingEnabled = true`; `rgbBlendOperation`/`alphaBlendOperation` `.max`; all four blend factors `.one` on both. - Uniform layout: field-for-field identical (`canvasSize`, `center`, `radius`, `hardness`, `opacity`, `_padding`), and both match the MSL struct. - Encoding: `setVertexBytes` + `setFragmentBytes` at index `0` with `MemoryLayout<...>.stride`, then `drawPrimitives(type: .triangleStrip, vertexStart: 0, vertexCount: 4)` on both. Only the prose of the surrounding comments differed. The shared implementation is the committed path's code verbatim — the path the parity test pins — so behavior is bit-identical. ## Verification - `xcodebuild -scheme SwiftUIDemo -destination 'generic/platform=iOS Simulator'` — **BUILD SUCCEEDED** - `BrightroomEngineTests` on a clean private simulator (iPhone 17 Pro / iOS 27.0), `-parallel-testing-enabled NO` — **153 tests in 36 suites passed**, including the three suites that guard this area: - `BrushMaskRasterizerParityTests` passed - `BrushStampFalloffTests` passed - `CommittedMaskParametricParityTests` passed Diff is confined to the two files that held the duplication plus the new shared file; no other render-path code was touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Three independent bugs in `BrightroomParametric`, found while reviewing the render/export path. Each is small, but each changes what the engine actually produces. They are unrelated to each other and grouped only because they were found together. ## 1. Color-cube filter cache pinned its render input, and aliased by file name `ParametricColorCubeHelper.makeColorCubeFilter` had two defects, fixed together. **It pinned a full-resolution image chain.** The miss path stored the filter in the `NSCache` *and* returned that same instance. The caller (`ColorCubeFeature.apply`) then sets `kCIInputImageKey` on it, so the cache entry held a strong reference to that render's lazy full-resolution `CIImage` recipe until the entry was evicted. The hit path already returned `cached.copy()`, so only the first render of each cube leaked. The miss path now caches the pristine filter and returns a copy too. **It could hand back the wrong cube.** The cache key was the identifier string alone, and both file-based `ColorCubeFeature` initializers default that identifier to `url.lastPathComponent`. Two different LUT files sharing a file name would silently render with whichever cube was cached first — and because the lookup happens before the byte-count precondition, nothing caught the mismatch. The key now carries the cube's shape (`identifier|dimension|byteCount`), so a reused name cannot return a filter built from a differently shaped cube, and a hit now implies the cached data matched the requested dimension. The cube bytes themselves are deliberately **not** hashed: a 64³ float cube is ~4MB and this path runs per frame. A caller that reuses one identifier for two cubes of the *same* shape still has to pass distinct identifiers; the parameter is renamed `cacheKey` -> `identifier` and documented so that contract is visible at the call site. ## 2. `validate()` rejected documents the preview renders `FeatureGraphCompiler.validate(_:)` iterated **all** features and threw `emptyLocalAdjustmentEffectPipeline` for any local adjustment with no enabled effect — including one whose own `isEnabled` is `false`. Evaluation, meanwhile, skips disabled features entirely (`for feature in document.mainTree.features where feature.isEnabled`). The user-visible consequence: toggle a layer off, and export throws for the whole document while the preview keeps rendering it fine. Validation now skips the empty-pipeline check for disabled local adjustments. Duplicate-ID bookkeeping still walks them, so ID collisions are still caught. An **enabled** but empty pipeline still throws — that is a construction mistake worth surfacing, and the existing test covering it is unchanged. ## 3. Force-unwrap in a throwing export `ParametricExportRenderer.render(...)` is `throws`, but the `.memory` path force-unwrapped `ciContext.createCGImage(...)`. That call returns nil on allocation or GPU failure — most likely at exactly the huge sizes this type's own documentation points at `Output.file` to avoid — so the in-memory path crashed where callers expect a thrown error. The sibling file path already throws properly. Adds `RenderingError.failedToCreateCGImage(extent:)` (the enum previously had only `failedToDecodeRenderedFile(URL)`) and replaces the unwrap with a `guard let`. ## Verification - `SwiftUIDemo` builds clean for `generic/platform=iOS Simulator`. - Full `BrightroomEngineTests` suite on a clean iPhone 17 Pro simulator (iOS 27.0, `-parallel-testing-enabled NO`): **154 tests in 36 suites passed**, no failures — so there were no pre-existing failures to triage. - New regression test for finding 2, `disabledLocalAdjustmentWithEmptyPipelinePassesValidation`: a disabled local adjustment with an empty pipeline must compile and render identically to the same document without it. Confirmed it bites — with only the `FeatureGraphCompiler` change reverted, it is the **single** failure in the run (`Caught error: .emptyLocalAdjustmentEffectPipeline(...)`), and the other 153 tests still pass. - Findings 1 and 3 are not covered by new tests: the leak is a lifetime property with no cheap assertion, the aliasing needs two same-named LUT fixtures, and the nil return needs an allocation failure to reproduce. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.