From 790ce1a69d9853196f815537aba96b88ab719d6a Mon Sep 17 00:00:00 2001 From: claudecode_project Date: Sun, 6 Sep 2026 23:35:42 +0800 Subject: [PATCH 1/7] feat: add residual-orientation contract (Task 6, native-rotation-spec) DecodedRgba.appliedOrientation (default 1) and DngOrientingFullDecoder typedef (second typedef, DngFullDecoder stays byte-identical) plus residualExifOrientation({declared, applied}), a total D4-composition helper that lets multi-arm decoder dispatch report what orientation it already applied and Halcyon apply only the residual. --- .../image_pipeline/dng_decode_contract.dart | 29 +++++ .../image_pipeline/exif_orientation.dart | 115 +++++++++++++++++ .../image_pipeline/exif_residual_test.dart | 117 ++++++++++++++++++ 3 files changed, 261 insertions(+) create mode 100644 test/services/image_pipeline/exif_residual_test.dart diff --git a/lib/services/image_pipeline/dng_decode_contract.dart b/lib/services/image_pipeline/dng_decode_contract.dart index 75f03e6..c43756f 100644 --- a/lib/services/image_pipeline/dng_decode_contract.dart +++ b/lib/services/image_pipeline/dng_decode_contract.dart @@ -17,6 +17,7 @@ class DecodedRgba { this.nativeAddress = 0, this.nativeKeepAlive, this.releaseNative, + this.appliedOrientation = 1, }); /// RGBA8 interleaved, length == width * height * 4. @@ -56,6 +57,15 @@ class DecodedRgba { /// retained `PixelPayload` IS this buffer (the identity short-circuit in /// `decodedRgbaToOrientedFullRes`). final void Function()? releaseNative; + + /// The EXIF orientation the DECODER has already applied to [rgba], or 1 + /// when it applied none. Never a request; always a report. Halcyon applies + /// only the RESIDUAL (`residualExifOrientation` in `exif_orientation.dart`), + /// so a decoder that ignores the request, a build whose dylib predates the + /// oriented entry, and the pure-Dart TIFF arm are all correct without a + /// feature flag. Defaults to 1 so every existing construction site + /// (production and every fake decoder in the test suite) is unaffected. + final int appliedOrientation; } /// Decodes a DNG that carries no embedded full-size JPEG preview. @@ -63,6 +73,25 @@ class DecodedRgba { /// Throws on failure; callers treat any throw as "fall back to the old path". typedef DngFullDecoder = Future Function(String path); +/// Orientation-aware sibling of [DngFullDecoder]. +/// +/// A SECOND typedef, not a widened [DngFullDecoder] -- Dart's function-type +/// subtyping means adding even an OPTIONAL named parameter to a typedef +/// breaks every existing closure assigned to it (see the erratum recorded at +/// `payload_reencoder.dart:13-22`, where `Enc e = fakeOld;` is a compile +/// error after such a widening). [DngFullDecoder]'s declaration therefore +/// stays byte-identical, and this is a separate, additional seam -- exactly +/// as `PointerPayloadEncoder` is the separate sibling of `PayloadEncoder`. +/// +/// [exifOrientation] is the DECLARED orientation (from Halcyon's own IFD0 +/// walk); the returned [DecodedRgba.appliedOrientation] reports what the +/// decoder actually did with it, which may be less than requested (or +/// nothing at all, on an older dylib or a non-RAW arm). +typedef DngOrientingFullDecoder = Future Function( + String path, { + required int exifOrientation, +}); + /// The app's only defence against an OOM from a container header that claims /// an absurd extent: refuse when `width * height * 4` exceeds this many bytes. /// diff --git a/lib/services/image_pipeline/exif_orientation.dart b/lib/services/image_pipeline/exif_orientation.dart index 72f881b..2e15b82 100644 --- a/lib/services/image_pipeline/exif_orientation.dart +++ b/lib/services/image_pipeline/exif_orientation.dart @@ -34,3 +34,118 @@ ExifTransform exifTransformFor(int orientation) { _ => (quarterTurnsCw: 0, mirrored: false), }; } + +/// The orientation still to be applied by the host, given that the decoder +/// already applied [applied] and the file's IFD0 tag declares [declared]. +/// +/// Total: defined for every `int` pair (including values outside 1..8 -- +/// [exifTransformFor] already treats those as identity), never asserts, +/// never throws. Three cases: +/// * `applied == declared` -> 1 (the normal native-rotation case: nothing +/// left to do). +/// * `applied == 1` -> `declared` (today's behaviour: the decoder applied +/// nothing, so the host must apply everything). +/// * anything else -> the actual composite, computed by treating each of +/// the two `(quarterTurnsCw, mirrored)` pairs as an element of the +/// dihedral group D4 (the group of symmetries of a rectangle) and +/// looking the composite `declared ∘ applied⁻¹` back up in this file's +/// 8-case table. +/// +/// This is the mechanism that makes multi-arm decoder dispatch safe: a +/// decoder that ignores the orientation request, a build whose dylib +/// predates native orientation, and the pure-Dart TIFF arm are all correct +/// without a feature flag, because they all report `applied == 1` and this +/// function falls back to "apply the full declared orientation". +int residualExifOrientation({required int declared, required int applied}) { + if (applied == declared) return 1; + if (applied == 1) return declared; + + final declaredMatrix = _matrixFor(exifTransformFor(declared)); + final appliedMatrix = _matrixFor(exifTransformFor(applied)); + // Every matrix produced by _matrixFor is orthogonal (rows/columns are + // unit vectors, entries in {-1,0,1}), so its inverse is its transpose. + final appliedInverse = _transpose(appliedMatrix); + final residualMatrix = _matMul(declaredMatrix, appliedInverse); + + for (var quarterTurnsCw = 0; quarterTurnsCw < 4; quarterTurnsCw++) { + for (final mirrored in const [false, true]) { + final candidate = _matrixFor(( + quarterTurnsCw: quarterTurnsCw, + mirrored: mirrored, + )); + if (_matEq(candidate, residualMatrix)) { + return _orientationFor(quarterTurnsCw, mirrored); + } + } + } + // Unreachable: the 8 (quarterTurnsCw, mirrored) pairs above enumerate all + // of D4, which is closed under the composition above. Kept as a defined + // total fallback rather than an assertion per this function's contract. + return 1; +} + +/// A 2x2 integer matrix, row-major: `[[a,b],[c,d]]`. +typedef _Mat2 = List>; + +_Mat2 _rotationMatrix(int quarterTurnsCw) { + return switch (quarterTurnsCw % 4) { + 1 => [ + [0, -1], + [1, 0], + ], + 2 => [ + [-1, 0], + [0, -1], + ], + 3 => [ + [0, 1], + [-1, 0], + ], + _ => [ + [1, 0], + [0, 1], + ], + }; +} + +const _Mat2 _horizontalMirrorMatrix = [ + [-1, 0], + [0, 1], +]; + +/// The linear part of applying [transform]: rotate first, then mirror -- +/// matching this file's dartdoc ("[quarterTurnsCw] is applied FIRST, then +/// [mirrored] flips horizontally"). +_Mat2 _matrixFor(ExifTransform transform) { + final rotation = _rotationMatrix(transform.quarterTurnsCw); + return transform.mirrored ? _matMul(_horizontalMirrorMatrix, rotation) : rotation; +} + +_Mat2 _matMul(_Mat2 a, _Mat2 b) { + return [ + [ + a[0][0] * b[0][0] + a[0][1] * b[1][0], + a[0][0] * b[0][1] + a[0][1] * b[1][1], + ], + [ + a[1][0] * b[0][0] + a[1][1] * b[1][0], + a[1][0] * b[0][1] + a[1][1] * b[1][1], + ], + ]; +} + +_Mat2 _transpose(_Mat2 a) => [ + [a[0][0], a[1][0]], + [a[0][1], a[1][1]], +]; + +bool _matEq(_Mat2 a, _Mat2 b) => + a[0][0] == b[0][0] && a[0][1] == b[0][1] && a[1][0] == b[1][0] && a[1][1] == b[1][1]; + +int _orientationFor(int quarterTurnsCw, bool mirrored) { + for (var n = 1; n <= 8; n++) { + final t = exifTransformFor(n); + if (t.quarterTurnsCw == quarterTurnsCw && t.mirrored == mirrored) return n; + } + return 1; +} diff --git a/test/services/image_pipeline/exif_residual_test.dart b/test/services/image_pipeline/exif_residual_test.dart new file mode 100644 index 0000000..af82691 --- /dev/null +++ b/test/services/image_pipeline/exif_residual_test.dart @@ -0,0 +1,117 @@ +// Task 6 (native-rotation-spec.md): residual-orientation contract tests. +// +// AC-6.3 requires the 64-pair composition check to run against an +// INDEPENDENT oracle -- not a re-derivation of residualExifOrientation's own +// matrix algebra. The oracle below works a different way entirely: it +// simulates the actual pixel movement of a small labelled grid (rotate the +// list-of-lists, then mirror the rows), using only exifTransformFor (the +// normative table itself, per the spec) to decide how many quarter turns and +// whether to mirror. It never touches residualExifOrientation's matrix code. +import 'package:flutter_test/flutter_test.dart'; +import 'package:halcyon_flutter/services/image_pipeline/exif_orientation.dart'; + +/// A small labelled grid: `grid[y][x]` is a unique pixel id. +typedef _Grid = List>; + +_Grid _rotate90Cw(_Grid m) { + final h = m.length; + final w = m[0].length; + final result = List.generate(w, (_) => List.filled(h, -1)); + for (var y = 0; y < h; y++) { + for (var x = 0; x < w; x++) { + result[x][h - 1 - y] = m[y][x]; + } + } + return result; +} + +_Grid _mirrorHorizontal(_Grid m) => m.map((row) => row.reversed.toList()).toList(); + +_Grid _applyExifToGrid(_Grid base, int orientation) { + final transform = exifTransformFor(orientation); + var grid = base; + for (var i = 0; i < transform.quarterTurnsCw; i++) { + grid = _rotate90Cw(grid); + } + if (transform.mirrored) { + grid = _mirrorHorizontal(grid); + } + return grid; +} + +bool _gridEq(_Grid a, _Grid b) { + if (a.length != b.length) return false; + for (var y = 0; y < a.length; y++) { + if (a[y].length != b[y].length) return false; + for (var x = 0; x < a[y].length; x++) { + if (a[y][x] != b[y][x]) return false; + } + } + return true; +} + +/// A 3x2 (width x height) base grid with a unique id per pixel, chosen +/// non-square and non-symmetric so rotate/mirror confusions are observable. +_Grid _baseGrid() => [ + [0, 1, 2], + [3, 4, 5], +]; + +void main() { + group('residualExifOrientation', () { + test('AC-6.1: residual(n, n) == 1 for all n in 1..8', () { + for (var n = 1; n <= 8; n++) { + expect( + residualExifOrientation(declared: n, applied: n), + 1, + reason: 'declared == applied == $n should need nothing further', + ); + } + }); + + test('AC-6.2: residual(n, 1) == n for all n in 1..8', () { + for (var n = 1; n <= 8; n++) { + expect( + residualExifOrientation(declared: n, applied: 1), + n, + reason: 'decoder applied nothing, host must apply everything', + ); + } + }); + + test( + 'AC-6.3: all 64 (declared, applied) pairs match an independent ' + 'pixel-simulation oracle', + () { + final base = _baseGrid(); + for (var declared = 1; declared <= 8; declared++) { + for (var applied = 1; applied <= 8; applied++) { + final residual = residualExifOrientation( + declared: declared, + applied: applied, + ); + + final appliedGrid = _applyExifToGrid(base, applied); + final resultGrid = _applyExifToGrid(appliedGrid, residual); + final declaredGrid = _applyExifToGrid(base, declared); + + expect( + _gridEq(resultGrid, declaredGrid), + isTrue, + reason: + 'declared=$declared applied=$applied residual=$residual: ' + 'applying residual on top of the already-applied frame ' + 'must equal applying declared directly to the original', + ); + } + } + }, + ); + + test('is total for out-of-range ints (no throw, no assert)', () { + expect(() => residualExifOrientation(declared: 0, applied: 0), returnsNormally); + expect(() => residualExifOrientation(declared: -5, applied: 99), returnsNormally); + expect(residualExifOrientation(declared: 0, applied: 0), 1); + }); + }); +} From edc010e02e7475ace480b2a9a732bc8df9ae7372 Mon Sep 17 00:00:00 2001 From: claudecode_project Date: Sun, 6 Sep 2026 23:42:09 +0800 Subject: [PATCH 2/7] feat: thread orienting decoder seam through PhotoSource (Task 8, native-rotation-spec) Adds DngOrientingFullDecoder? orientingDngDecoder to PhotoSource, selected in decodePhase/decodePhaseExpensive over dngDecoder when non-null. Adds dispatchOrientingFullDecode routing only the RAW arm to an orienting decoder (heif/jxl/tiff report appliedOrientation:1, unchanged). dng_decode_service.dart gains a pass-through production binding since the pinned ceyx package has no oriented entry yet (lands with spec Tasks 3-5). usePointer at photo_source.dart:615 is byte-identical (AC-8.2). New test file covers AC-8.1-8.5 against a fake orienting decoder. Co-Authored-By: Claude --- .../image_pipeline/dng_decode_service.dart | 76 ++++++- .../image_pipeline/full_decoder_dispatch.dart | 31 ++- .../image_preload_controller.dart | 23 +- lib/services/image_pipeline/photo_source.dart | 28 ++- .../native_orientation_pointer_test.dart | 212 ++++++++++++++++++ 5 files changed, 353 insertions(+), 17 deletions(-) create mode 100644 test/services/image_pipeline/native_orientation_pointer_test.dart diff --git a/lib/services/image_pipeline/dng_decode_service.dart b/lib/services/image_pipeline/dng_decode_service.dart index a280975..16994f0 100644 --- a/lib/services/image_pipeline/dng_decode_service.dart +++ b/lib/services/image_pipeline/dng_decode_service.dart @@ -2,6 +2,7 @@ import 'package:ceyx/ceyx.dart'; import 'package:flutter/foundation.dart'; import '../../perf/perf_log.dart'; +import '../platform/working_set_trim.dart'; import 'dng_decode_contract.dart'; /// P2: routes the [DngFullDecoder] seam through ceyx's persistent worker pool @@ -59,7 +60,7 @@ bool decodePoolEnabledFor(String raw) => raw != '0' && raw != 'false' && raw != 'off'; Future decodeDngFull(String path) async { - _ensurePoolLogger(); + ensureHalcyonDecodePoolConfigured(); final image = kDecodePoolEnabled ? await CeyxDecodePool.shared.decode(path) // LEGACY ARM: one isolate spawn + one dylib load per decode. Kept @@ -97,15 +98,68 @@ Future decodeDngFull(String path) async { /// `image_preload_controller.dart`. const DngFullDecoder halcyonDngFullDecoder = decodeDngFull; -bool _poolLoggerInstalled = false; +/// Task 8 (native-rotation-spec) production binding for +/// [DngOrientingFullDecoder]. The pinned ceyx package in this tree today has +/// NO oriented decode entry (that arrives with spec Tasks 3-5's pin bump) -- +/// so THIS ROUND it is a thin pass-through: run today's unoriented decode and +/// report `appliedOrientation: 1`, exactly like every existing fake decoder +/// default. Halcyon applies the whole declared orientation via the residual +/// (`residualExifOrientation`), so behaviour is byte-identical to the +/// `dngDecoder`-only path -- this only exists so `PhotoSource` has a seam to +/// call, wired end-to-end, ready for the oriented pool entry to drop in here +/// once the pin is bumped. +/// +/// ponytail: one function, no branching on decodeIntoBufferOrientedAvailable +/// yet -- there is nothing to branch on until Task 5 lands. Add the guarded +/// lookup then, not before (referencing a not-yet-existing ceyx symbol here +/// would break the build today). +Future decodeDngFullOriented( + String path, { + required int exifOrientation, +}) => decodeDngFull(path); + +const DngOrientingFullDecoder halcyonOrientingDngFullDecoder = + decodeDngFullOriented; + +bool _poolConfigured = false; + +/// One-time process configuration of the ceyx decode pool. Idempotent; called +/// from every entry point below, so no startup ordering has to be maintained. +/// +/// Does three things: +/// +/// 1. **Wires the native buffer pool (R6, Task #9, user ruling 2026-09-06).** +/// `CeyxDecodePool.nativeBufferPool` defaults to null in the ceyx package, +/// and every pooled-route gate short-circuits on that null +/// (`decode_pool.dart:532-535`). Until this assignment existed the whole +/// WP6/WP10 decode-into route was reachable only from ceyx's own tests: +/// production decodes fell back to the legacy native allocator, and nothing +/// was red anywhere — the route was shipped, tested, and carrying zero +/// traffic. The assignment lives HERE rather than as a default inside ceyx +/// because a library must not decide on its own to hold eight ~100MB +/// resident slots for every consumer; the host app owns that budget. +/// +/// 2. **Suppresses the idle working-set trim.** See +/// [WorkingSetTrim.suppressed]: idle trimming pages out exactly the idle +/// pooled slots the pool keeps resident for immediate reuse. The +/// folder-switch trim (`trimNow`) is deliberately left enabled. +/// +/// 3. **Routes pool events into the perf log and the console.** A silently +/// narrowed pool is exactly the defect class this loudness exists to +/// prevent, so it is deliberately not gated on `PerfLog.enabled`. +void ensureHalcyonDecodePoolConfigured() { + // RE-ASSERTED on every call, deliberately NOT behind the latch below. These + // two are process invariants held in mutable statics that other code (and + // any test helper) can clear; two stores are free, whereas a latched + // assignment that something else resets afterwards leaves the pooled route + // silently off — which is the exact failure this whole task exists to fix. + // The latch guards only the closure allocations, which is all it was ever + // for. + CeyxDecodePool.nativeBufferPool = CeyxNativeBufferPool.shared; + WorkingSetTrim.suppressed = true; -/// Routes pool events (ready / worker died / respawn / narrowing) into the -/// perf log AND onto the console. A silently narrowed pool is exactly the -/// defect class this loudness exists to prevent, so it is deliberately not -/// gated on `PerfLog.enabled`. -void _ensurePoolLogger() { - if (_poolLoggerInstalled) return; - _poolLoggerInstalled = true; + if (_poolConfigured) return; + _poolConfigured = true; CeyxDecodePool.logger = (line) { PerfLog.log(line); debugPrint('[ceyx-pool] $line'); @@ -134,7 +188,7 @@ void _ensurePoolLogger() { /// resulting throw, so a superseded decode can never write a permanent-miss /// latch into the newly opened folder's state. void bumpHalcyonDecodePoolGeneration() { - _ensurePoolLogger(); + ensureHalcyonDecodePoolConfigured(); CeyxDecodePool.shared.bumpGeneration(); } @@ -166,7 +220,7 @@ List? halcyonDecodeWidthRecommendations() { /// clamps it against the machine's recommended width; that recommendation is /// displayed in settings and is advisory only. void setHalcyonDecodePoolWidth(int width) { - _ensurePoolLogger(); + ensureHalcyonDecodePoolConfigured(); CeyxDecodePool.shared.width = width; // Requested, not effective: the effective value arrives asynchronously as a // worker ack and is logged by the pool logger installed above. Logging both diff --git a/lib/services/image_pipeline/full_decoder_dispatch.dart b/lib/services/image_pipeline/full_decoder_dispatch.dart index e3807a5..8def412 100644 --- a/lib/services/image_pipeline/full_decoder_dispatch.dart +++ b/lib/services/image_pipeline/full_decoder_dispatch.dart @@ -6,7 +6,8 @@ import 'package:image/image.dart' as img; import '../../models/supported_photo_formats.dart'; import 'dng_decode_contract.dart'; -import 'dng_decode_service.dart'; +import 'dng_decode_service.dart' + show halcyonDngFullDecoder, halcyonOrientingDngFullDecoder; import 'dng_embedded_jpeg_extractor.dart'; import 'heif_decode_service.dart'; import 'jxl_decode_service.dart'; @@ -163,3 +164,31 @@ Future dispatchFullDecode( /// closure: the optional named arms are extra parameters, so the tear-off is /// still a subtype of the seam typedef. const DngFullDecoder halcyonFullDecoder = dispatchFullDecode; + +/// [DngOrientingFullDecoder]-shaped sibling of [dispatchFullDecode] (Task 8, +/// native-rotation-spec). Routes ONLY the RAW arm to an orienting decoder; +/// heif/jxl/tiff have no native orientation capability this round (OQ-2: +/// RAW only, parked out of scope), so they call today's unoriented decoder +/// and report `appliedOrientation: 1` -- the host applies the full +/// orientation via the residual, exactly as it does today. +Future dispatchOrientingFullDecode( + String path, { + required int exifOrientation, + DngOrientingFullDecoder rawArm = halcyonOrientingDngFullDecoder, + DngFullDecoder tiffArm = decodeTiffFull, + DngFullDecoder heifArm = halcyonHeifFullDecoder, + DngFullDecoder jxlArm = halcyonJxlFullDecoder, +}) async { + if (SupportedPhotoFormats.isLibheifPath(path)) return heifArm(path); + if (SupportedPhotoFormats.isJxlPath(path)) return jxlArm(path); + if (SupportedPhotoFormats.isBitmapDecodePath(path)) return tiffArm(path); + if (SupportedPhotoFormats.isDecodablePath(path)) { + return rawArm(path, exifOrientation: exifOrientation); + } + throw UnsupportedError('no full-decode route for $path'); +} + +/// The composition root's entry point for the orienting seam. See +/// [halcyonFullDecoder]'s dartdoc for why this is a plain tear-off. +const DngOrientingFullDecoder halcyonOrientingFullDecoder = + dispatchOrientingFullDecode; diff --git a/lib/services/image_pipeline/image_preload_controller.dart b/lib/services/image_pipeline/image_preload_controller.dart index 85c9d1c..1add6cf 100644 --- a/lib/services/image_pipeline/image_preload_controller.dart +++ b/lib/services/image_pipeline/image_preload_controller.dart @@ -158,6 +158,14 @@ class ImagePreloadController { ImagePreloadController({ required NativeImageLoad imageLoader, DngFullDecoder? dngDecoder, + // Task 8 (native-rotation-spec): the orienting sibling of [dngDecoder], + // threaded straight to [PhotoSource]. No production default is baked in + // here -- unlike [payloadEncoder]/[pointerPayloadEncoder] this mirrors + // [dngDecoder]'s OWN pattern (explicit injection, no controller-side + // default), so every existing caller of this constructor (including + // `AppState.forTesting`, which this file does not own) is unaffected + // until the composition root chooses to pass one. + DngOrientingFullDecoder? orientingDngDecoder, PayloadEncoder? payloadEncoder = _encodeJpegNative, PointerPayloadEncoder? pointerPayloadEncoder = _encodeJpegFromNativeRgba, RetentionPolicy retention = const RetentionPolicy.floor(), @@ -181,6 +189,7 @@ class ImagePreloadController { _source = PhotoSource( loader: imageLoader, dngDecoder: dngDecoder, + orientingDngDecoder: orientingDngDecoder, payloadEncoder: payloadEncoder, pointerPayloadEncoder: pointerPayloadEncoder, compositeGate: compositeGate, @@ -1802,7 +1811,19 @@ class ImagePreloadController { '|payloadKind=$payloadKind' '|bytes=${decode.encodedPayload?.byteCost ?? decode.fullRes?.rgba.lengthInBytes ?? -1}' '|cost=${decode.observedCost}' - '|exifOrientation=${decode.exifOrientation}', + '|exifOrientation=${decode.exifOrientation}' + // PROBE 1 (jank-rootcause-analysis.md §6). `exifOrientation` above is + // null on every RAW item BY CONTRACT (photo_source.dart's decode arm + // reports "nothing to carry forward", not "no rotation"), so until now + // the log could not tell a rotated item from an identity one -- and + // that is the split that decides whether the item pays a GPU pass plus + // two full-frame copies on this isolate. `fullRes.image` is non-null + // if and ONLY if the EXIF transform was not the identity + // (decoded_rgba_image_provider.dart's `decodedRgbaToOrientedFullRes` + // short-circuit), so it IS the discriminator, readable here without + // widening the SourceDecode record. The numeric orientation itself + // rides on the `orient|` line emitted at the transform site. + '|rotatedPass=${decode.fullRes?.image != null}', ); PerfLog.log( 'decode|id=$id|rawDecode=${decode.rawDecodeRan}' diff --git a/lib/services/image_pipeline/photo_source.dart b/lib/services/image_pipeline/photo_source.dart index 8b2800f..d8ceb8e 100644 --- a/lib/services/image_pipeline/photo_source.dart +++ b/lib/services/image_pipeline/photo_source.dart @@ -193,6 +193,7 @@ class PhotoSource { const PhotoSource({ required this.loader, this.dngDecoder, + this.orientingDngDecoder, this.payloadEncoder, this.pointerPayloadEncoder, this.compositeGate = immediateCompositeGate, @@ -213,6 +214,13 @@ class PhotoSource { /// other unrecoverable file. final DngFullDecoder? dngDecoder; + /// Task 8 (native-rotation-spec): the orienting sibling of [dngDecoder]. + /// When non-null, [decodePhase]/[decodePhaseExpensive] call this instead of + /// [dngDecoder], passing the declared EXIF orientation so a RAW frame can + /// come back already oriented. Null (every existing construction site) is + /// the byte-identical legacy path -- [dngDecoder] is used exactly as today. + final DngOrientingFullDecoder? orientingDngDecoder; + /// Phase 13: turns the decoded RAW's FULL-RESOLUTION pixels into the single /// JPEG bitstream the item retains, so a no-preview RAW becomes the same /// cache citizen as a JPG at both tiers. NULL means "do not re-encode" -- @@ -311,7 +319,8 @@ class PhotoSource { :final declaredPreviewsUnreadable, ): final decoder = dngDecoder; - if (decoder == null) { + final orienting = orientingDngDecoder; + if (decoder == null && orienting == null) { // D3 (docs/logs/2026-08-26/raw-support-contract.md): a missing // native library is a static platform property, decided HERE, // before any decoder is invoked -- never inferred from a caught @@ -357,7 +366,13 @@ class PhotoSource { // conflated: this is FFI/decode wall time, `materialize` is the // GPU-texture/engine-buffer hand-off cost. final materializeStartUs = PerfLog.enabled ? PerfLog.us : 0; - final decoded = await decoder(path); + // Task 8: prefer the orienting seam when the caller wired one -- + // [decoder] is guaranteed non-null in the `else` arm because the + // guard above only lets this try block run when at least one of + // the two is non-null. + final decoded = orienting != null + ? await orienting(path, exifOrientation: exifOrientation) + : await decoder!(path); if (PerfLog.enabled) { PerfLog.log( 'decode.ffi|id=$path' @@ -482,7 +497,8 @@ class PhotoSource { required int exifOrientation, }) async { final decoder = dngDecoder; - if (decoder == null) { + final orienting = orientingDngDecoder; + if (decoder == null && orienting == null) { // D3: decided before invoking anything, same as [decodePhase]'s arm // above -- see its comment. In practice this arm is unreachable today // (a null decoder never defers in [decodePhase], so there is nothing @@ -506,7 +522,11 @@ class PhotoSource { try { // P0 -- `decode.ffi`, see the matching comment in [decodePhase]'s try block. final materializeStartUs = PerfLog.enabled ? PerfLog.us : 0; - final decoded = await decoder(path); + // Task 8: same orienting-seam preference as [decodePhase] -- see its + // comment on why [decoder]'s null-assert is safe here. + final decoded = orienting != null + ? await orienting(path, exifOrientation: exifOrientation) + : await decoder!(path); if (PerfLog.enabled) { PerfLog.log( 'decode.ffi|id=$path' diff --git a/test/services/image_pipeline/native_orientation_pointer_test.dart b/test/services/image_pipeline/native_orientation_pointer_test.dart new file mode 100644 index 0000000..13d520f --- /dev/null +++ b/test/services/image_pipeline/native_orientation_pointer_test.dart @@ -0,0 +1,212 @@ +// Task 8 (native-rotation-spec.md): PhotoSource's orienting decoder seam. +// +// This round has no real oriented ceyx entry (Tasks 3-5 land later), so every +// case here drives a FAKE `DngOrientingFullDecoder` -- exactly what the spec +// intends for this round (Task 8 is re-verified against the real dylib in +// round 3). AC-8.6 (full suite green, analyze 0) is verified by the test +// runner outside this file. + +import 'dart:io'; +import 'dart:typed_data'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:halcyon_flutter/models/photo_item.dart'; +import 'package:halcyon_flutter/services/image_pipeline/dng_decode_contract.dart'; +import 'package:halcyon_flutter/services/image_pipeline/image_preload_controller.dart'; +import 'package:halcyon_flutter/services/image_pipeline/image_source_types.dart'; +import 'package:halcyon_flutter/services/image_pipeline/payload_reencoder.dart'; +import 'package:halcyon_flutter/services/image_pipeline/photo_payload.dart'; +import 'package:image/image.dart' as img; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + /// A 4x8 OPAQUE RGBA frame, alpha 0xFF so the identity short-circuit's + /// sampled-opaque assert in `decoded_rgba_image_provider.dart` holds. + Uint8List opaqueRgba(int width, int height) { + final rgba = Uint8List(width * height * 4); + for (var i = 0; i < rgba.length; i += 4) { + rgba[i] = 0x40; + rgba[i + 1] = 0x80; + rgba[i + 2] = 0xC0; + rgba[i + 3] = 0xFF; + } + return rgba; + } + + List rawItems(List ids) => [ + for (final id in ids) PhotoItem(id: id, files: [File('/tmp/$id.dng')]), + ]; + + /// Every RAW item needs a real decode, declaring orientation 6 (the AC-8.1 + /// fixture's rotation): the loader answers NeedsRawDecode so the item is + /// deferred to the serial lane exactly as a preview-less DNG is. + Future needsRawDecodeLoaderOrientation6( + String path, { + required ImageRequestPurpose purpose, + int? targetLongEdge, + }) async => const NativeImageNeedsRawDecode(exifOrientation: 6); + + Future pumpMicrotasks([int rounds = 24]) async { + for (var i = 0; i < rounds; i++) { + await Future.delayed(Duration.zero); + } + } + + group('native_orientation_pointer_test.dart', () { + // AC-8.1 / AC-8.4: a fake orienting decoder reporting it already applied + // the declared orientation (residual == identity) and handing back a + // native-backed buffer must take the pointer encoder, never the byte + // encoder, and must not count as a re-encode fallback. + test( + 'orientingDngDecoder with appliedOrientation matching declared uses ' + 'the pointer encoder', + () async { + final keeper = Object(); + var pointerCalls = 0; + var copyCalls = 0; + resetReencodeCounters(); + addTearDown(resetReencodeCounters); + + final controller = ImagePreloadController( + imageLoader: needsRawDecodeLoaderOrientation6, + // Legacy seam left null: only the orienting seam is exercised, and + // AC-8.5 (a separate test below) is what proves that binding this + // to null does not disturb anything. + orientingDngDecoder: (path, {required exifOrientation}) async => + DecodedRgba( + // Already-oriented: the source frame was 4 wide x 8 tall, and + // orientation 6 (a 90 CW turn) swaps the extent -- exactly the + // ORIENTED shape a real ceyx entry would hand back. + rgba: opaqueRgba(8, 4), + width: 8, + height: 4, + nativeAddress: 0x5678, + nativeKeepAlive: keeper, + appliedOrientation: exifOrientation, + ), + payloadEncoder: + (rgba, {required width, required height, required quality}) async { + copyCalls++; + return Uint8List.fromList([0xFF, 0xD8, 0xFF, 0xD9]); + }, + pointerPayloadEncoder: + ({ + required nativeAddress, + required width, + required height, + required quality, + keepAlive, + }) async { + pointerCalls++; + expect(nativeAddress, 0x5678); + expect(identical(keepAlive, keeper), isTrue); + // AC-8.3 (AD-040): return a REAL jpeg at the ORIENTED extent, so + // the retained payload is provably the swapped shape rather than + // an opaque stub the test cannot check. + final frame = img.Image(width: width, height: height); + return Uint8List.fromList(img.encodeJpg(frame, quality: quality)); + }, + decodeLaneWidth: 1, + ); + addTearDown(controller.dispose); + controller.updateTargetSize(32, 32); + + await controller.preloadImages( + items: rawItems(['a']), + selectedItemId: 'a', + notifyLoaded: () {}, + ); + await pumpMicrotasks(); + + expect(pointerCalls, 1); + expect(copyCalls, 0, reason: 'AC-8.1: byte encoder must not run'); + expect( + reencodeFallbacks, + 0, + reason: 'AC-8.4: an oriented native decode must not fall back', + ); + + final payload = controller.payloadFor('a'); + expect(payload, isA()); + + // AC-8.3: the retained bytes decode to the ORIENTED extent (8x4, the + // swap of the fake decoder's declared 4x8 source), proving the + // pointer path carried the oriented width/height through, not the + // unrotated ones. + final decoded = img.decodeJpg((payload as EncodedPayload).bytes); + expect(decoded, isNotNull); + expect(decoded!.width, 8); + expect(decoded.height, 4); + }, + ); + + // AC-8.2: `usePointer`'s expression at photo_source.dart:615 is + // byte-identical after this task -- grepped mechanically, not eyeballed. + test( + 'photo_source.dart usePointer expression is byte-identical (AC-8.2)', + () { + final source = File( + 'lib/services/image_pipeline/photo_source.dart', + ).readAsStringSync(); + final needle = + 'final usePointer = fullRes != null && fullRes.image == null'; + final matches = needle.allMatches(source).length; + expect( + matches, + 1, + reason: 'expected exactly one byte-identical usePointer line', + ); + }, + ); + + // AC-8.5: the null-binding arm is the byte-for-byte control -- a decode + // with NO orientingDngDecoder configured must behave exactly as the + // pre-Task-8 byte-copy path (this mirrors encode_test.dart's existing + // "native-backed identity decode" case, but explicitly asserts the new + // parameter defaulting to null changes nothing). + test( + 'orientingDngDecoder: null leaves the legacy dngDecoder path unchanged', + () async { + var legacyCalls = 0; + var copyCalls = 0; + resetReencodeCounters(); + addTearDown(resetReencodeCounters); + + Future identityLoader( + String path, { + required ImageRequestPurpose purpose, + int? targetLongEdge, + }) async => const NativeImageNeedsRawDecode(exifOrientation: 1); + + final controller = ImagePreloadController( + imageLoader: identityLoader, + dngDecoder: (path) async { + legacyCalls++; + return DecodedRgba(rgba: opaqueRgba(4, 4), width: 4, height: 4); + }, + // Deliberately omitted: orientingDngDecoder defaults to null. + payloadEncoder: + (rgba, {required width, required height, required quality}) async { + copyCalls++; + return Uint8List.fromList([0xFF, 0xD8, 0xFF, 0xD9]); + }, + decodeLaneWidth: 1, + ); + addTearDown(controller.dispose); + controller.updateTargetSize(32, 32); + + await controller.preloadImages( + items: rawItems(['a']), + selectedItemId: 'a', + notifyLoaded: () {}, + ); + await pumpMicrotasks(); + + expect(legacyCalls, 1); + expect(copyCalls, 1); + expect(controller.payloadFor('a'), isA()); + }, + ); + }); +} From a03b513c1b3bb896a2adc19c7924eaf6430977d0 Mon Sep 17 00:00:00 2001 From: claudecode_project Date: Sun, 6 Sep 2026 23:42:51 +0800 Subject: [PATCH 3/7] feat: residual-driven orientation in decoded_rgba_image_provider (Task 7) Compute _ExifTransform from residualExifOrientation(declared, applied) in all three entry points (decodedRgbaToImage, decodedRgbaToPixelPayload, decodedRgbaToOrientedFullRes), so a natively-oriented RAW frame hits the identity short-circuit and keeps its native buffer instead of paying a GPU round trip. orient| probe line gains applied=/residual= (append-only, existing fields frozen). Adds TC-1086..1089 covering AC-7.1/7.2/7.5 and the extended probe format. Co-Authored-By: Claude --- .../decoded_rgba_image_provider.dart | 43 +++++- .../image_pipeline/decoded_rgba_test.dart | 136 ++++++++++++++++++ 2 files changed, 175 insertions(+), 4 deletions(-) diff --git a/lib/services/image_pipeline/decoded_rgba_image_provider.dart b/lib/services/image_pipeline/decoded_rgba_image_provider.dart index ec240c7..30683e4 100644 --- a/lib/services/image_pipeline/decoded_rgba_image_provider.dart +++ b/lib/services/image_pipeline/decoded_rgba_image_provider.dart @@ -32,13 +32,17 @@ Future decodedRgbaToImage( // below: a slow or never-granted slot must not be able to leave a ~50MB // `ui.Image` parked with no owner. AC7: an identity orientation composites // nothing, so it buys nothing. - if (!_ExifTransform.forOrientation(exifOrientation).isIdentity) { + final residual = residualExifOrientation( + declared: exifOrientation, + applied: rgba.appliedOrientation, + ); + if (!_ExifTransform.forOrientation(residual).isIdentity) { await gate(); } final raw = await _imageFromPixels(rgba); late final ui.Image oriented; try { - oriented = await applyExifOrientation(raw, exifOrientation); + oriented = await applyExifOrientation(raw, residual); } catch (_) { raw.dispose(); rethrow; @@ -149,7 +153,12 @@ Future decodedRgbaToPixelPayload( CompositeGate gate = immediateCompositeGate, }) async { _assertDecodedBufferLength(decoded); - final transform = _ExifTransform.forOrientation(exifOrientation); + final transform = _ExifTransform.forOrientation( + residualExifOrientation( + declared: exifOrientation, + applied: decoded.appliedOrientation, + ), + ); // SHORT-CIRCUIT. With an identity transform and no downscale to apply, the // old code uploaded ~50MB to the GPU, drew nothing new, and read ~50MB back @@ -255,7 +264,33 @@ Future decodedRgbaToOrientedFullRes( CompositeGate gate = immediateCompositeGate, }) async { _assertDecodedBufferLength(decoded); - final transform = _ExifTransform.forOrientation(exifOrientation); + final residual = residualExifOrientation( + declared: exifOrientation, + applied: decoded.appliedOrientation, + ); + final transform = _ExifTransform.forOrientation(residual); + + // PROBE 1 (jank-rootcause-analysis.md §6): the REAL EXIF orientation and + // whether it forces a GPU pass. `req_end`'s `exifOrientation=` is null on + // every RAW item by construction (photo_source.dart's decode arm reports + // "nothing to carry forward", not "no rotation"), so the log could not tell + // a rotated item from an identity one -- which is exactly the split that + // decides whether this item pays two full-frame on-isolate copies. Logged + // here because this is the single place that knows both facts. + // + // `applied=`/`residual=` (Task 7, native-rotation-spec.md §1.4): appended + // fields, existing field names/meanings frozen. `rotated` still means "a + // GPU pass will run" -- now decided by the RESIDUAL, not the declared + // value, so a natively-oriented RAW frame correctly reports `rotated=false`. + if (PerfLog.enabled) { + PerfLog.log( + 'orient|exif=$exifOrientation' + '|applied=${decoded.appliedOrientation}' + '|residual=$residual' + '|rotated=${!transform.isIdentity}' + '|bytes=${decoded.rgba.lengthInBytes}', + ); + } // Same short-circuit as decodedRgbaToPixelPayload's: nothing to rotate and // nothing to scale means there is nothing for the GPU to do. diff --git a/test/services/image_pipeline/decoded_rgba_test.dart b/test/services/image_pipeline/decoded_rgba_test.dart index f6ebaff..109e373 100644 --- a/test/services/image_pipeline/decoded_rgba_test.dart +++ b/test/services/image_pipeline/decoded_rgba_test.dart @@ -6,6 +6,7 @@ import 'dart:ui' as ui; import 'package:ceyx/ceyx.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:halcyon_flutter/models/supported_photo_formats.dart'; +import 'package:halcyon_flutter/perf/perf_log.dart'; import 'package:halcyon_flutter/services/image_pipeline/decoded_rgba_image_provider.dart'; import 'package:halcyon_flutter/services/image_pipeline/dng_decode_contract.dart'; import 'package:halcyon_flutter/services/image_pipeline/exif_orientation.dart'; @@ -363,6 +364,41 @@ void main() { full.image!.dispose(); }); + // TC-1085 -- probe 1 (jank-rootcause-analysis.md §6). `req_end` reports + // `exifOrientation=null` for every RAW item by construction, so this line + // is the only place the log can tell a rotated item (which pays a GPU pass + // plus two full-frame on-isolate copies) from an identity one. + test('orient probe records the real orientation and whether it rotates', + () async { + final dir = await Directory.systemTemp.createTemp('orient-probe'); + final logPath = '${dir.path}/perf.log'; + try { + PerfLog.init(logPath); + await decodedRgbaToOrientedFullRes(_sourceShort(), exifOrientation: 1); + final rotated = await decodedRgbaToOrientedFullRes( + _sourceShort(), + exifOrientation: 6, + ); + rotated.image!.dispose(); + await PerfLog.flush(); + + final orient = File(logPath) + .readAsStringSync() + .split('\n') + .where((l) => l.contains('orient|')) + .toList(); + expect(orient, hasLength(2)); + expect(orient[0], contains('exif=1')); + expect(orient[0], contains('rotated=false')); + expect(orient[1], contains('exif=6')); + expect(orient[1], contains('rotated=true')); + expect(orient[1], contains('bytes=${2 * 3 * 4}')); + } finally { + PerfLog.enabled = false; + await dir.delete(recursive: true); + } + }); + // TC-824c -- the handle is handed out live, not already disposed. test('the returned handle is the caller\'s to dispose', () async { final src = _sourceShort(); @@ -1154,6 +1190,106 @@ void main() { }); }); + group('decoded_rgba_residual_orientation_test.dart', () { + TestWidgetsFlutterBinding.ensureInitialized(); + + /// A 2x3 opaque frame whose decoder already applied orientation + /// [appliedOrientation]. + DecodedRgba nativelyOriented(int appliedOrientation) { + final bytes = Uint8List(2 * 3 * 4); + for (var p = 0; p < 6; p++) { + bytes[p * 4] = 10 + p * 20; + bytes[p * 4 + 3] = 0xFF; + } + return DecodedRgba( + rgba: bytes, + width: 2, + height: 3, + appliedOrientation: appliedOrientation, + ); + } + + // TC-1086 -- AC-7.1: declared == applied means the residual is identity, + // so the short-circuit runs and the native buffer is kept, not copied. + test('declared 6 + applied 6 short-circuits and keeps the native buffer', + () async { + final src = nativelyOriented(6); + final full = + await decodedRgbaToOrientedFullRes(src, exifOrientation: 6); + expect(full.image, isNull); + expect(identical(full.rgba, src.rgba), isTrue); + }); + + // TC-1087 -- AC-7.2: the legacy arm (decoder applied nothing) is + // untouched -- a GPU pass still runs and a handle comes back. + test('declared 6 + applied 1 still runs the GPU pass', () async { + final src = nativelyOriented(1); + final full = + await decodedRgbaToOrientedFullRes(src, exifOrientation: 6); + expect(full.image, isNotNull); + full.image!.dispose(); + }); + + // TC-1088 -- AC-7.5, seen RED before the change (verified manually by + // reverting the residual call in decodedRgbaToOrientedFullRes: this test + // failed with 1 materialize| event instead of 0). A natively-oriented RAW + // frame must never hit ui.decodeImageFromPixels for the full-res frame. + test('a natively-oriented frame emits zero materialize| events', () async { + final dir = await Directory.systemTemp.createTemp('residual-probe'); + final logPath = '${dir.path}/perf.log'; + try { + PerfLog.init(logPath); + final full = await decodedRgbaToOrientedFullRes( + nativelyOriented(6), + exifOrientation: 6, + ); + full.image?.dispose(); + await PerfLog.flush(); + + final materializeLines = File(logPath) + .readAsStringSync() + .split('\n') + .where((l) => l.contains('materialize|')) + .toList(); + expect(materializeLines, isEmpty); + } finally { + PerfLog.enabled = false; + await dir.delete(recursive: true); + } + }); + + // TC-1089 -- AC-7.4 companion: the orient| probe line now carries + // applied= and residual=, appended after the frozen exif=/rotated=/bytes= + // fields; script-level parsing is exercised separately by + // analyze_perf.py's own test (a synthetic log with both shapes). + test('orient probe appends applied= and residual=, existing fields frozen', + () async { + final dir = await Directory.systemTemp.createTemp('orient-fields'); + final logPath = '${dir.path}/perf.log'; + try { + PerfLog.init(logPath); + await decodedRgbaToOrientedFullRes( + nativelyOriented(6), + exifOrientation: 6, + ); + await PerfLog.flush(); + + final line = File(logPath) + .readAsStringSync() + .split('\n') + .firstWhere((l) => l.contains('orient|')); + expect(line, contains('exif=6')); + expect(line, contains('applied=6')); + expect(line, contains('residual=1')); + expect(line, contains('rotated=false')); + expect(line, contains('bytes=${2 * 3 * 4}')); + } finally { + PerfLog.enabled = false; + await dir.delete(recursive: true); + } + }); + }); + group('exif_orientation_test.dart', () { test('TC-213 exifTransformFor maps all eight EXIF values', () { expect(exifTransformFor(1), (quarterTurnsCw: 0, mirrored: false)); From a1a4b3ede977566a156a33acced909304186379f Mon Sep 17 00:00:00 2001 From: claudecode_project Date: Sun, 6 Sep 2026 23:45:40 +0800 Subject: [PATCH 4/7] build(macos): hard staged-decoder arch gate, arch stamp auto-clean, x64 docs un-parked --- scripts/build_apps.py | 141 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 130 insertions(+), 11 deletions(-) diff --git a/scripts/build_apps.py b/scripts/build_apps.py index 992bd38..47d1bee 100755 --- a/scripts/build_apps.py +++ b/scripts/build_apps.py @@ -151,10 +151,18 @@ never executed must not report success. Unchanged here. S1 accepted: HALIDE_SHA256 pins every asset this script can fetch; verified after download and before extraction; a mismatch quarantines the file. -PL-6: macOS builds arm64 only (MACOS_DEFAULT_ARCH). Intel is parked because the - prebuilt decoder dylib is arm64-only, so a universal app's x86_64 slice - links without the native decoder - ld only WARNS about that, which is how it - went unnoticed. verify_macos_slices() now fails the build instead. +PL-6: macOS DEFAULTS to arm64 (MACOS_DEFAULT_ARCH). Intel was parked when the + prebuilt decoder dylib was arm64-only, so a universal app's x86_64 slice + linked without the native decoder - ld only WARNS about that, which is how + it went unnoticed. verify_macos_slices() fails the build instead. + SUPERSEDED 2026-09-06: the ceyx release now publishes separate macos-arm64 + and macos-x86_64 assets, both pinned by digest, so `--macos-arch x86_64` is + supported and Intel is no longer parked. `universal` still is: there is no + fat archive to fetch, so fetch_target_for() rejects it instead of guessing. + The same round closed the inverse hazard - a fetch was due only when the + destination was ABSENT, never when the WRONG ARCHITECTURE was present, so an + x86_64 build against a staged arm64 dylib succeeded with a mere warning. + verify_placed_macos_arch() now hard-fails that before the build starts. """ import argparse import hashlib @@ -232,13 +240,33 @@ "x86-64-windows": "4efec94b7c8958b1ae0125a73245a148e4c98dbb54f2678bc34c6abe36ee899a", } -# The user parked Intel/x86_64 macOS support (PL-6): the prebuilt decoder dylib -# is arm64-only, so a universal app links its x86_64 slice without the native -# decoder. Forwarded to xcodebuild via FLUTTER_XCODE_ARCHS, which flutter_tools -# passes straight through (flutter_tools/lib/src/macos/build_macos.dart:258 -> +# Default only, no longer a restriction. PL-6 parked Intel because the single +# prebuilt decoder dylib was arm64-only; since the ceyx v0.1.8 pin the release +# publishes SEPARATE macos-arm64 and macos-x86_64 assets, so `--macos-arch +# x86_64` is a supported, digest-verified build (see CEYX_FETCH_SPECS). arm64 +# stays the default because that is what this project ships and what CI's +# Apple-silicon runner builds. `universal` remains unsupported: there is no fat +# release archive, and fetch_target_for() rejects it rather than guess an arch. +# +# RUNTIME FLOOR, and it differs by architecture: the bundled six-dylib native +# stack requires macOS 15 on Apple silicon and macOS 14 on Intel (measured from +# the dylibs' own LC_BUILD_VERSION load commands; see the MINIMUM-OS section in +# scripts/ceyx_release_pin.json). Halcyon's own declared application minimum is +# macOS 11 (macos/Runner/Configs/AppInfo.xcconfig, Info.plist +# LSMinimumSystemVersion). That gap is REAL and deliberately left open here: by +# standing user ruling the native stack's requirements and the application's +# declared minimum are not to be conflated, and nothing in this file changes +# the app minimum. +# +# Forwarded to xcodebuild via FLUTTER_XCODE_ARCHS, which flutter_tools passes +# straight through (flutter_tools/lib/src/macos/build_macos.dart:258 -> # ios/xcodeproj.dart:442). No file under macos/ needs to change. MACOS_DEFAULT_ARCH = "arm64" +# Records which architecture build/macos was last built for, so switching arch +# can invalidate it (enforce_macos_arch_stamp). +MACOS_ARCH_STAMP = ".halcyon-macos-arch" + # JDK search order copied verbatim from scripts/build.sh:87-89. MACOS_JDK_CANDIDATES = [ ("25", "/Library/Java/JavaVirtualMachines/temurin-25.jdk/Contents/Home"), @@ -2426,6 +2454,83 @@ def verify_macos_slices(app_bundle, layout, args): ) +def verify_placed_macos_arch(layout, args): + """Hard gate, run on EVERY macOS build: the decoder already staged in the + ceyx checkout must be the architecture this build asked for. + + Why this is separate from the post-fetch architecture check in + build_target(): a fetch is due only when a destination file is ABSENT + (ceyx_fetch_is_due), never when a file of the WRONG ARCHITECTURE is + present. So `--macos-arch x86_64` against an already-staged arm64 dylib + used to skip the fetch, skip the post-fetch check (which lives inside + `if fetch_due:`), build an x86_64 app around an arm64 decoder, and exit 0 + - the only complaint being verify_macos_slices()'s warn() after the fact. + That ships an Intel app whose RAW decoder cannot load on any Intel Mac. + + This is not an exotic corruption: CEYX_FETCH_SPECS places macos-arm64 and + macos-x86_64 into the SAME directory (plugin/macos/Libraries), so "the + other architecture is staged" is the normal state right after building the + other arch. Hence a hard fail rather than a warning.""" + if args.macos_arch == "universal": + return # unreachable today: fetch_target_for() rejects universal first. + spec = NATIVE_SPECS["macos"] + dylib = layout.decoder / spec["dest"] / spec["artifact"] + if not dylib.exists(): + return # a missing library is a different failure, reported elsewhere. + slices = lipo_slices(dylib) + if slices is None: + fail( + f"could not determine the architecture of the staged {dylib} " + "(lipo failed or produced no usable output) - refusing to build on " + "an unverified architecture.", + hints=[f"Run `lipo -info {dylib}` directly to see the error.", + "Re-place the library with --fetch-native."], + ) + if slices != {args.macos_arch}: + fail( + f"the staged decoder is {' '.join(sorted(slices))} but this build " + f"targets {args.macos_arch}: {dylib}", + hints=["Pass --fetch-native to replace it with the pinned " + f"macos-{args.macos_arch} release asset.", + "Both architectures are placed into the same directory, so " + "the previous build's architecture is still staged here.", + "Without this check the build would succeed and ship an " + f"app whose decoder cannot load on {args.macos_arch}."], + ) + ok(f"staged decoder architecture matches the build: " + f"{' '.join(sorted(slices))}") + + +def enforce_macos_arch_stamp(layout, args): + """Wipe the macOS build directory when the requested architecture differs + from the one it was last built for. + + Xcode keys its incremental state on the build directory, not on ARCHS, so + an arm64 tree reused for an x86_64 build can carry stale products through. + Deliberate trade-off (team-lead ruling, 2026-09-06): the two architectures + therefore cannot coexist on disk. The alternative was redirecting SYMROOT + per arch, which also moves the path `flutter build macos` expects to find + its OWN products at - a real risk to the working arm64 path for a + cosmetic gain. CI legs each build on a fresh runner, so they never see + this; it exists for local arch-switching.""" + if args.macos_arch == "universal": + return + build_dir = layout.halcyon / "build" / "macos" + stamp = layout.halcyon / "build" / MACOS_ARCH_STAMP + previous = None + if stamp.exists(): + try: + previous = stamp.read_text(encoding="utf-8").strip() or None + except OSError: + previous = None + if build_dir.exists() and previous is not None and previous != args.macos_arch: + step(f"architecture changed ({previous} -> {args.macos_arch}) - " + f"removing {build_dir} so no stale {previous} product survives") + shutil.rmtree(build_dir, ignore_errors=True) + stamp.parent.mkdir(parents=True, exist_ok=True) + stamp.write_text(args.macos_arch + "\n", encoding="utf-8") + + def verify_macos_heif_rpaths(app_bundle): """Mechanically prove spec section 7.2's DYNAMIC-linking decision shipped. @@ -2724,10 +2829,19 @@ def build_target(target, layout, mode, args): ok("native library already present - skipping the native build " "(pass --native always to force a rebuild).") + if target == "macos": + # Unconditional, and deliberately AFTER both the fetch and the local + # native build: whatever produced the staged dylib, it is the file the + # Flutter build is about to embed, so it is the file that must match. + verify_placed_macos_arch(layout, args) + if args.skip_flutter_build: phase("Phase 2: skipped (--skip-flutter-build)") return + if target == "macos": + enforce_macos_arch_stamp(layout, args) + build_flutter(target, layout, mode, args, placed_native) @@ -2837,9 +2951,14 @@ def make_parser(): help="Exit 2 if any warning was raised.") p.add_argument("--macos-arch", choices=["arm64", "x86_64", "universal"], default=MACOS_DEFAULT_ARCH, - help=f"macOS architecture (default: {MACOS_DEFAULT_ARCH}). Intel/universal is " - "parked: the prebuilt decoder dylib is arm64-only, so a universal app's " - "x86_64 slice links without native RAW decode.") + help=f"macOS architecture (default: {MACOS_DEFAULT_ARCH}). x86_64 (Intel) is " + "supported and cross-compiles from an Apple-silicon host; it consumes the " + "pinned macos-x86_64 ceyx asset, so pass --fetch-native when the other " + "architecture's dylib is already staged (both are placed into the same " + "directory, and a mismatch is now a hard failure). 'universal' is NOT " + "supported: the release publishes no fat archive, so there is no asset to " + "fetch and the build is refused rather than silently linking an x86_64 " + "slice with no native RAW decoder.") p.add_argument("--print-halide-pins", action="store_true", help="Print the pinned Halide sha256 table and exit.") p.add_argument("--ios-codesign", action="store_true", From 84976c45eea87d6f50b1b2d7d28a2d7e79af7857 Mon Sep 17 00:00:00 2001 From: claudecode_project Date: Sun, 6 Sep 2026 23:46:26 +0800 Subject: [PATCH 5/7] ci: add macos-x64 leg with decoder-arch and multi-symbol assertions --- .github/workflows/ci.yml | 33 ++++-- .github/workflows/release.yml | 13 +- scripts/ci/assertions.py | 164 ++++++++++++++++++++++++-- scripts/ci/phases.py | 8 +- scripts/ci/targets.py | 93 +++++++++++++++ scripts/ci/tests/golden/macos-x64.txt | 1 + scripts/ci/tests/test_policy.py | 69 +++++++++++ scripts/ci/tests/test_render.py | 48 +++++++- scripts/dng_ffi_artifacts.json | 22 ++++ 9 files changed, 429 insertions(+), 22 deletions(-) create mode 100644 scripts/ci/tests/golden/macos-x64.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52148e6..41688cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,8 +72,21 @@ jobs: path: Halcyon/build/ci-logs/** if-no-files-found: warn + # Two macOS legs, one per architecture. BOTH run on macos-14 (Apple silicon): + # the x64 leg is a CROSS-COMPILE (`--macos-arch x86_64`, rendered by + # scripts/ci/targets.py), not an Intel runner. That is deliberate — it matches + # how the capability was first proved locally and avoids depending on the + # retiring macos-13 Intel image — and it has a stated cost: the functional FFI + # probe cannot load an x86_64 dylib from an arm64 process, so the x64 leg's + # assertion list omits it in data and gates on Mach-O header + nm symbol + # checks instead. A green x64 leg therefore does NOT claim runtime loadability + # on real Intel hardware. See the "assertions" comment in targets.py. build: - name: Build macOS (release) + name: Build ${{ matrix.target }} (release) + strategy: + fail-fast: false # lets the sibling arch leg finish; a leg still fails when it fails + matrix: + target: [macos, macos-x64] runs-on: macos-14 steps: - uses: actions/checkout@v4 @@ -92,20 +105,22 @@ jobs: channel: 'stable' # R-8 cache added in F6 after R-7 assertions are green (Spec §6) - - name: ci.py provision --target macos - run: python3 scripts/ci.py provision --target macos + - name: ci.py provision + run: python3 scripts/ci.py provision --target ${{ matrix.target }} - - name: ci.py build --target macos - run: python3 scripts/ci.py build --target macos + - name: ci.py build + run: python3 scripts/ci.py build --target ${{ matrix.target }} - - name: ci.py assert-capabilities --target macos - run: python3 scripts/ci.py assert-capabilities --target macos + - name: ci.py assert-capabilities + run: python3 scripts/ci.py assert-capabilities --target ${{ matrix.target }} - - name: Upload ci-logs (build macos) + # Names must be unique across matrix legs or upload-artifact@v4 rejects the + # collision, hence the matrix value in the name. + - name: Upload ci-logs (build ${{ matrix.target }}) uses: actions/upload-artifact@v4 if: always() with: - name: ci-logs-build-macos + name: ci-logs-build-${{ matrix.target }} path: Halcyon/build/ci-logs/** if-no-files-found: warn diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9112e40..a4a52dd 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -67,9 +67,16 @@ jobs: fail-fast: false # lets sibling matrix legs finish; a leg still fails when it fails matrix: include: - - {os: macos-14, target: macos, archive: 'Halcyon-macos-arm64-'} - - {os: windows-latest, target: windows, archive: 'Halcyon-windows-x64-'} - - {os: ubuntu-latest, target: linux, archive: 'Halcyon-linux-x64-'} + # macos-x64 runs on macos-14 too: it is a CROSS-COMPILE + # (--macos-arch x86_64, rendered from scripts/ci/targets.py), not an + # Intel runner. Its archive prefix must differ from the arm64 leg's or + # the two would overwrite each other on the release; the + # `test_macos_legs_do_not_collide_on_archive_name` selftest asserts + # that mechanically against targets.py. + - {os: macos-14, target: macos, archive: 'Halcyon-macos-arm64-'} + - {os: macos-14, target: macos-x64, archive: 'Halcyon-macos-x64-'} + - {os: windows-latest, target: windows, archive: 'Halcyon-windows-x64-'} + - {os: ubuntu-latest, target: linux, archive: 'Halcyon-linux-x64-'} runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 diff --git a/scripts/ci/assertions.py b/scripts/ci/assertions.py index 3305dd2..92ddf92 100644 --- a/scripts/ci/assertions.py +++ b/scripts/ci/assertions.py @@ -55,6 +55,21 @@ SYMBOL = "dng_decode_and_process_sized" +# The full entry-point set the Dart side looks up in the shipped decoder. SYMBOL +# above is the historical single-symbol record (H-SIZED-SYMBOL / -NM) and is kept +# exactly as it was; this tuple is what H-CEYX-SYMBOLS-NM checks, and it includes +# SYMBOL so that assertion is a strict superset rather than a parallel truth. +# Verified present in the shipped dylib on 2026-09-06 (`nm -gU` on +# ../ceyx/plugin/macos/Libraries/libdng_decoder_native.dylib listed all three). +# NOTE the leading-underscore convention: Mach-O prefixes C symbols with "_", so +# nm prints "_ceyx_probe_output_size". check_symbol() does a substring match, so +# the undecorated spelling below matches on both Mach-O and ELF. +CEYX_SYMBOLS = ( + SYMBOL, + "ceyx_probe_output_size", + "ceyx_decode_into_buffer", +) + # Host detection is a dict lookup, not a branch (G-5). It is used ONLY for skip # semantics — "is this artefact running on its own platform?" — never to choose # what to build or where to look. @@ -278,6 +293,64 @@ class Assertion: ), expected=f"'{SYMBOL}' occurs in the captured nm output", ), + "H-DECODER-ARCH": Assertion( + id="H-DECODER-ARCH", + measures=( + "the machine architecture of the ceyx DECODER LIBRARY inside the " + "shipped artefact equals the architecture declared for that target" + ), + valid_on=("macos", "linux", "windows"), + why_valid=( + "Same structural header read as H-ARCH (Mach-O cputype / ELF " + "e_machine / PE Machine), in pure Python, so it is host-independent " + "and no toolchain can invert it. It is a SEPARATE assertion from " + "H-ARCH because they measure different files that can disagree: " + "H-ARCH reads the Flutter runner, which the local build produces, " + "while this reads the prebuilt decoder, which is FETCHED from the " + "ceyx release pin. The macOS x64 leg is exactly the case where they " + "can diverge — an app correctly built x86_64 next to an arm64 " + "decoder dylib fetched from the wrong pin entry is a shipping " + "artefact that cannot load its own decoder at runtime, and every " + "other assertion in the suite (presence, pinned digests, nm " + "symbols) passes on it, because nm parses a foreign-architecture " + "Mach-O file perfectly happily. Nothing else in the suite would " + "catch it." + ), + red_state=( + "put the arm64 decoder dylib in an x86_64 target's artefact (i.e. " + "fetch the macos-arm64 pin entry for the macos-x64 leg): the " + "assertion fails naming both the expected and the observed arch" + ), + expected="observed decoder arch == the target's expected_arch in dng_ffi_artifacts.json", + ), + "H-CEYX-SYMBOLS-NM": Assertion( + id="H-CEYX-SYMBOLS-NM", + measures=( + "EVERY entry point the Dart side looks up — " + + ", ".join(CEYX_SYMBOLS) + + " — appears in the shipped decoder's symbol table" + ), + valid_on=("macos", "linux"), + why_valid=( + "Same instrument and same validity argument as H-SIZED-SYMBOL-NM " + "(Mach-O/ELF default visibility means a listed symbol really is " + "dlsym-resolvable), applied to the whole set instead of one member " + "of it. It exists because a guarded FFI lookup nulls out the ENTIRE " + "binding when ANY one symbol is missing, so a decoder carrying only " + "the historical symbol ships a silently absent feature — the " + "2026-09-06 incident. Windows is excluded for the identical reason " + "H-SIZED-SYMBOL-NM excludes it (PE exports nothing by default, so " + "the symbol table measures a build setting, not reachability). The " + "tool's output is captured to a str and matched in Python, never " + "piped to grep (G-3)." + ), + red_state=( + "run against a decoder built without ceyx_decode_into_buffer (or " + "strip it): the captured nm output lacks that name and the " + "assertion fails naming exactly which symbols were missing" + ), + expected="all of " + ", ".join(CEYX_SYMBOLS) + " occur in the captured nm output", + ), } _MANDATORY_FIELDS = ("measures", "valid_on", "why_valid", "red_state", "expected") @@ -324,14 +397,24 @@ def ffi_entry_for(repo_root, target): def platform_of(repo_root, target): """The artefact platform name (macos/windows/linux/android) for a CI target. - The mapping lives in dng_ffi_artifacts.json as the ``ci_target`` field, so - it is data, not a branch. Falls back to the target name itself. + Read from ``targets.spec(target)["assert_platform"]`` — data, not a branch, + and in the one file G-5 designates for per-platform facts. + + Why this is NOT derived from dng_ffi_artifacts.json's key any more: two CI + legs can ship the SAME platform for different architectures (macos / + macos-x64), and each needs its own manifest entry for expected_arch and + decoder_artifact. Deriving the platform name from the manifest KEY would + have given the x64 leg a platform of its own ("macos-x86_64"), which is in + no assertion's ``valid_on`` and, worse, would never equal ``host_platform()`` + — so run_suite()'s "a skip on the artefact's OWN platform is a FAILURE" rule + (Spec §4.5) would have silently stopped applying to that entire leg, which + is precisely the 2026-08-25 silently-skipped-gate failure. Architecture is + asserted as architecture (H-ARCH / H-DECODER-ARCH), never smuggled in as a + platform name. The ``repo_root`` parameter is retained for call-site + compatibility and is unused. """ - manifest = _load_json(Path(repo_root) / "scripts" / "dng_ffi_artifacts.json") - for name, entry in manifest["platforms"].items(): - if entry.get("ci_target") == target: - return name - return target + del repo_root # kept for signature stability; the answer is target data now + return targets.spec(target)["assert_platform"] def pinned_libraries(repo_root, pin_platform): @@ -832,6 +915,71 @@ def _assert_sized_symbol_nm(ctx): return "pass", f"{SYMBOL} listed by {tool} for {path.name}" +def _assert_decoder_arch(ctx): + """The FETCHED decoder library's own architecture, read from its header. + + Deliberately reads the member's bytes out of the artefact source (rather + than materialising and shelling out to lipo/file) for the same reason + H-ARCH does: a struct.unpack of a format-defined constant cannot be + inverted by a missing tool, a foreign host, or a shell pipeline (G-3). + """ + entry = ctx["ffi_entry"] + if entry is None: + return "skip", "no dng_ffi_artifacts.json entry declares this ci_target" + expected = entry["expected_arch"] + basename = entry["decoder_artifact"] + hits = ctx["source"].find([basename]) + if not hits: + return "fail", ( + f"{basename} is absent from {ctx['source'].describe()}, so its " + "architecture cannot be read" + ) + try: + arch = machine_arch(ctx["source"].read(hits[0])) + except ValueError as exc: + return "fail", f"{hits[0]}: {exc}" + if arch != expected: + return "fail", ( + f"{hits[0]}: expected decoder arch {expected}, observed {arch} — " + "the app would fail at DynamicLibrary.open on the target machine" + ) + return "pass", f"{hits[0]}={arch} (expected {expected})" + + +def _assert_ceyx_symbols_nm(ctx): + """All of CEYX_SYMBOLS in the shipped decoder's symbol table.""" + entry = ctx["ffi_entry"] + if entry is None: + return "skip", "no dng_ffi_artifacts.json entry declares this ci_target" + tool = entry.get("tool") + if not tool or shutil.which(tool) is None: + return "skip", f"symbol-table tool {tool!r} is not on PATH" + path, error = _decoder_disk_path(ctx) + if path is None: + return "fail", error + check = _check_symbol() + tool_args = entry.get("tool_args", []) + missing = [] + unreadable = [] + for symbol in CEYX_SYMBOLS: + status = check(path, tool, tool_args, symbol) + if status == "skipped": + unreadable.append(symbol) + elif status != "present": + missing.append(symbol) + if unreadable: + # The tool resolved but could not read the file — an environment defect, + # reported as a skip so run_suite()'s own-platform rule can turn it into + # a failure where that is the right answer, rather than deciding here. + return "skip", f"{tool} resolved but could not inspect {path.name}" + if missing: + return "fail", ( + f"absent from {tool} output for {path.name}: {', '.join(missing)} " + f"(checked {', '.join(CEYX_SYMBOLS)})" + ) + return "pass", f"{tool} lists all of {', '.join(CEYX_SYMBOLS)} for {path.name}" + + def _check_symbol(): """Import scripts/check_dng_ffi_artifacts.py's check_symbol lazily. @@ -853,6 +1001,8 @@ def _check_symbol(): "H-DECODER-HASH": _assert_decoder_hash, "H-SIZED-SYMBOL": _assert_sized_symbol, "H-SIZED-SYMBOL-NM": _assert_sized_symbol_nm, + "H-DECODER-ARCH": _assert_decoder_arch, + "H-CEYX-SYMBOLS-NM": _assert_ceyx_symbols_nm, } diff --git a/scripts/ci/phases.py b/scripts/ci/phases.py index 7996289..d8febd8 100644 --- a/scripts/ci/phases.py +++ b/scripts/ci/phases.py @@ -31,10 +31,16 @@ def _build_argv(repo_root: Path, target: str) -> list: `check_python_interpreter()` refuses for this process. Handing the child the interpreter that already passed that check is what makes the refusal cover the whole run rather than only its first process. + + The positional comes from ``spec["build_target"]``, NOT from ``target``: + the CI target name and the build_apps.py target name coincide for most + legs but not all (``macos-x64`` builds the ``macos`` target with + ``--macos-arch x86_64``). Reading it from the data keeps that difference a + dict lookup rather than a name-equality branch (G-5). """ spec = targets.spec(target) build_apps = os.fspath(Path(repo_root, "scripts", "build_apps.py").resolve()) - return [sys.executable, build_apps, target, *spec["build_flags"]] + return [sys.executable, build_apps, spec["build_target"], *spec["build_flags"]] def provision(repo_root: Path, target: str) -> int: diff --git a/scripts/ci/targets.py b/scripts/ci/targets.py index 0ea60bd..60f7d1c 100644 --- a/scripts/ci/targets.py +++ b/scripts/ci/targets.py @@ -11,9 +11,27 @@ provision release.yml:137 (apt), ci.yml:121 / release.yml:57 (pod install) artifact_path build_apps.py:1730-1752 (flutter_artifact) archive_name release.yml:66/105/144 + build_target build_apps.py:2820-2845 (the positional `target` argparse accepts) app_executable macos/Runner/Configs/AppInfo.xcconfig:8 (PRODUCT_NAME), windows/CMakeLists.txt:7 and linux/CMakeLists.txt:7 (BINARY_NAME) +``build_target`` vs the dict KEY. The key is the CI TARGET NAME (what +``ci.py --target`` takes and what a workflow matrix leg names); +``build_target`` is the positional ``scripts/build_apps.py`` is invoked with. +For five of the six they are identical. They are NOT identical for +``macos-x64``: build_apps.py has no separate Intel target, it has a +``--macos-arch`` FLAG on the one ``macos`` target (build_apps.py:2838-2840), +so that CI leg renders ``build_apps.py macos --macos-arch x86_64 …``. Keeping +the two names as separate fields is what lets one build entry point serve two +CI legs without any name-equality branching anywhere else (G-5). + +``assert_platform`` is the artefact PLATFORM name the R-7 assertion suite +judges this target as (``assertions.platform_of``). It is neither the +architecture nor the runner: BOTH macOS legs declare "macos", so both stay +subject to run_suite()'s "a skip on the artefact's own platform is a FAILURE" +rule. See the macos-x64 entry's own comment for why inventing a per-arch +platform name would have silently disabled that rule for the whole leg. + ``app_executable`` is the basename of the Flutter runner binary inside the shipped artefact, and it is NOT the same string on every platform: only macOS is named after the product ("Halcyon"); Windows is lowercase ("halcyon.exe") @@ -26,6 +44,8 @@ TARGETS: dict = { "macos": { + "build_target": "macos", + "assert_platform": "macos", "runs_on": "macos-14", # --fetch-native: as of the HALCYON-MIGRATION campaign (2026-09, tag # v0.1.8) macOS is fetched from the ceyx release pin, the same as @@ -66,7 +86,69 @@ ], "pin_platform": "macos-arm64", }, + "macos-x64": { + # Intel macOS, CROSS-COMPILED on the same Apple-silicon runner image the + # arm64 leg uses. build_apps.py has no separate Intel target: it has one + # `macos` target plus a `--macos-arch` flag (build_apps.py:2838-2840), + # which sets FLUTTER_XCODE_ARCHS (2560-2565), selects the pin's + # "macos-x86_64" asset via fetch_target_for() (1440-1467), and already + # refuses to finish if the produced app's slices are not exactly + # {x86_64} (2405) or the fetched dylib's are not (2702). Hence + # build_target "macos" with the arch carried in build_flags. + "build_target": "macos", + # The artefact platform for the assertion suite is "macos", NOT a new + # platform name. That is deliberate and load-bearing: assertions.py + # treats a skip as a FAILURE only when the artefact's platform equals + # the host's (Spec §4.5, the 2026-08-25 silently-skipped-gate lesson). + # Inventing a "macos-x86_64" artefact platform would make every skip on + # this leg "legitimate" — a missing nm, an unreadable manifest entry — + # and the leg would report green while measuring nothing. Architecture + # is not a platform here; it is asserted directly, by H-ARCH and + # H-DECODER-ARCH, against expected_arch=x86_64. + "assert_platform": "macos", + # macos-14 (Apple silicon), not macos-13 (Intel): this leg is a + # cross-compile, which is exactly what the local proof did, and it keeps + # both macOS legs on one runner image rather than depending on the + # retiring Intel image. The cost is stated, not hidden — see the + # H-SIZED-SYMBOL omission in "assertions" below. + "runs_on": "macos-14", + "build_flags": ["--macos-arch", "x86_64", "--fetch-native"], + # Identical to the arm64 leg: same Podfile, same gitignored + # Flutter-Generated.xcconfig that only `pub get` creates. + "provision": [["flutter", "pub", "get"], ["pod", "install"]], + "artifact_kind": "app_bundle", + # Same output path as the arm64 build — the two never coexist on one + # runner, because each CI/release matrix leg builds exactly one of them. + "artifact_path": "build/macos/Build/Products/Release/Halcyon.app", + "app_executable": "Halcyon", + "archive_name": "Halcyon-macos-x64-{version}.zip", + "archive_format": "zip", + # H-SIZED-SYMBOL (the functional FFI probe) is DELIBERATELY ABSENT, for + # the same class of reason H-SIZED-SYMBOL-NM is absent on windows: the + # instrument is structurally invalid here. The probe is + # `dart run` + DynamicLibrary.open, and an arm64 dart process cannot + # load an x86_64 dylib, so on this runner it could only ever report a + # loader failure that says nothing about the artefact. It is omitted in + # DATA, visibly, rather than silently skipped at run time. What replaces + # it: H-DECODER-ARCH (the shipped dylib really is x86_64) and + # H-SIZED-SYMBOL-NM / H-CEYX-SYMBOLS-NM (nm reads a foreign-arch Mach-O + # file fine on any host, because it parses the file rather than loading + # it). Runtime loadability on real Intel hardware is therefore NOT + # measured by this leg and must not be claimed from a green run. + "assertions": [ + "H-ARCH", + "H-DECODER-PRESENT", + "H-DECODER-ARCH", + "H-DECODER-DEPS", + "H-DECODER-HASH", + "H-SIZED-SYMBOL-NM", + "H-CEYX-SYMBOLS-NM", + ], + "pin_platform": "macos-x86_64", + }, "windows": { + "build_target": "windows", + "assert_platform": "windows", "runs_on": "windows-latest", # --fetch-native, not plain auto: ceyx still carries a committed # dng_decoder_native.dll (hand-built, no S4 colour-gate record). Auto-fetch @@ -93,6 +175,8 @@ "pin_platform": "windows", }, "linux": { + "build_target": "linux", + "assert_platform": "linux", "runs_on": "ubuntu-latest", "build_flags": [], "provision": [ @@ -118,6 +202,8 @@ "pin_platform": "linux", }, "android-apk": { + "build_target": "android-apk", + "assert_platform": "android", "runs_on": "ubuntu-latest", "build_flags": [], "provision": [], @@ -134,6 +220,11 @@ "pin_platform": None, }, "web": { + "build_target": "web", + # No manifest entry and no native artefact: platform_of() previously fell + # back to the target name for this target, and "web" preserves that + # exactly. Its assertion list is empty, so nothing consumes it. + "assert_platform": "web", "runs_on": "ubuntu-latest", "build_flags": [], "provision": [], @@ -150,6 +241,8 @@ # Every entry must carry exactly these keys (Plan §2). Enforced by _validate(). REQUIRED_KEYS = ( + "build_target", + "assert_platform", "runs_on", "build_flags", "provision", diff --git a/scripts/ci/tests/golden/macos-x64.txt b/scripts/ci/tests/golden/macos-x64.txt new file mode 100644 index 0000000..56f4913 --- /dev/null +++ b/scripts/ci/tests/golden/macos-x64.txt @@ -0,0 +1 @@ +['python3', 'scripts/build_apps.py', 'macos', '--macos-arch', 'x86_64', '--fetch-native'] diff --git a/scripts/ci/tests/test_policy.py b/scripts/ci/tests/test_policy.py index d5dbaa0..683000d 100644 --- a/scripts/ci/tests/test_policy.py +++ b/scripts/ci/tests/test_policy.py @@ -313,6 +313,75 @@ def test_release_publish_step_marks_latest_explicitly(self): "release.yml must expose a real publish path for the gate") +class TestReleaseMatrixMatchesTargets(unittest.TestCase): + """release.yml's matrix and scripts/ci/targets.py must agree. + + The failure this prevents is silent and expensive: the publish step globs + `Halcyon/.*`, while `ci.py package` writes whatever + targets.py's `archive_name` says. If the two drift, the leg builds, asserts + and packages GREEN, and then `fail_on_unmatched_files` fires at the very end + of the release — or, worse, two legs share a prefix and one silently + overwrites the other's asset. Nothing else in the tree ties the YAML literal + to the Python data, so this test is that tie. + """ + + MATRIX_LINE_RE = re.compile( + r"^\s*-\s*\{os:\s*([\w.-]+)\s*,\s*target:\s*([\w.-]+)\s*,\s*archive:\s*'([^']+)'\s*\}" + ) + + def _matrix_entries(self): + path = WORKFLOWS_DIR / "release.yml" + if not path.is_file(): + self.skipTest(f"{path} not present") + entries = [] + for line in path.read_text(encoding="utf-8").splitlines(): + m = self.MATRIX_LINE_RE.match(line) + if m: + entries.append((m.group(1), m.group(2), m.group(3))) + self.assertTrue(entries, "no release.yml matrix include entries parsed") + return entries + + def _targets_module(self): + import sys # noqa: PLC0415 + + scripts_dir = REPO_ROOT / "scripts" + if str(scripts_dir) not in sys.path: + sys.path.insert(0, str(scripts_dir)) + import ci.targets as targets # noqa: PLC0415 + + return targets + + def test_every_matrix_target_exists_and_prefixes_match(self): + targets = self._targets_module() + for runner, target, prefix in self._matrix_entries(): + with self.subTest(target=target): + spec = targets.spec(target) # KeyError -> unknown target + expected_prefix = spec["archive_name"].split("{version}")[0] + self.assertEqual( + prefix, + expected_prefix, + f"release.yml's archive prefix for {target!r} ({prefix!r}) does " + f"not match targets.py's archive_name ({spec['archive_name']!r}); " + f"the publish glob would find nothing", + ) + self.assertEqual( + runner, + spec["runs_on"], + f"release.yml runs {target!r} on {runner!r} but targets.py " + f"declares runs_on={spec['runs_on']!r}", + ) + + def test_no_two_release_legs_share_an_archive_prefix(self): + prefixes = [prefix for _, _, prefix in self._matrix_entries()] + duplicates = sorted({p for p in prefixes if prefixes.count(p) > 1}) + self.assertEqual( + duplicates, + [], + f"two release legs publish the same archive prefix, so one would " + f"overwrite the other's asset: {duplicates!r}", + ) + + class TestNoTestExecutionInCI(unittest.TestCase): """CLAUDE.md (2026-08-31 decree): "CI is compile-only ... Functional tests ... are NOT run in CI." No argv list literal anywhere under diff --git a/scripts/ci/tests/test_render.py b/scripts/ci/tests/test_render.py index 6911c34..2ec6234 100644 --- a/scripts/ci/tests/test_render.py +++ b/scripts/ci/tests/test_render.py @@ -37,7 +37,7 @@ # Plan §3/WP-E "print_plan() prints, per phase, `PLAN : `". PLAN_LINE_RE = re.compile(r"^PLAN (\w[\w-]*): (\[.*\])\s*$") -TARGET_NAMES = ["macos", "windows", "linux", "android-apk", "web"] +TARGET_NAMES = ["macos", "macos-x64", "windows", "linux", "android-apk", "web"] def _capture_print_plan(target): @@ -72,7 +72,9 @@ def _expected_build_argv(target): import ci.targets as targets # noqa: PLC0415 spec = targets.spec(target) - return ["python3", "scripts/build_apps.py", target, *spec["build_flags"]] + # The positional is build_target, NOT the CI target name: macos-x64 builds + # build_apps.py's `macos` target with --macos-arch x86_64 (targets.py). + return ["python3", "scripts/build_apps.py", spec["build_target"], *spec["build_flags"]] def _normalize_build_argv(argv): @@ -179,6 +181,48 @@ def test_macos_has_fetch_native(self): ) +class MacosX64ArgvTestCase(GoldenArgvTestCase): + """The macOS x64 leg's whole reason to exist is one flag PAIR. A dropped or + reordered `--macos-arch x86_64` renders an argv that builds arm64 while every + downstream name still says x64, and the golden file alone would not say + which half drifted — so assert the pair's adjacency and the value directly.""" + + def test_macos_x64_renders_the_arch_flag_pair(self): + argv = self._plans_for("macos-x64")["build"] + self.assertIn("--macos-arch", argv, f"macos-x64 build argv lost --macos-arch: {argv!r}") + self.assertEqual( + argv[argv.index("--macos-arch") + 1], + "x86_64", + f"--macos-arch must be immediately followed by x86_64: {argv!r}", + ) + self.assertIn("--fetch-native", argv, + "macos-x64 consumes the ceyx pin's macos-x86_64 entry, which " + "only --fetch-native selects") + + def test_macos_x64_builds_the_macos_positional(self): + """build_apps.py has no `macos-x64` target — passing the CI target name + as the positional would fail with an unknown-target error on the runner, + which is the exact mismatch this file exists to catch pre-commit.""" + argv = self._plans_for("macos-x64")["build"] + import ci.targets as targets # noqa: PLC0415 + + self.assertEqual(argv[2], "macos", f"positional must be 'macos': {argv!r}") + self.assertEqual(targets.spec("macos-x64")["build_target"], "macos") + + def test_macos_legs_do_not_collide_on_archive_name(self): + import ci.targets as targets # noqa: PLC0415 + + names = {t: targets.spec(t)["archive_name"] for t in ("macos", "macos-x64")} + self.assertEqual(len(set(names.values())), 2, + f"the two macOS legs must not publish the same archive name: {names!r}") + + def test_macos_legs_consume_different_pin_entries(self): + import ci.targets as targets # noqa: PLC0415 + + self.assertEqual(targets.spec("macos")["pin_platform"], "macos-arm64") + self.assertEqual(targets.spec("macos-x64")["pin_platform"], "macos-x86_64") + + class ChildInterpreterTestCase(unittest.TestCase): """`ci.py:51-60` refuses an MSYS-style interpreter for the PARENT process. Rendering the literal ``"python3"`` let ``run.py:60``'s ``shutil.which`` diff --git a/scripts/dng_ffi_artifacts.json b/scripts/dng_ffi_artifacts.json index db19af9..8053fb9 100644 --- a/scripts/dng_ffi_artifacts.json +++ b/scripts/dng_ffi_artifacts.json @@ -24,6 +24,28 @@ }, "expected": true }, + "macos-x86_64": { + "path": "../ceyx/plugin/macos/Libraries/libdng_decoder_native.dylib", + "format": "macho", + "tool": "nm", + "tool_args": [ + "-gU" + ], + "comment": "Intel/x86_64 macOS. SAME on-disk path as the 'macos' entry on purpose: build_apps.py places whichever architecture --macos-arch selected at that one path (fetch_target_for(), build_apps.py:1440-1467), so only ONE of the two can be on disk at a time and the manual checker reads whichever the last build fetched. The CI assertions do NOT read this path -- they read the member inside the built/packaged artefact -- so the shared path is not a source of ambiguity there. expected_arch is what distinguishes this entry: it is the value H-ARCH and H-DECODER-ARCH judge the macos-x64 leg against.", + "ci_target": "macos-x64", + "decoder_artifact": "libdng_decoder_native.dylib", + "expected_arch": "x86_64", + "r7": { + "measures": "the shipped Intel macOS app and its decoder dylib are both x86_64, and the dylib exposes every ceyx entry point", + "valid_on": [ + "macos" + ], + "why_valid": "Architecture is read structurally from the Mach-O header in pure Python, which is host-independent, and nm parses a foreign-architecture Mach-O file on any host -- both instruments therefore stay valid on the Apple-silicon runner that cross-compiles this leg. The functional FFI probe does NOT: an arm64 process cannot dlopen an x86_64 dylib, so H-SIZED-SYMBOL is omitted from this target's assertion list in scripts/ci/targets.py, visibly and in data. A green macos-x64 leg consequently claims correct bytes, NOT demonstrated runtime loadability on Intel hardware.", + "red_state": "fetch the macos-arm64 pin entry for this leg (or build without --macos-arch x86_64): H-DECODER-ARCH / H-ARCH fail naming expected x86_64 vs observed arm64", + "expected": "present=True, arch=x86_64 for both the app executable and the decoder, and all of dng_decode_and_process_sized / ceyx_probe_output_size / ceyx_decode_into_buffer listed by nm" + }, + "expected": true + }, "windows": { "path": "../ceyx/plugin/windows/Libraries/dng_decoder_native.dll", "format": "pe", From 6babcf5f7baeeb510ac1b26e3e984fa313a324e5 Mon Sep 17 00:00:00 2001 From: claudecode_project Date: Sun, 6 Sep 2026 23:47:03 +0800 Subject: [PATCH 6/7] test: condition-driven waits replace fixed pump counts; tolerant temp-dir teardown --- .../image_pipeline/admission_gate_test.dart | 21 ++++++- test/services/image_pipeline/encode_test.dart | 12 +++- .../image_preload_flow_test.dart | 15 ++++- test/services/image_pipeline/pacing_test.dart | 21 +++++-- .../services/image_pipeline/sidebar_test.dart | 60 ++++++++++++++++--- 5 files changed, 111 insertions(+), 18 deletions(-) diff --git a/test/services/image_pipeline/admission_gate_test.dart b/test/services/image_pipeline/admission_gate_test.dart index 0acdac6..3113bc0 100644 --- a/test/services/image_pipeline/admission_gate_test.dart +++ b/test/services/image_pipeline/admission_gate_test.dart @@ -11,6 +11,8 @@ import 'package:halcyon_flutter/services/image_pipeline/image_source_types.dart' import 'package:halcyon_flutter/services/image_pipeline/inflight_bytes_budget.dart'; import 'package:halcyon_flutter/services/image_pipeline/retention_policy.dart'; +import '../../support/preload_fixtures.dart' show until; + void main() { group('inflightByteBudgetFor', () { // TC-1042 @@ -204,8 +206,15 @@ void main() { notifyLoaded: () {}, ) .timeout(const Duration(seconds: 5)); - await pumpMicrotasks(64); - for (final id in ['a', 'b', 'c', 'd']) { + // BOUNDED WAIT, not a fixed pump count: preloadImages resolving doesn't + // guarantee every off-lane continuation has landed its payload yet, so + // a fixed 64-pump budget flaked under load ("X never completed"). + const ids = ['a', 'b', 'c', 'd']; + await until( + () => ids.every((id) => controller.payloadFor(id) != null), + reason: 'every item to land a payload once the byte gate frees up', + ); + for (final id in ids) { expect(controller.payloadFor(id), isNotNull, reason: '$id never completed'); } expect( @@ -237,7 +246,13 @@ void main() { notifyLoaded: () {}, ), ); - for (var i = 0; i < 64; i++) { + // CONDITION-DRIVEN sampling, not a fixed 64-microtask budget: this + // still needs to *sample* the running count each tick (there's no + // single event to await for "peak concurrency"), but the sampling + // window is now wall-clock bounded so scheduler contention that slows + // down the ticks can't cut the sample off before the peak is reached. + final deadline = DateTime.now().add(const Duration(seconds: 5)); + while (peakEncodes < 2 && DateTime.now().isBefore(deadline)) { await Future.delayed(Duration.zero); final running = controller.debugEncodeStageRunningCount; if (running > peakEncodes) peakEncodes = running; diff --git a/test/services/image_pipeline/encode_test.dart b/test/services/image_pipeline/encode_test.dart index 9a51262..660e8d4 100644 --- a/test/services/image_pipeline/encode_test.dart +++ b/test/services/image_pipeline/encode_test.dart @@ -13,6 +13,8 @@ import 'package:halcyon_flutter/services/image_pipeline/jpeg_encoder.dart'; import 'package:halcyon_flutter/services/image_pipeline/payload_reencoder.dart'; import 'package:halcyon_flutter/services/image_pipeline/photo_payload.dart'; +import '../../support/preload_fixtures.dart' show until; + void main() { group('encode_stage_test.dart', () { test('runningCount never exceeds width', () async { @@ -236,7 +238,15 @@ void main() { selectedItemId: 'a', notifyLoaded: () => observed.add(controller.payloadFor('a')), ); - await pumpMicrotasks(); + // BOUNDED WAIT, not a pump count: the off-lane encode continuation lands + // the final payload one await after preloadImages resolves, so a fixed + // pump count can legitimately stop between "resolved" and "landed", + // which is what made this flake under load (Expected EncodedPayload, + // Actual null). + await until( + () => controller.payloadFor('a') is EncodedPayload, + reason: "the off-lane encode to land 'a's final EncodedPayload", + ); final landed = controller.payloadFor('a'); expect(landed, isA()); diff --git a/test/services/image_pipeline/image_preload_flow_test.dart b/test/services/image_pipeline/image_preload_flow_test.dart index 8059951..51329f4 100644 --- a/test/services/image_pipeline/image_preload_flow_test.dart +++ b/test/services/image_pipeline/image_preload_flow_test.dart @@ -459,7 +459,20 @@ void main() { selectedItemId: photos[selected].id, notifyLoaded: () {}, ); - await Future.delayed(const Duration(milliseconds: 60)); + // BOUNDED WAIT, not a fixed 60ms sleep: the 40ms debounce plus the + // full-size decodes it fires are real engine futures, so a sleep + // just-longer-than-the-debounce flaked under load when those decodes + // took longer than the 20ms margin. Wait for the actual condition + // (every in-window id ready), bounded at 5s so a real regression + // still fails instead of hanging. + await until( + () => [ + for (var d = -kTierTwoBefore; d <= kTierTwoAfter; d++) + photos[selected + d].id, + ].every(controller.isFullSizeReady), + reason: 'every id in the forward-biased tier-2 window to become ' + 'full-size ready after the debounce settles', + ); for (var d = -kTierTwoBefore; d <= kTierTwoAfter; d++) { expect( diff --git a/test/services/image_pipeline/pacing_test.dart b/test/services/image_pipeline/pacing_test.dart index 605171d..0fabca3 100644 --- a/test/services/image_pipeline/pacing_test.dart +++ b/test/services/image_pipeline/pacing_test.dart @@ -1094,22 +1094,33 @@ void main() { ); await pumpMicrotasks(); - // One drain per frame; pump more frames than the window has slots. - for (var i = 0; i < 12; i++) { + final windowIds = controller.debugRetentionIds; + expect(windowIds.length, greaterThan(4), reason: 'the cap is under test'); + + // CONDITION-DRIVEN, not a fixed frame count: one drain per frame, but + // keep draining until every slot is registered (bounded so a real + // regression -- a slot that never registers -- still fails instead of + // hanging). A fixed 12-frame budget flaked under load because the + // drains themselves can be delayed by scheduler contention, not just + // the registrations they're waiting on. + const maxFrameDrains = 200; + var drains = 0; + while (drains < maxFrameDrains && + !windowIds.every(controller.debugTierOneKeyIds.contains)) { frames.frame(); await pumpMicrotasks(4); + drains++; } - final windowIds = controller.debugRetentionIds; final registered = controller.debugTierOneKeyIds; for (final id in windowIds) { expect( registered, contains(id), - reason: 'slot $id was submitted and must eventually be registered', + reason: 'slot $id was submitted and must eventually be registered ' + 'within $maxFrameDrains frame drains', ); } - expect(windowIds.length, greaterThan(4), reason: 'the cap is under test'); }); // TC-897 -- the controller-level twin of TC-894: with the pacer's exempt diff --git a/test/services/image_pipeline/sidebar_test.dart b/test/services/image_pipeline/sidebar_test.dart index 303cfc1..6db247b 100644 --- a/test/services/image_pipeline/sidebar_test.dart +++ b/test/services/image_pipeline/sidebar_test.dart @@ -127,6 +127,26 @@ Future _tempDirWithPixel(List names) async { return dir; } +/// TC-374's temp-dir teardown (errno-32 on Windows): each test already gets +/// its OWN uniquely-suffixed dir from `createTemp`, so this is not a shared +/// path -- but a transient handle (AV scanner, a still-draining async decode +/// holding the file open a beat longer under load) can still make a single +/// `delete(recursive: true)` fail. Retry a few times with a short backoff and +/// only then give up, so cleanup never fails the test itself. +Future _deleteDirTolerant(Directory dir) async { + for (var attempt = 0; attempt < 5; attempt++) { + try { + if (await dir.exists()) { + await dir.delete(recursive: true); + } + return; + } on FileSystemException { + if (attempt == 4) return; // best-effort cleanup, not a test assertion + await Future.delayed(Duration(milliseconds: 50 * (attempt + 1))); + } + } +} + /// Polls [cond] until it is true or [timeout] elapses, whichever is first -- /// a real debounce/async-drain still gets its full budget if it needs it, but /// the common case (condition already true) returns almost immediately @@ -311,7 +331,15 @@ void main() { endIdx: 5, notifyLoaded: () {}, ); - await Future.delayed(const Duration(milliseconds: 250)); + // CONDITION-DRIVEN, not a fixed 250ms sleep: wait for the sweep's + // debounce to actually enqueue the ids under test rather than a sleep + // sized off "the debounce plus margin", which is the same flaky shape + // fixed above in this file. + await _pollUntilLane( + () => ['p1', 'p2', 'p3', 'p4', 'p5'] + .every((id) => controller.debugLanePendingPriorityFor(id) != null), + const Duration(milliseconds: 5000), + ); for (final id in ['p1', 'p2', 'p3', 'p4', 'p5']) { final priority = controller.debugLanePendingPriorityFor(id); @@ -570,7 +598,7 @@ void main() { test('TC-374 INV-MEM: the sidebar cache stays viewport-bound', () async { final names = [for (var i = 0; i < 200; i++) 'f${i.toString().padLeft(3, "0")}.dng']; final dir = await _tempDirWithPixel(names); - addTearDown(() => dir.delete(recursive: true)); + addTearDown(() => _deleteDirTolerant(dir)); final controller = ImagePreloadController( imageLoader: _alwaysFailLoaderPixel, @@ -609,7 +637,7 @@ void main() { test('TC-378 a stale generation writes nothing into the sidebar cache', () async { final dir = await _tempDirWithPixel(['c.dng']); - addTearDown(() => dir.delete(recursive: true)); + addTearDown(() => _deleteDirTolerant(dir)); final gate = Completer(); final controller = ImagePreloadController( @@ -666,9 +694,6 @@ void main() { endIdx: safeEnd, notifyLoaded: () {}, ); - // Let the sweep's 100ms debounce fire and enqueue every row. - await Future.delayed(const Duration(milliseconds: 250)); - final visibleIds = [ for (var i = safeStart; i <= safeEnd; i++) 'p$i', ]; @@ -676,6 +701,16 @@ void main() { for (var i = safeStart - 20; i < safeStart; i++) 'p$i', for (var i = safeEnd + 1; i <= safeEnd + 20; i++) 'p$i', ]; + // CONDITION-DRIVEN, not a fixed 250ms sleep: the assertions below + // hard-require every margin id to be pending, so wait for that + // directly instead of a sleep sized off "the 100ms debounce plus + // margin", which flaked under load when the debounce's real timer + // was delayed past the fixed budget. + await until( + () => marginIds + .every((id) => controller.debugLanePendingPriorityFor(id) != null), + reason: "the sweep's debounce to fire and enqueue every margin row", + ); // decodeLaneWidth is clamped to a minimum of 1 (decode_lane.dart:75), so // exactly one task is always IN FLIGHT (removed from the pending map, @@ -751,7 +786,10 @@ void main() { endIdx: 150, notifyLoaded: () {}, ); - await Future.delayed(const Duration(milliseconds: 250)); + await until( + () => controller.debugLanePendingPriorityFor('p150') != null, + reason: "the sweep's debounce to fire and enqueue p150", + ); final firstPriority = controller.debugLanePendingPriorityFor('p150'); expect(firstPriority, isNotNull, reason: 'p150 should be pending after sweep 1'); expect( @@ -769,7 +807,13 @@ void main() { endIdx: 152, notifyLoaded: () {}, ); - await Future.delayed(const Duration(milliseconds: 250)); + await until( + () => + controller.debugLanePendingPriorityFor('p150') == + kSidebarPayloadPriorityBase, + reason: 'the second sweep to re-enqueue p150 at its new, improved ' + 'priority', + ); final secondPriority = controller.debugLanePendingPriorityFor('p150'); expect( secondPriority, From c67db8e9a69e8d2ee9e620318c70d36978d66f59 Mon Sep 17 00:00:00 2001 From: claudecode_project Date: Sun, 6 Sep 2026 23:47:28 +0800 Subject: [PATCH 7/7] fix(pool): define WorkingSetTrim.suppressed (repairs edc010e's forward reference) + wiring tests + trim suppression --- .../platform/working_set_trim_io.dart | 25 ++++++ .../platform/working_set_trim_stub.dart | 5 ++ .../image_pipeline/pool_wiring_test.dart | 78 +++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 test/services/image_pipeline/pool_wiring_test.dart diff --git a/lib/services/platform/working_set_trim_io.dart b/lib/services/platform/working_set_trim_io.dart index 71d1cfb..d7bc29c 100644 --- a/lib/services/platform/working_set_trim_io.dart +++ b/lib/services/platform/working_set_trim_io.dart @@ -84,6 +84,23 @@ class WorkingSetTrim { @visibleForTesting static int debugTrimAttempts = 0; + /// Suppresses the IDLE-delayed trim ([request]) while something else owns + /// resident memory deliberately. + /// + /// Set true by `ensureHalcyonDecodePoolConfigured` once the ceyx native + /// buffer pool is wired in (R6, Task #9): that pool keeps a fixed set of + /// ~100MB RGBA slots resident precisely so a returned buffer is reusable + /// IMMEDIATELY, and an idle trim pages out exactly those slots — turning the + /// pool's whole reason for existing into a page-fault storm on the next + /// decode. The two mechanisms want opposite things about the same bytes, and + /// the pool is the one that has a measured job. + /// + /// Deliberately does NOT suppress [trimNow]: that fires at folder switch, + /// after the caches are already evicted and with nothing about to be + /// re-read, which is the one moment where releasing pages is unambiguously + /// right. + static bool suppressed = false; + static Timer? _idleTimer; static DateTime? _lastTrimAt; static bool _resolved = false; @@ -105,6 +122,13 @@ class WorkingSetTrim { /// on any platform, at any frequency. Never throws. static void request() { debugRequestCalls++; + if (suppressed) { + // Cancel any timer armed before suppression turned on, so a trim already + // in flight cannot land after the pool took ownership. + _idleTimer?.cancel(); + _idleTimer = null; + return; + } _idleTimer?.cancel(); _idleTimer = Timer(idleDelay, () { _idleTimer = null; @@ -134,6 +158,7 @@ class WorkingSetTrim { debugRequestCalls = 0; debugTrimNowCalls = 0; debugTrimAttempts = 0; + suppressed = false; } static bool _performTrim({required bool bypassRateLimit}) { diff --git a/lib/services/platform/working_set_trim_stub.dart b/lib/services/platform/working_set_trim_stub.dart index 3e35086..412ad87 100644 --- a/lib/services/platform/working_set_trim_stub.dart +++ b/lib/services/platform/working_set_trim_stub.dart @@ -34,6 +34,10 @@ class WorkingSetTrim { @visibleForTesting static int debugTrimAttempts = 0; + /// Surface parity with the real implementation (see its dartdoc). Nothing + /// here ever trims, so the flag changes no behaviour on this target. + static bool suppressed = false; + static bool get isSupported => false; static void request() {} @@ -48,5 +52,6 @@ class WorkingSetTrim { debugRequestCalls = 0; debugTrimNowCalls = 0; debugTrimAttempts = 0; + suppressed = false; } } diff --git a/test/services/image_pipeline/pool_wiring_test.dart b/test/services/image_pipeline/pool_wiring_test.dart new file mode 100644 index 0000000..1a1c6c4 --- /dev/null +++ b/test/services/image_pipeline/pool_wiring_test.dart @@ -0,0 +1,78 @@ +import 'package:ceyx/ceyx.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:halcyon_flutter/services/image_pipeline/dng_decode_service.dart'; +import 'package:halcyon_flutter/services/platform/working_set_trim.dart'; + +/// R6 pool activation (Task #9), Halcyon half: the host app must actually +/// ASSIGN the buffer pool, because ceyx leaves `CeyxDecodePool.nativeBufferPool` +/// null by default and every pooled-route gate short-circuits on that null. +/// +/// Before this wiring the whole WP6/WP10 pooled route was reachable only from +/// ceyx's own tests: production decodes fell back to the legacy native +/// allocator and nothing was red anywhere. That is the failure this file +/// exists to make impossible to reintroduce. +void main() { + setUp(WorkingSetTrim.debugReset); + tearDown(WorkingSetTrim.debugReset); + + test( + 'R6-AC2: the production init assigns the shared native buffer pool', + () { + ensureHalcyonDecodePoolConfigured(); + + expect( + CeyxDecodePool.nativeBufferPool, + isNotNull, + reason: + 'a null pool disables the pooled decode route silently — every ' + 'decode falls back to the legacy native allocator and no test ' + 'anywhere goes red', + ); + expect( + identical(CeyxDecodePool.nativeBufferPool, CeyxNativeBufferPool.shared), + isTrue, + reason: + 'the process-wide instance is the one sized against the host ' + 'budget; a second pool would double the slot bound', + ); + }, + ); + + test( + 'R6-AC3: the idle working-set trim is suppressed once the pool route owns ' + 'native buffers, while the folder-switch trim still runs', + () async { + ensureHalcyonDecodePoolConfigured(); + + expect( + WorkingSetTrim.suppressed, + isTrue, + reason: + 'idle-delayed trimming pages out exactly the idle pooled slots the ' + 'pool keeps resident for immediate reuse', + ); + + // The idle path must not reach the platform call at all -- not even the + // rate-limit bookkeeping, which is what `debugTrimAttempts` counts. + // + // Real time, not FakeAsync: the delay must be long enough that an + // UNsuppressed request would have fired its zero-delay timer by now, + // otherwise this assertion could not fail and would prove nothing. + WorkingSetTrim.idleDelay = Duration.zero; + WorkingSetTrim.request(); + await Future.delayed(const Duration(milliseconds: 50)); + expect(WorkingSetTrim.debugTrimAttempts, 0); + + // The folder-switch trim is a different event: it fires after the caches + // have already been evicted and nothing is about to be re-read, so it + // stays enabled. Suppressing it too would be a bigger change than the + // ruling asked for. + WorkingSetTrim.trimNow(); + expect( + WorkingSetTrim.debugTrimAttempts, + 1, + reason: 'trimNow is deliberately NOT suppressed', + ); + }, + ); +}