From e097078f7a89695c58cc98cc19b0d5f65de8faa1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 18:27:52 -0400 Subject: [PATCH 01/20] docs(spec): the Tiles wall as a persistent source (slice 2, #448) Answers #447 (T5.1) by naming exactly which #419 foundations slice 2 consumes, and makes #448 the first real consumer so those foundations can land on main wired rather than as an unwired island. The residual defect: adoptSettledFrom only hands a wall across when every tile is atRest, so a wall taken MID-ANIMATION still re-animates. With two per-bus animators there is no correct answer to that, which is why the fix is structural -- one animator per wall, one texture, buses sample it. Owner decisions recorded in the spec: the wall registers in SourceRegistry under a composed kind (not a capture kind, whose subscription/availability fields are meaningless for a wall); the plan carries ONE tiles-wall layer rather than collapsing expanded tile layers inside the D3D adapter, which pulls some Metal and CPU-preview parity forward deliberately; and the multiview PVW cell moves to sampling the Preview bus texture the way the PGM cell already samples Program. Identity was verified rather than assumed: the shell already emits tiles: (TilesLayerPayloadBuilder.cs:50) and the core takes it verbatim (MediaCore.cpp:1808), so dropping today's redundant sceneId prefix preserves the exact equivalence classes. Gates are recorded with their honest state: the iGPU budget number CANNOT be produced here (needs #425) and Metal will compile and pass CI but not be executed. Neither is quietly skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- ...-12-tiles-wall-persistent-source-design.md | 252 ++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md diff --git a/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md new file mode 100644 index 00000000..313648fb --- /dev/null +++ b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md @@ -0,0 +1,252 @@ +# The Tiles wall as a persistent source (persistent-sources slice 2) + +**Status:** approved in conversation 2026-09-12; not implemented. +**Issue:** [#448](https://github.com/iamfatness/CoreVideoPro/issues/448) (T5.2). +**Answers:** [#447](https://github.com/iamfatness/CoreVideoPro/issues/447) (T5.1) — +which #419 foundations slices 2-3 build on. +**Parent spec:** `docs/superpowers/specs/2026-09-10-persistent-sources-design.md` +(slice 2 of its section 5 phasing). + +## 1. Why + +A Tiles wall cued in Preview and taken to Program re-animates on the cut. The +owner's words, live show 2026-09-09: + +> I am ok if panelists leave and join the video but what I can't have is a total +> rerender from what is in preview to program like it is loading for the first +> time. + +That report produced a **partial** fix, which is what ships today: +`TilesPlanAnimation::adoptSettledFrom` MOVES spring state from the preview +animator to the program animator on the take tick. It is scoped hard — exact wall +key match, and **every sampled tile must be `atRest`**: + +```cpp +// compositor/TilesPlanAnimation.h +for (const auto& tile : previous.sampled_) { + if (!tile.atRest) return false; +} +``` + +So a wall taken **mid-animation still re-animates**. The hand-off refuses it +deliberately — "mid-flight state belongs to the bus that is flying it" — because +with two animators there is no correct answer. That residual defect is #448. + +The structural cause is that the wall's animation lives on the **bus** +(`MediaCore::programTilesAnimation_`, `MediaCore::previewTilesAnimation_`), so a +cut is a hand-over between two owners rather than a change of who is looking at +one object. The parent spec's model says a bus owns no source state: "A cut +changes which stack a bus samples and nothing else." + +This slice also carries the **first real consumer** of the #419 realtime +architecture foundations, so those foundations can land on `main` wired rather +than as an unwired island (CLAUDE.md: "A #419 architecture foundation lands on +`main` only together with its first real consumer"). + +## 2. The model and identity + +**The wall becomes a source.** A `TilesWallSource` owns three things that live on +the buses today: + +- the spring animator (`compositor::TilesAnimator`), +- one transparent, canvas-sized texture, +- an input signature: admitted members and their frame ids, wall settings, + canvas dimensions. + +It re-composites **only** when the signature changes. A settled wall on static +input does no GPU work. + +**Identity is the Tiles layer id, which is already `tiles:`.** + +Verified, not assumed: the shell builds it as +`LayerId: $"tiles:{scene.Id}"` (`TilesLayerPayloadBuilder.cs:50`) and the core +takes it verbatim off the wire (`tiles.layerId = node.getString("layerId")`, +`MediaCore.cpp:1808`). So it is unique per scene and already carries the +`tiles:` prefix the parent spec's `tiles:` calls for. + +Today's animation key is `sceneId + ":" + layerId`, i.e. `:tiles:` +— the scene id twice. Dropping the redundant prefix yields exactly the same +equivalence classes: a gallery settled in Preview and the same gallery a Take +puts on Program still share one identity (which is why `adoptSettledFrom` can +match them at all), and two different scenes' walls remain distinct. This slice +therefore changes **continuity**, not **which walls are the same wall**. + +A consequence worth stating: because identity is scene-derived, editing a scene +in place keeps one wall, while a different scene is always a different wall. + +Cross-scene wall sharing — one wall referenced by two different scenes — is the +parent spec's "placeable wall source" and is explicitly **out of scope**. + +**Registration.** `SourceRegistry` (from #419) gains a composed kind. A wall +registers on first scene reference and is released when no scene references it, +per the parent spec's Lifetime rule ("A source referenced by no scene is released +after a short grace period"). + +The registry's existing fields are capture-oriented — `externalId`, +`subscriptionRequested`, `subscriptionObserved`, `availability` — and are +meaningless for a composed source. The composed kind carries only `Token`, +`displayName`, `format` and publication state. Five inert fields on a type is how +a registry starts lying about what it knows; the type says what applies. + +**What this deletes:** `adoptSettledFrom`, `programTilesAnimation_`, +`previewTilesAnimation_`, and the take-tick hand-off in +`MediaCore::renderSyntheticTick` (`MediaCore.cpp:6266-6277`). With one animator +there is nothing to hand over — which is precisely why a wall taken mid-animation +becomes continuous. + +**What `AtomicTakeCoordinator` buys.** The cut becomes one atomic revision +transition, so "which bus samples this wall" changes at a single defined instant. +That also closes the race CLAUDE.md documents on the take record: + +> If a repeating spine sync applies the swapped Preview (the outgoing scene) +> BEFORE that `load-scene-graph` lands, the "outgoing Preview plan" is already +> the new one and the before-union can miss incoming sources. + +## 3. Components and data flow + +Three units, each understandable and testable alone. + +| Unit | Owns | Depends on | +|---|---|---| +| `core/TilesWallSource` | animator, input signature, and the decision "does this wall need re-compositing this tick?" | nothing GPU — pure, unit-testable | +| `SourceRegistry` (composed kind) | the wall's `Token` and lifetime by scene reference | #419 | +| compositor adapters | a `wallTextures_` map keyed by `wallId` -> render target + SRV | per-backend | + +Per render tick: + +1. The gather asks each referenced wall's source whether its signature changed. +2. If so, the adapter re-composites that wall into its own texture. +3. Each bus that references the wall draws that texture as **one layer**. + +**The plan carries one wall layer.** The render plan emits a single +`tiles-wall:` layer instead of N expanded `tile:` layers. This is a +deliberate choice over collapsing the expanded layers inside the D3D adapter: +grouping layers by wall membership inside a backend is inference that rots, and +slice 3 (transitions blending finished bus images) wants the bus-image shape +anyway. + +The cost is that **Metal and the CPU preview must implement the new layer kind in +this slice**, pulling some slice-4 parity work forward. That is accepted. + +**Drawing rides machinery that already exists.** `ResolvedLayer::retainedProgram` +is an `ID3D11ShaderResourceView*` already used to draw the delivered Program +texture into the multiview PGM cell (`D3D11CompositorAdapter.cpp:323-340`). The +wall texture and the PVW cell below both use that same path. + +**The multiview PVW cell samples the Preview bus texture.** Today it +re-composites the whole preview stack into the PVW sub-rect +(`MediaCore::buildMultiviewRenderPlan`, the `hasPreviewScene()` branch), remapping +every preview layer. It will instead sample the preview composite the core already +produces, the way the PGM cell already samples the delivered Program texture. This +is the parent spec's "Also" item, included here at the owner's direction. + +**Threading is unchanged.** Everything stays on the single render thread and +immediate context. No second device. No pixel work under `coreMutex` or on a hot +tick. + +## 4. Invariants this must not break + +Every rule here was written after something broke on air. They are constraints on +the design, not advice. + +1. **An empty render plan is not "draw nothing."** All three compositors + improvise a full-canvas grid per decoded frame when a plan has zero layers — + on PROGRAM, which the virtual camera, recordings and streams inherit. The + wall's background layer is emitted **above** the admission gate today and must + stay there. Collapsing to one layer must never make an all-stale wall emit + zero layers. +2. **`wall.present` ALONE decides whether a wall is active** — never + `present && !members.empty()`. `members: []` is an ordinary state (every + camera off, or a momentarily empty roster), and a members-aware gate re-opened + the on-air hole once already. +3. **The stale rules are asymmetric and stay that way.** A stale *tile* is + refused — it occupies a slot and would seat a dead guest. A stale *background* + is held (`compositor::tilesBackgroundSourceIsDrawable`) — a backdrop frozen + for a beat is invisible, where its absence is a full-frame colour change on + air. `kTilesStaleFrameMs` (1500 ms) does not move: it is shared with tile + admission and changing it changes wall membership for every source. +4. **The all-stale transient must not wipe retained tiles.** Today's guard is + `if (targets.empty() && !sampled_.empty()) return;`. With one animator this + matters MORE, not less: a single wipe would now affect every bus at once. +5. **Ordering gets structurally safer, and that is worth keeping.** Today routes + and the wall share one order namespace (tiles-bg at `wall.order`, tile *i* at + `wall.order + 1 + i`), so a surviving gallery route at order 2 can composite + BETWEEN tiles. One wall layer makes that impossible by construction. +6. **The `tiles` snapshot node stays published unconditionally.** A node that + vanishes in exactly the case worth detecting is the multiviewer mistake. +7. **Borders stay multiview-only.** Route layers are forced to + `borderStyle="none"`; a wall layer must not become a new way to composite an + adornment into Program. + +**Failure handling.** If a wall texture cannot be created (device loss, resource +exhaustion), the wall draws its **background but not its tiles**, and says so +through `sceneValidationWarnings_` — never a silent empty layer set, never an +improvised grid. Wall textures are per device generation: they retire with the +generation and are rebuilt on adoption, like every other per-generation resource +under `DeviceLossPolicy`. + +## 5. Proof + +"Nothing re-rendered" is measured, not eyeballed. + +**The headline test, and it must be RED first.** +`AWallTakenMidAnimationIsContinuous` — cue a wall in Preview, Take it **while +tiles are still flying**, assert the wall's generation does not change and +sampled tile positions continue rather than restart. Today this must FAIL: +`adoptSettledFrom` refuses a non-settled wall by design, so the program animator +resets. **The red is verified by reverting the change, not by assuming it** — +three tests in the 2026-09-12 session initially passed without their fix. + +1. **Generation counters.** The wall carries `generation` (bumped on animator + reset or texture recreate) and `lastFrameId`, and rides the take record + through the existing `core/SourceContinuityLedger.h`. A take record cannot + read `cut` while the wall's generation moved. +2. **Pixel continuity.** `ProgramPixelContinuityTest.cpp` gains a wall case using + the luma-comb technique. A re-animation is a luma discontinuity on the ticks + around the Take and fails regardless of what the counters say. +3. **Live soak.** `scripts/qa/live-meeting-soak.mjs --takes N` against a real + meeting. **It has never been run** (CLAUDE.md records this), so slice 2 is the + first time; expect it to surface its own problems. + +**Rewritten, never deleted** — these pin today's per-bus contract and must pin the +one-animator contract instead: `TilesRenderPlanTest.cpp:519,916-1081`, +`TilesAnimatorTest.cpp:97-163`, `RenderedSceneAttributionTest.cpp:196`. + +**New, for the #419 wiring:** `SourceRegistry` accepting and releasing a composed +wall by scene reference; the Take running through `AtomicTakeCoordinator` as one +revision transition, including the interleaving race it closes. + +## 6. What lands, and what does not + +**The carve.** Only the #419 foundations this slice actually consumes come to +`main`, together with slice 2, as one reviewable change: `SourceRegistry` (plus +the composed kind), `AtomicTakeCoordinator`, and their tests. The rest of #419 +stays a draft and shrinks as later slices land. + +**Gates, and their honest state:** + +| Gate | Status | +|---|---| +| `MonitorRenderFaultInjection` on a quiet machine | can be run here before merge | +| Integrated-GPU budget number | **CANNOT be produced** — needs the reference machines in #425. Recorded as an unmet gate, not skipped quietly | +| Metal parity | compiles and passes CI's `native-metal-macos`; **not executed** — no Mac here, and the M3 Max is QA-only | + +**Out of scope:** the placeable wall source and scene-as-source; per-layer in/out +animation; scene UI changes; lower-thirds and graphics as composed sources +(slice 3); full Metal and CPU-preview parity beyond the new layer kind (slice 4). + +## 7. Risks + +- **This slice touches the render thread**, which is the highest-consequence code + in the product. Fault-injection tests run on a quiet machine before merge; a + loaded machine cannot gate a timing property at any threshold. +- **The one-layer plan change reaches macOS**, which cannot be executed here. + A Metal regression would be invisible until someone runs the M3 Max. +- **The live soak is unproven tooling.** Its first real run is part of this slice, + so a soak failure may be the harness rather than the product — that must be + diagnosed, not assumed either way. +- **The 16-decoder cap** (`OwnedMediaFrameSource.h`) becomes a real limit as + sources outlive buses. It must warn loudly, never drop silently. +- **Scope grew during design, deliberately.** The owner chose option C (wall + source + PVW cell) and plan shape (ii) (one wall layer, absorbing parity work). + Both are recorded here so the size is not a surprise at review. From e84c13b829912c9419910ee4dda3e08c262b9961 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 18:33:10 -0400 Subject: [PATCH 02/20] docs(spec): composed sources use nullopt, not a struct split (owner ruling) The first draft said the composed kind "carries only Token, displayName, format and publication state", which implied splitting SourceRegistry::Source into a common core plus a capture payload. Investigating the cost changed the answer. SourceRegistry::Source is one struct with a kind discriminator, and about five of its fields are meaningless for a wall: no personId, no externalId (there is no SDK handle), it cannot be Departed (a wall is released when unreferenced), and it is never subscribed. Two findings made this more than cosmetic: - The defaults are ASSERTIONS. subscriptionObserved{false} is initialised engaged, and its own comment says nullopt is what means unknown -- so a registered wall would assert "subscription observed = false". That is the shape of #468 and of "absent lifecycle means UNKNOWN, never healthy". - This is a PUBLISHED, golden-tested contract, not an internal struct: test/data/wave1-authority.json is authority-goldens-v2 with ~1,700 lines of scenarios, and a serialized source carries personId, videoAvailable, videoFresh, audioAvailable, audioFresh, audioMuted. Registering a wall forces an answer to "is this wall's audio muted?". Splitting the struct was therefore rejected on cost: it touches every existing #419 consumer and its tests and reshapes those goldens -- #419 surgery smuggled in under a Tiles-wall fix, when the owner asked for a minimal carve. Leaving the fields as-is was rejected too: a field that serializes a false claim is how #468 happened. The fields become std::optional and stay nullopt for composed sources, the serializer omits them, and two new tests pin it: a wall never claims a subscription state, and the existing capture goldens stay byte-identical. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- ...-12-tiles-wall-persistent-source-design.md | 66 ++++++++++++++++--- 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md index 313648fb..f500e6a1 100644 --- a/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md +++ b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md @@ -82,11 +82,51 @@ registers on first scene reference and is released when no scene references it, per the parent spec's Lifetime rule ("A source referenced by no scene is released after a short grace period"). -The registry's existing fields are capture-oriented — `externalId`, -`subscriptionRequested`, `subscriptionObserved`, `availability` — and are -meaningless for a composed source. The composed kind carries only `Token`, -`displayName`, `format` and publication state. Five inert fields on a type is how -a registry starts lying about what it knows; the type says what applies. +**The non-applying fields become `std::optional` and are left `nullopt` for a +composed source. They are NOT deleted, and the struct is NOT split.** + +`SourceRegistry::Source` is one struct with a `kind` discriminator, so every +source carries every field. About five are meaningless for a wall: it has no +`personId`, no `externalId` (there is no SDK handle), it cannot be `Departed` +(a wall is *released when unreferenced* — a different lifecycle), and it is never +subscribed, so neither `subscriptionRequested` nor `subscriptionObserved` applies. + +This is not cosmetic, for two reasons. + +**The defaults are assertions, not blanks.** `std::optional +subscriptionObserved{false}` is initialised ENGAGED, and the comment beside it +says `nullopt` is what means "unacknowledged/unknown". A registered wall would +therefore assert *"subscription observed = false"* rather than "not applicable". +That is the shape of #468, where a field asserted a state nothing had +established, and of the standing rule "absent lifecycle means UNKNOWN, never +healthy". + +**And this is a published, golden-tested contract**, not an internal struct: +`test/data/wave1-authority.json` is `authority-goldens-v2` with ~1,700 lines of +scenarios, and a serialized source reads +`{sourceId, instanceId, processEpoch, generation, personId, videoAvailable, +videoFresh, audioAvailable, audioFresh, audioMuted}`. Registering a wall forces +an answer to "is this wall's audio muted? is its video fresh? who is its person?", +and whatever is answered lands in the goldens and on the wire, where the shell's +`NativeSourceAuthority` reads it. + +**Why `nullopt` rather than splitting the struct.** Splitting `Source` into a +common core plus a `CaptureDetails` payload is the tidier type, and it was this +spec's first draft. It was rejected on cost: it touches every existing #419 +consumer and its tests (`NativeSourceAuthorityTests`, `SourceAuthorityAdmissionTests`) +and reshapes ~1,700 lines of golden scenarios — #419 surgery smuggled in under a +Tiles-wall fix, which is not the minimal carve this slice is supposed to be. +`nullopt` buys the honesty (a wall never claims a subscription state) at a +fraction of the churn, and the split stays available if a later slice needs it. + +Leaving the fields as-is and documenting that composed sources ignore them was +also rejected: a field that serializes a false claim is exactly how #468 +happened. + +**Consequences to hold to:** the serializer omits `nullopt` fields rather than +emitting nulls or defaults, so existing capture goldens are unchanged; and any +registry query that means "unsubscribed" or "departed" must treat `nullopt` as +NOT-APPLICABLE, never as false. **What this deletes:** `adoptSettledFrom`, `programTilesAnimation_`, `previewTilesAnimation_`, and the take-tick hand-off in @@ -212,9 +252,19 @@ three tests in the 2026-09-12 session initially passed without their fix. one-animator contract instead: `TilesRenderPlanTest.cpp:519,916-1081`, `TilesAnimatorTest.cpp:97-163`, `RenderedSceneAttributionTest.cpp:196`. -**New, for the #419 wiring:** `SourceRegistry` accepting and releasing a composed -wall by scene reference; the Take running through `AtomicTakeCoordinator` as one -revision transition, including the interleaving race it closes. +**New, for the #419 wiring:** + +- `SourceRegistry` accepting and releasing a composed wall by scene reference. +- **`AComposedWallNeverClaimsASubscriptionState`** — a registered wall reports + `personId`, `externalId`, `availability`, `subscriptionRequested` and + `subscriptionObserved` as `nullopt`, and the serializer OMITS them rather than + emitting `null` or `false`. This is the test that stops the `nullopt` decision + decaying back into a false claim. +- **The existing capture goldens are byte-identical.** `wave1-authority.json` must + not change: making a field optional may not alter what a capture source emits. + If the goldens move, the change is wrong. +- The Take running through `AtomicTakeCoordinator` as one revision transition, + including the interleaving race it closes. ## 6. What lands, and what does not From d1e99bb6ec2a793d297611f1c45d698748b9e506 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 18:41:05 -0400 Subject: [PATCH 03/20] docs(spec): correct two wrong facts the spec was approved on Found while gathering detail for the implementation plan. Both errors were mine, and one of them was the argument AGAINST the option I talked the owner out of. 1. The spec claimed SourceRegistry::Snapshot is a published, golden-tested wire contract. IT IS NOT. That snapshot is consumed in-process by ShowPlanGenerator and SceneVersionShadow and never reaches the wire or the session state. The authority-goldens-v2 scenarios belong to ZoomSourceAuthorityAdapter::Observation::Source -- the Zoom ROSTER OBSERVATION synced INTO the registry, carrying videoFresh/audioMuted/personId. A wall registered directly in SourceRegistry never appears there. 2. Because of (1), the stated cost of splitting the struct ("reshapes ~1,700 lines of goldens") was fabricated. Measured: about seven files in the Zoom authority path plus tests. Real but modest. The conclusion still stands after re-deciding with correct facts (owner: keep the nullopt approach), but on NARROWER grounds: subscriptionObserved{false} is initialised engaged while its own comment says nullopt means unknown, so a registered wall asserts "subscription observed = false" -- and that feeds PLAN GENERATION. The split is deferred to slice 3, where more than one composed kind would justify the shape. Also records a finding that applies whichever option is chosen: validRegistration requires a non-empty externalId, so add() returns Invalid for a wall today, and externalConflict matches kind+processEpoch+externalId, so two walls with empty external ids would collide. Both now have named tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- ...-12-tiles-wall-persistent-source-design.md | 65 +++++++++++++------ 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md index f500e6a1..d20e3c78 100644 --- a/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md +++ b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md @@ -101,28 +101,47 @@ That is the shape of #468, where a field asserted a state nothing had established, and of the standing rule "absent lifecycle means UNKNOWN, never healthy". -**And this is a published, golden-tested contract**, not an internal struct: -`test/data/wave1-authority.json` is `authority-goldens-v2` with ~1,700 lines of -scenarios, and a serialized source reads -`{sourceId, instanceId, processEpoch, generation, personId, videoAvailable, -videoFresh, audioAvailable, audioFresh, audioMuted}`. Registering a wall forces -an answer to "is this wall's audio muted? is its video fresh? who is its person?", -and whatever is answered lands in the goldens and on the wire, where the shell's -`NativeSourceAuthority` reads it. +**And that false claim feeds PLAN GENERATION.** `SourceRegistry::Snapshot` is +consumed in-process by `ShowPlanGenerator` and `SceneVersionShadow`. A wall +asserting "subscription observed = false" is therefore an input to how shows are +planned, not a cosmetic field. + +*(Correction, recorded because this spec was first approved on a wrong fact: an +earlier draft claimed the registry snapshot is a published, golden-tested wire +contract. It is NOT. `SourceRegistry::Snapshot` never reaches the wire or the +session state. The `authority-goldens-v2` scenarios in +`test/data/wave1-authority.json` belong to +`ZoomSourceAuthorityAdapter::Observation::Source` — the Zoom ROSTER OBSERVATION +that is synced INTO the registry, carrying `videoFresh` / `audioMuted` / +`personId`. A wall registered directly in `SourceRegistry` never appears there. +The in-process consequence above is the real argument, and it is the narrower +one.)* **Why `nullopt` rather than splitting the struct.** Splitting `Source` into a common core plus a `CaptureDetails` payload is the tidier type, and it was this -spec's first draft. It was rejected on cost: it touches every existing #419 -consumer and its tests (`NativeSourceAuthorityTests`, `SourceAuthorityAdmissionTests`) -and reshapes ~1,700 lines of golden scenarios — #419 surgery smuggled in under a -Tiles-wall fix, which is not the minimal carve this slice is supposed to be. -`nullopt` buys the honesty (a wall never claims a subscription state) at a -fraction of the churn, and the split stays available if a later slice needs it. +spec's first draft. Measured cost of that split: about seven files, all in the +Zoom authority path (`SourceRegistry`, `ZoomSourceAuthorityAdapter`, +`ZoomRuntimeAuthorityBridge`, `ZoomEngineRuntime`) plus their tests. That is +real but modest — **an earlier draft wrongly claimed it also reshaped ~1,700 +lines of goldens; it does not.** The split was still declined for this slice +because it is #419 surgery inside a Tiles-wall change, and the owner asked for a +minimal carve. It stays available, and slice 3 (lower-thirds and graphics as +composed sources) is the natural place to revisit it, with more than one composed +kind to justify the shape. Leaving the fields as-is and documenting that composed sources ignore them was -also rejected: a field that serializes a false claim is exactly how #468 +rejected: a field that asserts a state nothing established is exactly how #468 happened. +**Two registration rules must gain composed handling, whichever option is +chosen.** `SourceRegistry::validRegistration` currently requires +`!r.externalId.empty()`, so `add()` returns `Result::Invalid` for a wall, which +has no SDK handle. And `externalConflict` matches on +`kind + processEpoch + externalId`, so two walls with empty external ids would +collide as duplicates. A composed registration is identified by its `sourceId` +alone; `externalId` is not required and not compared. A wall's `processEpoch` is +the CORE's process epoch — walls do not outlive the core. + **Consequences to hold to:** the serializer omits `nullopt` fields rather than emitting nulls or defaults, so existing capture goldens are unchanged; and any registry query that means "unsubscribed" or "departed" must treat `nullopt` as @@ -257,12 +276,16 @@ one-animator contract instead: `TilesRenderPlanTest.cpp:519,916-1081`, - `SourceRegistry` accepting and releasing a composed wall by scene reference. - **`AComposedWallNeverClaimsASubscriptionState`** — a registered wall reports `personId`, `externalId`, `availability`, `subscriptionRequested` and - `subscriptionObserved` as `nullopt`, and the serializer OMITS them rather than - emitting `null` or `false`. This is the test that stops the `nullopt` decision - decaying back into a false claim. -- **The existing capture goldens are byte-identical.** `wave1-authority.json` must - not change: making a field optional may not alter what a capture source emits. - If the goldens move, the change is wrong. + `subscriptionObserved` as `nullopt`. This is the test that stops the `nullopt` + decision decaying back into a false claim. +- **`ShowPlanGeneratorTreatsNulloptAsNotApplicable`** — plan generation must not + read an absent subscription as an unsubscribed source. This is the consequence + that actually matters: the registry snapshot is a plan-generation input. +- **`AWallIsAdmittedWithoutAnExternalId`** and **`TwoWallsWithNoExternalIdDoNotCollide`** + — pinning the two registration rules above, which reject or merge walls today. +- **The Zoom authority goldens are byte-identical.** `wave1-authority.json` must + not change: making a registry field optional must not alter what the Zoom + observation path emits. If those goldens move, the change has leaked. - The Take running through `AtomicTakeCoordinator` as one revision transition, including the interleaving race it closes. From d1d3f8ade6343dfd3dd0b2386025f9276a31236d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 18:45:48 -0400 Subject: [PATCH 04/20] =?UTF-8?q?docs(plan):=20plan=201=20of=202=20for=20s?= =?UTF-8?q?lice=202=20=E2=80=94=20one=20animator=20per=20Tiles=20wall=20(#?= =?UTF-8?q?448)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four tasks, each ending in an independently testable deliverable: 1. SourceRegistry admits a composed source and never claims a subscription state for it (both current rules reject a wall today: validRegistration demands a non-empty externalId, and externalConflict would merge two walls that both have none). 2. TilesWallSource owns one wall's animation, keyed by wall id. 3. MediaCore uses one animator per wall and adoptSettledFrom is DELETED. This is the task that fixes #448. 4. The wall registers as a composed source and the take record proves its continuity with a generation. Split from the spec deliberately. Plan 1 changes NO drawing, so it can land while beta-2026-09-12-c425e3c is still being live-checked; plan 2 (wall texture, one tiles-wall layer, Metal/CPU parity, PVW cell, AtomicTake) changes the render path and should wait for that beta to be shaken out. Each plan produces working, testable software on its own. Self-review caught a type inconsistency worth recording: the first draft had TilesPlanAnimation calling a std::function installed by TilesWallSource, which Task 2 never installed AND which would allocate on the render tick. Replaced by advance() returning bool "did I reset", which the wrapper turns into a generation bump -- so the generation cannot move without the animator moving, and vice versa. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- .../2026-09-12-tiles-wall-one-animator.md | 776 ++++++++++++++++++ 1 file changed, 776 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md diff --git a/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md b/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md new file mode 100644 index 00000000..2506d93e --- /dev/null +++ b/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md @@ -0,0 +1,776 @@ +# Tiles wall: one animator per wall (slice 2, plan 1 of 2) + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the Tiles wall's animation belong to the wall instead of to each bus, so a wall taken to Program **mid-animation** is continuous — and register the wall in `SourceRegistry`, making #448 the first real consumer of the #419 foundations. + +**Architecture:** Today `MediaCore` holds two `TilesPlanAnimation` objects, one per bus, and hands settled state across on the take tick. This plan replaces them with one `TilesWallSource` per wall, owned by a `TilesWallSources` map keyed by wall id, and deletes the hand-off. The wall also registers in `SourceRegistry` under a new composed kind whose capture-only fields stay `nullopt`. **No drawing changes** — the plan still emits expanded `tile:` layers, so no compositor is touched. + +**Tech Stack:** C++20, MSVC; GoogleTest (`native/tests`); CMake target `corevideo-native-tests`. + +**Spec:** `docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md` + +## Global Constraints + +- **Windows build:** `cmake --build native/build-dev --target corevideo-native-tests` with `ZOOM_SDK_DIR` set to the staged SDK x64 dir. Run the binary the build just wrote: `native/build-dev/corevideo-native-tests.exe`. **Do not** run `native/build-dev/Release/` — nothing updates it. +- **Run one test:** `native/build-dev/corevideo-native-tests.exe --gtest_filter='Suite.Name'`. **Negative filters do not work** in this runner; to skip, run the full suite. +- **Full suite must stay green:** 931 tests as of `c425e3ca`, plus what this plan adds. +- **Escape scanner must pass:** `python scripts/qa/check-string-escapes.py`. MSVC tolerates invalid escapes that GCC/Clang reject; this scanner is what stops a Linux/macOS CI break. +- **`kTilesStaleFrameMs` (1500 ms) must not change.** It is shared with tile admission; moving it changes wall membership for every source. +- **Never do pixel work under `coreMutex` or on a hot tick.** This plan adds no I/O and no allocation on the render tick. +- **Red before green is verified by reverting**, never by assuming. Three tests in the 2026-09-12 session passed without their fix. + +--- + +### Task 1: `SourceRegistry` admits a composed source + +**Files:** +- Modify: `native/src/core/SourceRegistry.h` (the `Kind` enum; the `Source` and `Registration` field types) +- Modify: `native/src/core/SourceRegistry.cpp:49-71` (`validRegistration`, `externalConflict`) +- Test: `native/tests/SourceRegistryComposedTest.cpp` (create) +- Modify: `native/CMakeLists.txt` (register the new test file) + +**Interfaces:** +- Consumes: nothing. +- Produces: `SourceRegistry::Kind::Composed`; `Registration` accepted with an empty `externalId` when `kind == Kind::Composed`; `Source::personId`, `Source::externalId`, `Source::availability`, `Source::subscriptionRequested`, `Source::subscriptionObserved` are `std::optional` and left `nullopt` for composed sources. + +**Background the implementer needs.** `SourceRegistry` lives on the `#419` branch (`origin/codex/production-realtime-architecture`), not on `main`. This plan assumes the minimal carve described in the spec §6 has already been cherry-picked onto the working branch: `SourceRegistry.{h,cpp}` and their existing tests. If it has not, do that first as a separate commit with no behaviour change. + +Two current rules reject a wall. `validRegistration` requires a non-empty `externalId`: + +```cpp +// SourceRegistry.cpp:56 +!retiredProcessEpochs_.contains(r.processEpoch) && !r.externalId.empty() && +``` + +and `externalConflict` treats two sources with the same `kind + processEpoch + externalId` as duplicates, so two walls with empty external ids would collide: + +```cpp +// SourceRegistry.cpp:68-70 +if (entry.first != r.sourceId.value && source.availability != Availability::Departed && + source.kind == r.kind && source.token.processEpoch == r.processEpoch && source.externalId == r.externalId) + return true; +``` + +A wall has no SDK handle. It is identified by its `sourceId` alone. + +- [ ] **Step 1: Write the failing tests** + +Create `native/tests/SourceRegistryComposedTest.cpp`: + +```cpp +#include "core/SourceRegistry.h" + +#include + +namespace { +using corevideo::core::SourceRegistry; + +SourceRegistry::Registration wallRegistration(const std::string& id) { + SourceRegistry::Registration registration; + registration.sourceId = {id}; + registration.kind = SourceRegistry::Kind::Composed; + registration.displayName = "Gallery"; + // A wall lives and dies with the core process, so the core's epoch is its epoch. + registration.processEpoch = "core-epoch-1"; + // externalId deliberately left EMPTY: a wall has no SDK handle. + return registration; +} + +// A wall has no SDK handle, and validRegistration rejects an empty externalId +// today, so add() answers Invalid and the wall can never be registered. +TEST(SourceRegistryComposed, AWallIsAdmittedWithoutAnExternalId) { + SourceRegistry registry("registry-epoch-1"); + const auto mutation = registry.add(wallRegistration("tiles:scene-a")); + EXPECT_EQ(mutation.result, SourceRegistry::Result::Applied); + ASSERT_TRUE(mutation.token.has_value()); + EXPECT_EQ(mutation.token->sourceId.value, "tiles:scene-a"); +} + +// externalConflict matches on kind + processEpoch + externalId, so two walls +// that both have an EMPTY externalId look like duplicates of each other. +TEST(SourceRegistryComposed, TwoWallsWithNoExternalIdDoNotCollide) { + SourceRegistry registry("registry-epoch-1"); + ASSERT_EQ(registry.add(wallRegistration("tiles:scene-a")).result, + SourceRegistry::Result::Applied); + const auto second = registry.add(wallRegistration("tiles:scene-b")); + EXPECT_EQ(second.result, SourceRegistry::Result::Applied); +} + +// The load-bearing honesty test. subscriptionObserved is initialised ENGAGED +// with the value false, and its own comment says nullopt means unknown - so a +// registered wall would otherwise ASSERT "subscription observed = false" into +// ShowPlanGenerator, which consumes this snapshot. +TEST(SourceRegistryComposed, AComposedWallNeverClaimsASubscriptionState) { + SourceRegistry registry("registry-epoch-1"); + ASSERT_EQ(registry.add(wallRegistration("tiles:scene-a")).result, + SourceRegistry::Result::Applied); + + const auto snapshot = registry.snapshot(); + ASSERT_NE(snapshot, nullptr); + ASSERT_EQ(snapshot->sources.size(), 1U); + const auto& wall = snapshot->sources.front(); + + EXPECT_EQ(wall.kind, SourceRegistry::Kind::Composed); + EXPECT_FALSE(wall.personId.has_value()); + EXPECT_FALSE(wall.externalId.has_value()); + EXPECT_FALSE(wall.availability.has_value()); + EXPECT_FALSE(wall.subscriptionRequested.has_value()); + EXPECT_FALSE(wall.subscriptionObserved.has_value()); +} + +// A capture source is unchanged: it still carries every field it always did. +TEST(SourceRegistryComposed, ACaptureSourceStillCarriesItsCaptureFields) { + SourceRegistry registry("registry-epoch-1"); + SourceRegistry::Registration camera; + camera.sourceId = {"camera-alice"}; + camera.kind = SourceRegistry::Kind::ParticipantVideo; + camera.displayName = "Alice"; + camera.processEpoch = "zoom-process-1"; + camera.externalId = "alice-sdk-id"; + ASSERT_EQ(registry.add(camera).result, SourceRegistry::Result::Applied); + + const auto& source = registry.snapshot()->sources.front(); + ASSERT_TRUE(source.externalId.has_value()); + EXPECT_EQ(*source.externalId, "alice-sdk-id"); + ASSERT_TRUE(source.availability.has_value()); + EXPECT_EQ(*source.availability, SourceRegistry::Availability::Available); +} +} // namespace +``` + +Register it in `native/CMakeLists.txt` beside the other core tests: + +```cmake + tests/SourceRegistryComposedTest.cpp +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `native/build-dev/corevideo-native-tests.exe --gtest_filter='SourceRegistryComposed.*'` + +Expected: compile failure — `Kind::Composed` does not exist, and `personId`/`externalId`/`availability`/`subscriptionRequested`/`subscriptionObserved` have no `.has_value()`. That compile failure IS the red for this task; it names every field the implementation must change. + +- [ ] **Step 3: Add the composed kind and make the capture-only fields optional** + +In `native/src/core/SourceRegistry.h`: + +```cpp + // Composed: a source the CORE renders rather than captures (the Tiles wall + // today; lower-thirds and graphics in slice 3). It has no SDK handle, no + // person, and is never subscribed - so the capture-only fields below stay + // nullopt for it, and "nullopt" means NOT APPLICABLE, never false. + enum class Kind { ParticipantVideo, ParticipantShare, Device, Media, Browser, Composed }; +``` + +and in `struct Source`, change the five capture-only members: + +```cpp + struct Source { + Token token; + Kind kind = Kind::ParticipantVideo; + std::optional personId; + uint64_t personGeneration = 0; + std::string displayName; + // nullopt = NOT APPLICABLE to this kind. Never read as false. + std::optional externalId; + std::optional availability; + std::optional subscriptionRequested; + std::optional subscriptionObserved; + std::optional format; + bool hasPublication = false; + bool hasPublicationWatermark = false; + uint64_t publicationSequence = 0; + int64_t lastPublicationNs = 0; + }; +``` + +In `native/src/core/SourceRegistry.cpp`, teach both rules about composed sources: + +```cpp +bool SourceRegistry::validRegistration(const Registration& r) const { + const bool composed = r.kind == Kind::Composed; + const bool knownKind = composed || r.kind == Kind::ParticipantVideo || + r.kind == Kind::ParticipantShare || r.kind == Kind::Device || + r.kind == Kind::Media || r.kind == Kind::Browser; + // A composed source is identified by its sourceId alone: it has no SDK handle, + // so requiring an externalId would reject every wall outright. + const bool externalIdOk = composed + ? r.externalId.empty() + : (!r.externalId.empty() && r.externalId.size() <= 512); + return knownKind && externalIdOk && ... // rest of the existing expression unchanged +} +``` + +```cpp +bool SourceRegistry::externalConflict(const Registration& r) const { + // A composed source has no externalId to collide on; two walls are distinct + // whenever their sourceIds differ. + if (r.kind == Kind::Composed) { + return false; + } + ... // existing body unchanged +} +``` + +Then fix the assignment sites in `install()` so a composed registration leaves the five fields `nullopt` and a capture registration sets them exactly as before. Every other read of these fields in `ZoomSourceAuthorityAdapter.cpp`, `ZoomRuntimeAuthorityBridge.cpp` and `ZoomEngineRuntime.cpp` must be updated to dereference the optional — those paths only ever handle capture kinds, so `*source.externalId` is correct there; do **not** introduce a `value_or(false)`, which would recreate the false claim this task exists to remove. + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `native/build-dev/corevideo-native-tests.exe --gtest_filter='SourceRegistryComposed.*'` +Expected: 4 tests PASS. + +- [ ] **Step 5: Verify the Zoom authority goldens did not move** + +Run: `native/build-dev/corevideo-native-tests.exe` (full suite) +Expected: the pre-existing authority/golden tests PASS **unchanged**. Making a registry field optional must not alter what the Zoom observation path emits. If a golden moves, the change has leaked into the capture path — fix that rather than re-recording the golden. + +- [ ] **Step 6: Commit** + +```bash +git add native/src/core/SourceRegistry.h native/src/core/SourceRegistry.cpp \ + native/tests/SourceRegistryComposedTest.cpp native/CMakeLists.txt +git commit -m "feat(registry): admit composed sources, and never claim a subscription state for them" +``` + +--- + +### Task 2: `TilesWallSource` owns one wall's animation + +**Files:** +- Create: `native/src/core/TilesWallSource.h` +- Test: `native/tests/TilesWallSourceTest.cpp` (create) +- Modify: `native/CMakeLists.txt` + +**Interfaces:** +- Consumes: `compositor::TilesPlanAnimation` (existing, `native/src/compositor/TilesPlanAnimation.h`). +- Produces: + - `core::TilesWallSource` with + `void advance(modules::CompositorRenderPlan&, const std::string& wallId, bool present, bool enabled, double durationMs, double nowMs)`, + `void applyLatest(modules::CompositorRenderPlan&, const std::string& wallId) const`, + `uint64_t generation() const`, `void noteReset()`. + - `core::TilesWallSources` with `TilesWallSource& forWall(const std::string& wallId)`, + `const TilesWallSource* find(const std::string& wallId) const`, + `void releaseAllExcept(const std::vector& liveWallIds)`, `std::size_t size() const`. + - `compositor::TilesPlanAnimation::advance` changes return type from `void` to + `bool` — **true when it reset the animator**. This is how the generation is + bumped: no `std::function` callback, so nothing is allocated or indirected on + the render tick. + +**Why a generation.** The spec's proof (§5) requires that a take record cannot read `cut` while the wall's animation reset. `generation` is bumped whenever the animator is reset, so "did this wall restart?" is a number rather than an opinion. + +- [ ] **Step 1: Write the failing test** + +Create `native/tests/TilesWallSourceTest.cpp`: + +```cpp +#include "core/TilesWallSource.h" + +#include + +namespace { +using corevideo::core::TilesWallSources; + +// One wall id yields ONE source however many buses ask for it. This is the +// whole point of the slice: the animation belongs to the wall, not the bus. +TEST(TilesWallSources, BothBusesAskingForOneWallGetTheSameSource) { + TilesWallSources sources; + auto& fromProgram = sources.forWall("tiles:scene-a"); + auto& fromPreview = sources.forWall("tiles:scene-a"); + EXPECT_EQ(&fromProgram, &fromPreview); + EXPECT_EQ(sources.size(), 1U); +} + +TEST(TilesWallSources, DifferentWallsAreDifferentSources) { + TilesWallSources sources; + auto& a = sources.forWall("tiles:scene-a"); + auto& b = sources.forWall("tiles:scene-b"); + EXPECT_NE(&a, &b); + EXPECT_EQ(sources.size(), 2U); +} + +// Lifetime is "referenced by a scene", per the parent spec. A wall no scene +// references is released; a wall still referenced survives the sweep. +TEST(TilesWallSources, AWallNoSceneReferencesIsReleased) { + TilesWallSources sources; + sources.forWall("tiles:scene-a"); + sources.forWall("tiles:scene-b"); + + sources.releaseAllExcept({"tiles:scene-b"}); + + EXPECT_EQ(sources.size(), 1U); + EXPECT_EQ(&sources.forWall("tiles:scene-b"), &sources.forWall("tiles:scene-b")); +} + +// A generation that never moves proves nothing. It must move on a reset... +TEST(TilesWallSources, AResetBumpsTheGeneration) { + TilesWallSources sources; + auto& wall = sources.forWall("tiles:scene-a"); + const auto before = wall.generation(); + wall.noteReset(); + EXPECT_GT(wall.generation(), before); +} + +// ...and a released-then-recreated wall is a NEW wall, so its generation +// must not silently continue the old one's. +TEST(TilesWallSources, ARecreatedWallDoesNotInheritTheOldGeneration) { + TilesWallSources sources; + sources.forWall("tiles:scene-a").noteReset(); + const auto retired = sources.forWall("tiles:scene-a").generation(); + sources.releaseAllExcept({}); + EXPECT_EQ(sources.forWall("tiles:scene-a").generation(), 0U); + EXPECT_NE(sources.forWall("tiles:scene-a").generation(), retired); +} +} // namespace +``` + +Register it in `native/CMakeLists.txt`: + +```cmake + tests/TilesWallSourceTest.cpp +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `native/build-dev/corevideo-native-tests.exe --gtest_filter='TilesWallSources.*'` +Expected: compile failure — `core/TilesWallSource.h` does not exist. + +- [ ] **Step 3: Write the implementation** + +Create `native/src/core/TilesWallSource.h`: + +```cpp +#pragma once + +#include "compositor/TilesPlanAnimation.h" + +#include +#include +#include +#include + +namespace corevideo::core { + +// One wall's animation, owned by the WALL rather than by a bus. +// +// Before this existed, MediaCore held programTilesAnimation_ and +// previewTilesAnimation_, and a Take handed settled state from one to the other +// (TilesPlanAnimation::adoptSettledFrom). That hand-off REFUSED a wall whose +// tiles were still flying - "mid-flight state belongs to the bus that is flying +// it" - because with two animators there is no correct answer. So a wall taken +// mid-animation re-animated on the cut, which is #448. +// +// With one animator there is nothing to hand over: both buses sample the same +// object, and a cut changes only which bus is looking at it. +class TilesWallSource final { + public: + // Wraps the animation so a reset can never happen without the generation + // moving. TilesPlanAnimation::advance returns true when it reset the + // animator; a plain bool return keeps this allocation-free on the render tick + // (a std::function callback would not be). + void advance(modules::CompositorRenderPlan& plan, const std::string& wallId, bool present, + bool enabled, double durationMs, double nowMs) { + if (animation_.advance(plan, wallId, present, enabled, durationMs, nowMs)) { + noteReset(); + } + } + + void applyLatest(modules::CompositorRenderPlan& plan, const std::string& wallId) const { + animation_.applyLatest(plan, wallId); + } + + // The take record's proof that nothing restarted (spec section 5), so + // "did this wall restart?" is a number rather than an opinion. + [[nodiscard]] uint64_t generation() const { return generation_; } + void noteReset() { ++generation_; } + + private: + compositor::TilesPlanAnimation animation_; + uint64_t generation_ = 0; +}; + +// Wall id -> source. The id is the Tiles layer id, which the shell already +// emits as "tiles:" (TilesLayerPayloadBuilder.cs), so it is unique per +// scene and identical for the same gallery on either bus. +class TilesWallSources final { + public: + [[nodiscard]] TilesWallSource& forWall(const std::string& wallId) { + return sources_[wallId]; + } + + // Lifetime is "referenced by a scene" (parent spec section 2). Anything no + // live scene names is released; a recreated wall is a NEW wall and starts at + // generation 0, never continuing a retired one's count. + void releaseAllExcept(const std::vector& liveWallIds) { + for (auto it = sources_.begin(); it != sources_.end();) { + bool live = false; + for (const auto& id : liveWallIds) { + if (it->first == id) { live = true; break; } + } + it = live ? std::next(it) : sources_.erase(it); + } + } + + // Const lookup for readers (the take record). An unknown wall has never + // animated, so its caller reports generation 0 - which is true, not a guess. + [[nodiscard]] const TilesWallSource* find(const std::string& wallId) const { + const auto it = sources_.find(wallId); + return it == sources_.end() ? nullptr : &it->second; + } + + [[nodiscard]] std::size_t size() const { return sources_.size(); } + + private: + std::map sources_; +}; + +} // namespace corevideo::core +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `native/build-dev/corevideo-native-tests.exe --gtest_filter='TilesWallSources.*'` +Expected: 5 tests PASS. + +- [ ] **Step 5: Commit** + +```bash +git add native/src/core/TilesWallSource.h native/tests/TilesWallSourceTest.cpp native/CMakeLists.txt +git commit -m "feat(tiles): a wall owns its own animation, keyed by wall id" +``` + +--- + +### Task 3: MediaCore uses one animator per wall, and the hand-off is deleted + +**Files:** +- Modify: `native/src/core/MediaCore.h:522-523` (replace the two animators with the map) +- Modify: `native/src/core/MediaCore.cpp:3634` (program plan `applyLatest`) +- Modify: `native/src/core/MediaCore.cpp:3670` (multiview PVW `applyLatest`) +- Modify: `native/src/core/MediaCore.cpp:6263-6277` (the take-tick hand-off and both `advance` calls) +- Modify: `native/src/core/MediaCore.cpp:6558` (preview-scene `applyLatest`) +- Modify: `native/src/compositor/TilesPlanAnimation.h` (delete `adoptSettledFrom`) +- Test: `native/tests/TilesRenderPlanTest.cpp` (add the headline test) +- Modify: `native/tests/TilesAnimatorTest.cpp:97-163` (rewrite the hand-off tests) + +**Interfaces:** +- Consumes: `core::TilesWallSources` from Task 2. +- Produces: `MediaCore::tilesWallSources_`, and `MediaCore::tilesWallGeneration(const std::string& wallId) const` for the take record. + +**This is the task that fixes #448.** Everything before it is scaffolding. + +- [ ] **Step 1: Write the failing headline test** + +Add to `native/tests/TilesRenderPlanTest.cpp`: + +```cpp +// #448. adoptSettledFrom refuses a wall whose tiles are still flying, because +// with two animators mid-flight state has no correct owner. So a wall taken +// MID-ANIMATION re-animated from alpha 0 on Program. With one animator per wall +// there is nothing to hand over and the cut is continuous. +// +// This test MUST FAIL before Task 3. Verify that by reverting, not by assuming. +TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { + TilesRenderPlanHarness harness; + harness.cueWallInPreview("scene-a", /*members=*/4); + + // Advance only PART WAY through the entry animation: tiles are still flying. + harness.advanceMs(harness.animationDurationMs() / 3); + const auto midFlight = harness.sampledPreviewTiles(); + ASSERT_FALSE(midFlight.empty()); + ASSERT_FALSE(std::all_of(midFlight.begin(), midFlight.end(), + [](const auto& tile) { return tile.atRest; })) + << "precondition: the wall must still be animating for this test to mean anything"; + + const auto generationBefore = harness.wallGeneration("tiles:scene-a"); + harness.takeToProgram("scene-a"); + harness.renderOneTick(); + + // The wall did not restart... + EXPECT_EQ(harness.wallGeneration("tiles:scene-a"), generationBefore); + + // ...and its tiles continued from where they were, rather than snapping back + // to the entry state. Alpha is the sharpest signal: a replayed entrance + // restarts at 0. + const auto afterTake = harness.sampledProgramTiles(); + ASSERT_EQ(afterTake.size(), midFlight.size()); + for (size_t i = 0; i < afterTake.size(); ++i) { + EXPECT_GE(afterTake[i].alpha, midFlight[i].alpha) + << "tile " << i << " replayed its entrance instead of continuing"; + } +} +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `native/build-dev/corevideo-native-tests.exe --gtest_filter='TilesRenderPlan.AWallTakenMidAnimationIsContinuous'` +Expected: FAIL — the program animator resets on the key it has never held, so alpha restarts near 0 and the generation moves. + +If the harness helpers (`cueWallInPreview`, `advanceMs`, `sampledPreviewTiles`, `takeToProgram`, `wallGeneration`) do not exist, add them to the existing harness in that file first, in their own commit, with no behaviour change. + +- [ ] **Step 3: Replace the two animators with the map** + +In `native/src/core/MediaCore.h`, delete: + +```cpp + compositor::TilesPlanAnimation programTilesAnimation_; + compositor::TilesPlanAnimation previewTilesAnimation_; +``` + +and add: + +```cpp + // One animation per WALL, not per bus (#448). See core/TilesWallSource.h for + // why the per-bus pair and its hand-off were wrong. + core::TilesWallSources tilesWallSources_; +``` + +In `native/src/core/MediaCore.cpp`, replace the take-tick block at 6263-6277 with: + +```cpp + const std::string programWallId = tilesLayer_.layerId; + const std::string previewWallId = previewTilesLayer_.layerId; + if (tilesLayer_.present && tilesLayer_.style.animateLayout) { + tilesWallSources_.forWall(programWallId).advance( + renderPlan, programWallId, tilesLayer_.present, tilesLayer_.style.animateLayout, + tilesLayer_.style.animationDurationMs, animationNowMs); + } + // Advance Preview on the SAME render clock even when its wall is empty, so + // snapshot/prefetch builds cannot change entry/departure animation state. + // When both buses show the SAME wall this is the same object, advanced once + // above - guard against double-advancing it on one tick. + if (previewTilesLayer_.present && previewTilesLayer_.style.animateLayout && + hasPreviewScene() && previewWallId != programWallId) { + auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); + tilesWallSources_.forWall(previewWallId).advance( + previewAnimationPlan, previewWallId, true, previewTilesLayer_.style.animateLayout, + previewTilesLayer_.style.animationDurationMs, animationNowMs); + } + // Lifetime: release any wall no live scene still names (parent spec section 2). + std::vector liveWallIds; + if (tilesLayer_.present) liveWallIds.push_back(programWallId); + if (previewTilesLayer_.present) liveWallIds.push_back(previewWallId); + tilesWallSources_.releaseAllExcept(liveWallIds); +``` + +Update the three `applyLatest` sites to read from the map, passing the wall id +(no longer `sceneId + ":" + layerId`): + +- `MediaCore.cpp:3634` → `tilesWallSources_.forWall(tilesLayer_.layerId).applyLatest(programPlan, tilesLayer_.layerId);` +- `MediaCore.cpp:3670` and `:6558` → the same shape with `previewTilesLayer_.layerId`. + +Add the generation accessor for the take record: + +```cpp +uint64_t MediaCore::tilesWallGeneration(const std::string& wallId) const { + return tilesWallSources_.forWall(wallId).generation(); +} +``` + +Use the const `find()` added in Task 2 rather than making `forWall` const — +`forWall` inserts, and a read must never create a wall: + +```cpp +uint64_t MediaCore::tilesWallGeneration(const std::string& wallId) const { + const auto* wall = tilesWallSources_.find(wallId); + return wall ? wall->generation() : 0; +} +``` + +- [ ] **Step 4: Delete the hand-off** + +In `native/src/compositor/TilesPlanAnimation.h`, delete `adoptSettledFrom` entirely, along with its comment block. Nothing may call it: with one animator per wall there is no second animator to adopt from, and leaving it would invite a future caller to reintroduce per-bus state. + +Change `advance` to REPORT whether it reset, so the generation cannot move +without the animator moving (and vice versa). Return type `void` -> `bool`: + +```cpp + // Returns TRUE when this call reset the animator. TilesWallSource turns that + // into a generation bump; a bool return keeps the render tick allocation-free + // where a std::function callback would not. + bool advance(modules::CompositorRenderPlan& plan, const std::string& wallKey, + bool present, bool enabled, double durationMs, double nowMs) { + if (!present || !enabled) { const bool had = !key_.empty(); reset(); return had; } + bool didReset = false; + if (key_ != wallKey) { animator_.reset(); key_ = wallKey; sampled_.clear(); didReset = true; } + ... // body below unchanged, including the all-stale guard + return didReset; + } +``` + +**The all-stale guard stays exactly as it is** — `if (targets.empty() && !sampled_.empty()) return didReset;`. With one shared animator a wipe would now +affect every bus at once, so this guard matters more than before, not less. + +- [ ] **Step 5: Run the headline test to verify it passes** + +Run: `native/build-dev/corevideo-native-tests.exe --gtest_filter='TilesRenderPlan.AWallTakenMidAnimationIsContinuous'` +Expected: PASS. + +- [ ] **Step 6: Verify the red was real** + +```bash +git stash push -u -m "448-verify-red" +# confirm the test FAILS on the pre-change tree, then restore: +git stash list --format='%H %gs' # capture YOUR entry's SHA +git stash apply +``` + +Expected: FAIL without the change, PASS with it. **A test that passes both ways proves nothing** — this has happened three times in this codebase. + +- [ ] **Step 7: Rewrite the tests that pinned the hand-off** + +`native/tests/TilesAnimatorTest.cpp:97-163` contains three hand-off tests +(`AWallSettledInPreviewIsAlreadySettledOnItsFirstProgramFrame`, +`AWallTakenWhileItsFramesLapseIsStillCutToNotRedrawn`, +`AWallThatWasNeverInPreviewIsHandedNothing`). **Rewrite, do not delete** — they +pin real behaviour that must survive: + +- the first becomes "a wall settled on one bus is settled on the other, because it is the same object"; +- the second keeps its all-stale-beat assertion, which is now about the shared animator's retained tiles; +- the third becomes "a wall that was never cued starts cold", asserting generation 0 and a full entry animation. + +- [ ] **Step 8: Run the full suite and the escape scanner** + +Run: `native/build-dev/corevideo-native-tests.exe` +Expected: all green, 931 + the tests added by this plan. + +Run: `python scripts/qa/check-string-escapes.py` +Expected: `no invalid escape sequences found`. + +- [ ] **Step 9: Commit** + +```bash +git add native/src/core/MediaCore.h native/src/core/MediaCore.cpp \ + native/src/compositor/TilesPlanAnimation.h \ + native/tests/TilesRenderPlanTest.cpp native/tests/TilesAnimatorTest.cpp +git commit -m "fix(tiles): one animator per wall, so a wall taken mid-animation is continuous + +Closes #448" +``` + +--- + +### Task 4: The wall registers as a composed source, and the take record proves continuity + +**Files:** +- Modify: `native/src/core/MediaCore.cpp` (register/release around the `releaseAllExcept` call added in Task 3) +- Modify: `native/src/core/TakeRecordPolicy.h` (carry the wall generation) +- Test: `native/tests/TakeRecordTest.cpp` + +**Interfaces:** +- Consumes: `SourceRegistry::Kind::Composed` (Task 1); `TilesWallSources` (Task 2); `MediaCore::tilesWallGeneration` (Task 3). +- Produces: a take record whose `sources[]` includes `tiles:` with `generationBefore`/`generationAfter`. + +- [ ] **Step 1: Write the failing test** + +Add to `native/tests/TakeRecordTest.cpp`: + +```cpp +// The spec's proof (section 5): a take record cannot read "cut" while the wall +// restarted. Before this task the wall was invisible to the ledger entirely, so +// a re-animating wall could be recorded as a clean cut. +TEST(TakeRecord, AWallThatRestartedCannotBeRecordedAsACleanCut) { + TakeRecordHarness harness; + harness.cueWallInPreview("scene-a"); + harness.takeToProgram("scene-a"); + harness.forceWallReset("tiles:scene-a"); // a cold start, however caused + + const auto record = harness.completeTakeRecord(); + + const auto wall = record.sourceNamed("tiles:scene-a"); + ASSERT_TRUE(wall.has_value()); + EXPECT_NE(wall->generationBefore, wall->generationAfter); + EXPECT_TRUE(wall->restarted); + EXPECT_EQ(record.verdict, "rebuilt"); +} + +TEST(TakeRecord, AContinuousWallIsRecordedAsACut) { + TakeRecordHarness harness; + harness.cueWallInPreview("scene-a"); + harness.takeToProgram("scene-a"); + + const auto record = harness.completeTakeRecord(); + + const auto wall = record.sourceNamed("tiles:scene-a"); + ASSERT_TRUE(wall.has_value()); + EXPECT_EQ(wall->generationBefore, wall->generationAfter); + EXPECT_FALSE(wall->restarted); + EXPECT_EQ(record.verdict, "cut"); +} +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `native/build-dev/corevideo-native-tests.exe --gtest_filter='TakeRecord.AWallThatRestartedCannotBeRecordedAsACleanCut:TakeRecord.AContinuousWallIsRecordedAsACut'` +Expected: FAIL — `record.sourceNamed("tiles:scene-a")` is empty; the wall is not in the ledger. + +- [ ] **Step 3: Register the wall and feed its generation to the take record** + +Where Task 3 added `releaseAllExcept`, also register and release in the registry: + +```cpp + // The wall is a SOURCE (parent spec section 2), so it registers like one. + // A composed registration carries no externalId and never claims a + // subscription state - see SourceRegistry::Kind::Composed. + for (const auto& wallId : liveWallIds) { + if (registeredWallIds_.insert(wallId).second) { + SourceRegistry::Registration registration; + registration.sourceId = {wallId}; + registration.kind = SourceRegistry::Kind::Composed; + registration.displayName = "Tiles wall"; + registration.processEpoch = coreProcessEpoch_; + sourceRegistry_.add(std::move(registration)); + } + } +``` + +and remove entries from `registeredWallIds_` (and the registry) for walls the +sweep released, so a recreated wall registers afresh. + +Then include the wall in the take record's source set, using +`tilesWallGeneration(wallId)` for `generationBefore`/`generationAfter`, exactly as +`SourceContinuityLedger` already does for frame sources. + +- [ ] **Step 4: Run to verify it passes** + +Run: the same filter as Step 2. +Expected: both PASS. + +- [ ] **Step 5: Run the full suite and the escape scanner** + +Run: `native/build-dev/corevideo-native-tests.exe` then `python scripts/qa/check-string-escapes.py` +Expected: all green; `no invalid escape sequences found`. + +- [ ] **Step 6: Commit** + +```bash +git add native/src/core/MediaCore.cpp native/src/core/MediaCore.h \ + native/src/core/TakeRecordPolicy.h native/tests/TakeRecordTest.cpp +git commit -m "feat(tiles): register the wall as a composed source and prove its continuity in the take record" +``` + +--- + +## What this plan deliberately does NOT do + +These are plan 2, and each is listed in the spec: + +- The wall's own offscreen texture, and buses sampling it. +- Collapsing the plan to a single `tiles-wall` layer (and the Metal / CPU-preview parity that forces). +- The multiview PVW cell sampling the Preview bus texture. +- Adopting `AtomicTakeCoordinator` for the cut. +- The **pixel-continuity probe** (`ProgramPixelContinuityTest`, the luma-comb + technique). It belongs with plan 2 because it proves what is DRAWN, and plan 1 + changes no drawing. Plan 1's proof is the generation counter, which is the + spec's first proof layer. +- The fault-injection and live-soak gates, and the integrated-GPU number that + **cannot be produced** here (#425). + +**Invariants 1, 2, 6 and 7 of the spec are untouched by this plan** (the wall's +background is still emitted above the admission gate; `wall.present` alone still +gates; the `tiles` snapshot node is unchanged; borders stay multiview-only), +because layer emission is not modified. Invariants 3 and 4 — the asymmetric +stale rules and the all-stale guard — ARE in scope here and are pinned by the +rewritten tests in Task 3, Step 7. + +**Nothing in this plan touches how a frame is drawn**, which is why it can land while `beta-2026-09-12-c425e3c` is still being live-checked. Plan 2 changes the render path and should wait for that beta to be shaken out. From c1febac9aae90c6cacafb121eb1e82efa6180a84 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 18:55:37 -0400 Subject: [PATCH 05/20] chore(registry): carve SourceRegistry out of #419 onto main's line, unchanged Prerequisite for plan 1 of persistent-sources slice 2. No behaviour change: the files are taken verbatim from origin/codex/production-realtime-architecture and wired into the existing targets. The carve is clean because SourceRegistry.cpp includes only its own header plus and -- zero project dependencies -- which is the condition the parent spec set ("SourceRegistry.h is reused only if it fits cleanly"). Nothing else from #419 is needed: SceneVersionShadow and ShowPlanGenerator consume the registry but are not part of this plan. Verified: its own 22 tests pass, and the full suite is 953 green (931 + 22). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/CMakeLists.txt | 2 + native/src/core/SourceRegistry.cpp | 238 ++++++++++++++ native/src/core/SourceRegistry.h | 112 +++++++ native/tests/SourceRegistryTest.cpp | 483 ++++++++++++++++++++++++++++ 4 files changed, 835 insertions(+) create mode 100644 native/src/core/SourceRegistry.cpp create mode 100644 native/src/core/SourceRegistry.h create mode 100644 native/tests/SourceRegistryTest.cpp diff --git a/native/CMakeLists.txt b/native/CMakeLists.txt index a0ff73c3..6964dd76 100644 --- a/native/CMakeLists.txt +++ b/native/CMakeLists.txt @@ -65,6 +65,7 @@ add_library(corevideo_native src/core/BoundedAsyncLog.cpp src/core/LockHoldGuardrail.cpp src/core/MediaCore.cpp + src/core/SourceRegistry.cpp src/modules/AsyncEncoderSink.cpp src/modules/AsyncOutputSender.cpp src/modules/OutputDestinationSupervisor.cpp @@ -619,6 +620,7 @@ if(BUILD_TESTING) tests/CompositorFramingTest.cpp tests/ContractParityTest.cpp tests/RouteSourcePolicyTest.cpp + tests/SourceRegistryTest.cpp tests/EmptyRouteBlankTest.cpp tests/OutputLifecyclePolicyTest.cpp tests/MonitorShedPolicyTest.cpp diff --git a/native/src/core/SourceRegistry.cpp b/native/src/core/SourceRegistry.cpp new file mode 100644 index 00000000..b5dd1f40 --- /dev/null +++ b/native/src/core/SourceRegistry.cpp @@ -0,0 +1,238 @@ +#include "core/SourceRegistry.h" + +#include +#include + +namespace corevideo::core { +SourceRegistry::SourceRegistry(const SourceRegistry& other) { + std::lock_guard lock(other.mutex_); + epoch_ = other.epoch_; revision_ = other.revision_; decisionRevision_ = other.decisionRevision_; + persons_ = other.persons_; sources_ = other.sources_; retiredProcessEpochs_ = other.retiredProcessEpochs_; + maxPersons_ = other.maxPersons_; maxSources_ = other.maxSources_; maxRetiredProcessEpochs_ = other.maxRetiredProcessEpochs_; +} + +SourceRegistry::SourceRegistry(std::string registryEpoch, std::size_t maxPersons, + std::size_t maxSources, std::size_t maxRetiredProcessEpochs) + : epoch_(std::move(registryEpoch)), maxPersons_(maxPersons), maxSources_(maxSources), + maxRetiredProcessEpochs_(maxRetiredProcessEpochs) { + if (epoch_.empty() || epoch_.size() > 512 || maxPersons_ == 0 || maxSources_ == 0 || + maxRetiredProcessEpochs_ == 0) + throw std::invalid_argument("SourceRegistry requires an authority epoch and positive capacities"); +} + +bool SourceRegistry::sameToken(const Token& a, const Token& b) { + return a.sourceId.value == b.sourceId.value && a.instanceId.value == b.instanceId.value && + a.processEpoch == b.processEpoch && a.generation == b.generation; +} + +SourceRegistry::Result SourceRegistry::upsertPerson(Person person) { + std::lock_guard lock(mutex_); + if (person.id.value.empty() || person.id.value.size() > 512 || person.displayName.size() > 4096 || + person.generation == 0 || person.generation > kMaxRevision) + return Result::Invalid; + const auto existing = persons_.find(person.id.value); + if (existing != persons_.end()) { + if (person.generation < existing->second.generation) return Result::Stale; + if (existing->second.displayName == person.displayName && + existing->second.generation == person.generation) return Result::Unchanged; + } + if (existing == persons_.end() && persons_.size() >= maxPersons_) return Result::Exhausted; + if (revision_ == kMaxRevision) return Result::Exhausted; + const auto key = person.id.value; + const bool identityChanged = existing == persons_.end() || existing->second.generation != person.generation; + persons_[key] = std::move(person); + ++revision_; + if (identityChanged) ++decisionRevision_; + return Result::Applied; +} + +bool SourceRegistry::validRegistration(const Registration& r) const { + const bool knownKind = r.kind == Kind::ParticipantVideo || r.kind == Kind::ParticipantShare || + r.kind == Kind::Device || r.kind == Kind::Media || r.kind == Kind::Browser; + return knownKind && (!r.requestedGeneration || (*r.requestedGeneration > 0 && *r.requestedGeneration <= kMaxRevision)) && + (!r.instanceId || (!r.instanceId->value.empty() && r.instanceId->value.size() <= 512)) && + !r.sourceId.value.empty() && r.sourceId.value.size() <= 512 && + !r.processEpoch.empty() && r.processEpoch.size() <= 512 && + !retiredProcessEpochs_.contains(r.processEpoch) && !r.externalId.empty() && + r.externalId.size() <= 512 && r.displayName.size() <= 4096 && + ((!r.personId && r.personGeneration == 0) || + (r.personId && persons_.contains(r.personId->value) && r.personGeneration > 0 && + persons_.at(r.personId->value).generation == r.personGeneration)); +} + +bool SourceRegistry::externalConflict(const Registration& r) const { + for (const auto& entry : sources_) { + const auto& source = entry.second; + if (entry.first != r.sourceId.value && source.availability != Availability::Departed && + r.instanceId && source.token.instanceId.value == r.instanceId->value) return true; + if (entry.first != r.sourceId.value && source.availability != Availability::Departed && + source.kind == r.kind && source.token.processEpoch == r.processEpoch && source.externalId == r.externalId) + return true; + } + return false; +} + +SourceRegistry::Mutation SourceRegistry::install(Registration r, uint64_t generation) { + if (revision_ == kMaxRevision || generation > kMaxRevision) return {Result::Exhausted, {}}; + Source source; + source.token = {r.sourceId, r.instanceId.value_or(SourceInstanceId{epoch_ + ":" + std::to_string(revision_ + 1)}), r.processEpoch, generation}; + for (const auto& [id, existing] : sources_) { + if (id != r.sourceId.value && existing.availability != Availability::Departed && + existing.token.instanceId.value == source.token.instanceId.value) return {Result::Conflict, {}}; + } + source.kind = r.kind; + source.personId = std::move(r.personId); + source.personGeneration = r.personGeneration; + source.displayName = std::move(r.displayName); + source.externalId = std::move(r.externalId); + const auto token = source.token; + sources_[r.sourceId.value] = std::move(source); + ++revision_; + ++decisionRevision_; + return {Result::Applied, token}; +} + +SourceRegistry::Mutation SourceRegistry::add(Registration r) { + std::lock_guard lock(mutex_); + if (!validRegistration(r)) return {Result::Invalid, {}}; + if (sources_.find(r.sourceId.value) != sources_.end() || externalConflict(r)) return {Result::Conflict, {}}; + if (sources_.size() >= maxSources_) return {Result::Exhausted, {}}; + const auto generation = r.requestedGeneration.value_or(1); + return install(std::move(r), generation); +} + +SourceRegistry::Mutation SourceRegistry::replace(const Token& expected, Registration r) { + std::lock_guard lock(mutex_); + const auto found = sources_.find(expected.sourceId.value); + if (found == sources_.end()) return {Result::NotFound, {}}; + if (!sameToken(found->second.token, expected)) return {Result::Stale, {}}; + if (!validRegistration(r) || r.sourceId.value != expected.sourceId.value || r.kind != found->second.kind) + return {Result::Invalid, {}}; + if (externalConflict(r)) return {Result::Conflict, {}}; + if (r.requestedGeneration && *r.requestedGeneration <= expected.generation) return {Result::Stale, {}}; + const auto generation = r.requestedGeneration.value_or(expected.generation + 1); + return install(std::move(r), generation); +} + +SourceRegistry::Result SourceRegistry::setDisplayName(const Token& token, const std::string& name) { + // Empty labels are valid metadata; names are never identifiers. + if (name.size() > 4096) return Result::Invalid; + std::lock_guard lock(mutex_); + const auto found = sources_.find(token.sourceId.value); + if (found == sources_.end()) return Result::NotFound; + auto& source = found->second; + if (!sameToken(source.token, token) || source.availability == Availability::Departed) + return Result::Stale; + if (source.displayName == name) return Result::Unchanged; + if (revision_ == kMaxRevision) return Result::Exhausted; + source.displayName = name; + ++revision_; + return Result::Applied; +} + +SourceRegistry::Result SourceRegistry::setAvailability(const Token& token, Availability availability) { + std::lock_guard lock(mutex_); + const auto found = sources_.find(token.sourceId.value); + if (found == sources_.end()) return Result::NotFound; + auto& source = found->second; + if (!sameToken(source.token, token) || source.availability == Availability::Departed) return Result::Stale; + if (availability != Availability::Available && availability != Availability::Unavailable && availability != Availability::Departed) + return Result::Invalid; + if (source.availability == availability) return Result::Unchanged; + if (revision_ == kMaxRevision) return Result::Exhausted; + source.availability = availability; + ++decisionRevision_; + if (availability != Availability::Available) { + source.subscriptionObserved = false; + source.format.reset(); + source.hasPublication = false; + // Keep the incarnation's publication watermark across camera off/on. + } + ++revision_; + return Result::Applied; +} + +SourceRegistry::Result SourceRegistry::setSubscription(const Token& token, bool requested, std::optional observed) { + std::lock_guard lock(mutex_); + const auto found = sources_.find(token.sourceId.value); + if (found == sources_.end()) return Result::NotFound; + auto& source = found->second; + if (!sameToken(source.token, token) || source.availability == Availability::Departed) return Result::Stale; + if (observed.value_or(false) && source.availability != Availability::Available) return Result::Invalid; + if (source.subscriptionRequested == requested && source.subscriptionObserved == observed) + return Result::Unchanged; + if (revision_ == kMaxRevision) return Result::Exhausted; + source.subscriptionRequested = requested; + source.subscriptionObserved = observed; + ++revision_; + return Result::Applied; +} + +SourceRegistry::Result SourceRegistry::retireProcessEpoch(const std::string& processEpoch) { + std::lock_guard lock(mutex_); + if (processEpoch.empty() || processEpoch.size() > 512) return Result::Invalid; + if (retiredProcessEpochs_.contains(processEpoch)) return Result::Unchanged; + if (retiredProcessEpochs_.size() >= maxRetiredProcessEpochs_) return Result::Exhausted; + if (revision_ == kMaxRevision) return Result::Exhausted; + // Persist the fence even if retirement wins the race with the first roster + // callback. A delayed add from a dead helper must never look current. + retiredProcessEpochs_.insert(processEpoch); + ++decisionRevision_; + for (auto& entry : sources_) { + auto& source = entry.second; + if (source.token.processEpoch != processEpoch) continue; + source.availability = Availability::Departed; + source.subscriptionRequested = false; + source.subscriptionObserved = false; + source.format.reset(); + source.hasPublication = false; + // Retired tokens retain their publication watermark for diagnostics. + } + ++revision_; + return Result::Applied; +} + +SourceRegistry::Result SourceRegistry::publish(const Token& token, uint64_t sequence, int64_t observedNs, Format format) { + std::lock_guard lock(mutex_); + const auto found = sources_.find(token.sourceId.value); + if (found == sources_.end()) return Result::NotFound; + auto& source = found->second; + if (!sameToken(source.token, token) || source.availability != Availability::Available) return Result::Stale; + if (sequence > kMaxRevision || observedNs < 0 || format.width <= 0 || format.height <= 0 || + format.fpsNumerator.has_value() != format.fpsDenominator.has_value() || + (format.fpsNumerator && (*format.fpsNumerator <= 0 || *format.fpsDenominator <= 0)) || format.pixelFormat.empty() || + format.pixelFormat.size() > 128) return Result::Invalid; + if (source.hasPublicationWatermark && + (sequence <= source.publicationSequence || observedNs < source.lastPublicationNs)) return Result::Stale; + if (revision_ == kMaxRevision) return Result::Exhausted; + // A format change invalidates prepared GPU resources even when the exact + // source incarnation remains current. Steady publications do not churn plans. + if (!source.hasPublication || !source.format || *source.format != format) ++decisionRevision_; + source.format = std::move(format); + source.hasPublication = true; + source.hasPublicationWatermark = true; + source.publicationSequence = sequence; + source.lastPublicationNs = observedNs; + ++revision_; + return Result::Applied; +} + +std::shared_ptr SourceRegistry::snapshot() const { + std::lock_guard lock(mutex_); + auto result = std::make_shared(); + result->registryEpoch = epoch_; + result->revision = revision_; + result->decisionRevision = decisionRevision_; + for (const auto& entry : persons_) result->persons.push_back(entry.second); + for (const auto& entry : sources_) result->sources.push_back(entry.second); + return result; +} + +std::vector SourceRegistry::peopleNamed(const std::string& displayName) const { + std::lock_guard lock(mutex_); + std::vector result; + for (const auto& entry : persons_) if (entry.second.displayName == displayName) result.push_back(entry.second.id); + return result; +} + +} // namespace corevideo::core diff --git a/native/src/core/SourceRegistry.h b/native/src/core/SourceRegistry.h new file mode 100644 index 00000000..7e1a4629 --- /dev/null +++ b/native/src/core/SourceRegistry.h @@ -0,0 +1,112 @@ +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace corevideo::core { + +// IDs are intentionally different types; neither names nor SDK handles are IDs. +struct PersonId { std::string value; }; +struct SourceId { std::string value; }; +struct SourceInstanceId { std::string value; }; + +class SourceRegistry final { + public: + enum class Kind { ParticipantVideo, ParticipantShare, Device, Media, Browser }; + enum class Availability { Available, Unavailable, Departed }; + enum class Result { Applied, Unchanged, Invalid, NotFound, Conflict, Stale, Exhausted }; + struct Format { + int width = 0, height = 0; + std::optional fpsNumerator, fpsDenominator; // Both absent means unknown rate. + std::string pixelFormat; + bool operator==(const Format&) const = default; + }; + struct Token { + SourceId sourceId; + SourceInstanceId instanceId; + std::string processEpoch; + uint64_t generation = 0; + }; + struct Person { + PersonId id; + std::string displayName; + uint64_t generation = 1; + }; + struct Registration { + SourceId sourceId; + Kind kind = Kind::ParticipantVideo; + std::optional personId; // Explicit operator/authority binding only. + uint64_t personGeneration = 0; + std::string displayName; + std::string processEpoch; + std::string externalId; // SDK participant/device ID, scoped by processEpoch. + std::optional instanceId; // Provider-owned exact identity, if available. + std::optional requestedGeneration; // Provider fence; gaps are valid. + }; + struct Source { + Token token; + Kind kind = Kind::ParticipantVideo; + std::optional personId; + uint64_t personGeneration = 0; + std::string displayName, externalId; + Availability availability = Availability::Available; + bool subscriptionRequested = false; + std::optional subscriptionObserved{false}; // nullopt means unacknowledged/unknown. + std::optional format; + bool hasPublication = false; + bool hasPublicationWatermark = false; // Survives unavailable/departed until token replacement. + uint64_t publicationSequence = 0; + int64_t lastPublicationNs = 0; // Caller monotonic clock; never UTC. + }; + struct Snapshot { + std::string registryEpoch; + uint64_t revision = 0; + uint64_t decisionRevision = 0; // Binding/readiness changes, independent of frame traffic. + std::vector persons; + std::vector sources; // Stable SourceId order, includes departure tombstones. + }; + struct Mutation { Result result; std::optional token; }; + + explicit SourceRegistry(std::string registryEpoch, std::size_t maxPersons = 4'096, + std::size_t maxSources = 16'384, + std::size_t maxRetiredProcessEpochs = 4'096); + SourceRegistry(const SourceRegistry&); // Bounded independent staging copy, including tombstones. + Result upsertPerson(Person person); + Mutation add(Registration registration); + // Compare-and-replace: old callbacks can neither replace nor retire a new instance. + Mutation replace(const Token& expected, Registration registration); + Result setDisplayName(const Token& token, const std::string& name); + Result setAvailability(const Token& token, Availability availability); + Result setSubscription(const Token& token, bool requested, std::optional observed); + // Retire every source owned by a replaced helper process as one registry + // transaction. This fences callbacks even when a provider changes its source IDs. + Result retireProcessEpoch(const std::string& processEpoch); + Result publish(const Token& token, uint64_t sequence, int64_t observedNs, Format format); + [[nodiscard]] std::shared_ptr snapshot() const; + // Discovery only: zero/multiple matches remain explicit, with no auto-binding. + [[nodiscard]] std::vector peopleNamed(const std::string& displayName) const; + + private: + static bool sameToken(const Token& left, const Token& right); + bool validRegistration(const Registration& registration) const; + bool externalConflict(const Registration& registration) const; + Mutation install(Registration registration, uint64_t generation); + static constexpr uint64_t kMaxRevision = 9007199254740991ULL; + mutable std::mutex mutex_; + std::string epoch_; + uint64_t revision_ = 0; + uint64_t decisionRevision_ = 0; + std::map persons_; + std::map sources_; + std::set retiredProcessEpochs_; + std::size_t maxPersons_, maxSources_, maxRetiredProcessEpochs_; +}; + +} // namespace corevideo::core diff --git a/native/tests/SourceRegistryTest.cpp b/native/tests/SourceRegistryTest.cpp new file mode 100644 index 00000000..0106f9e1 --- /dev/null +++ b/native/tests/SourceRegistryTest.cpp @@ -0,0 +1,483 @@ +#include "core/SourceRegistry.h" +#include +#include +#include +#include + +namespace { +using Registry = corevideo::core::SourceRegistry; +Registry::Registration participant(const std::string& id, const std::string& handle = "42") { + Registry::Registration result; + result.sourceId = {id}; + result.processEpoch = "meeting-1"; + result.externalId = handle; + result.displayName = "Alex"; + return result; +} +Registry::Format format() { return {1920, 1080, 60, 1, "I420"}; } +} + +TEST(SourceRegistry, DuplicateNamesDoNotBindOrMergePeopleAndSources) { + Registry registry("authority"); + registry.upsertPerson({{"person-b"}, "Alex"}); + registry.upsertPerson({{"person-a"}, "Alex"}); + const auto matches = registry.peopleNamed("Alex"); + ASSERT_EQ(matches.size(), 2u); + EXPECT_EQ(matches[0].value, "person-a"); + EXPECT_EQ(matches[1].value, "person-b"); + EXPECT_TRUE(registry.peopleNamed("missing").empty()); + EXPECT_EQ(registry.add(participant("source-a")).result, Registry::Result::Applied); + auto bound = participant("source-b", "43"); + bound.personId = corevideo::core::PersonId{"person-b"}; + bound.personGeneration = 1; + EXPECT_EQ(registry.add(bound).result, Registry::Result::Applied); + const auto snapshot = registry.snapshot(); + ASSERT_EQ(snapshot->sources.size(), 2u); + EXPECT_FALSE(snapshot->sources[0].personId.has_value()); + ASSERT_TRUE(snapshot->sources[1].personId.has_value()); + EXPECT_EQ(snapshot->sources[1].personId->value, "person-b"); + bound.sourceId = {"source-c"}; + bound.externalId = "44"; + bound.personId = corevideo::core::PersonId{"unknown-person"}; + bound.personGeneration = 1; + EXPECT_EQ(registry.add(bound).result, Registry::Result::Invalid); +} + +TEST(SourceRegistry, ReconnectResetsTransientStateAndRejectsEveryOldMutation) { + Registry registry("authority"); + registry.upsertPerson({{"durable-person"}, "Alex"}); + auto registration = participant("camera"); + registration.personId = corevideo::core::PersonId{"durable-person"}; + registration.personGeneration = 1; + const auto original = registry.add(registration); + ASSERT_TRUE(original.token.has_value()); + registry.setSubscription(*original.token, true, true); + registry.publish(*original.token, 100, 1000, format()); + const auto before = registry.snapshot(); + registration.processEpoch = "meeting-2"; + const auto replacement = registry.replace(*original.token, registration); + ASSERT_TRUE(replacement.token.has_value()); + EXPECT_EQ(replacement.token->generation, 2u); + EXPECT_TRUE(replacement.token->instanceId.value != original.token->instanceId.value); + EXPECT_EQ(registry.publish(*original.token, 101, 1100, format()), Registry::Result::Stale); + EXPECT_EQ(registry.setAvailability(*original.token, Registry::Availability::Departed), Registry::Result::Stale); + EXPECT_EQ(registry.setSubscription(*original.token, false, false), Registry::Result::Stale); + EXPECT_EQ(registry.replace(*original.token, registration).result, Registry::Result::Stale); + const auto after = registry.snapshot(); + EXPECT_TRUE(before->sources[0].hasPublication); // Old snapshot cannot change. + EXPECT_FALSE(after->sources[0].hasPublication); + EXPECT_FALSE(after->sources[0].format.has_value()); + EXPECT_FALSE(after->sources[0].subscriptionObserved.value_or(false)); + EXPECT_EQ(after->sources[0].personId->value, "durable-person"); + EXPECT_EQ(after->sources[0].personGeneration, 1ULL); + EXPECT_EQ(registry.publish(*replacement.token, 0, 1, format()), Registry::Result::Applied); +} + +TEST(SourceRegistry, DepartureRequiresExplicitReplacementAndScopesReusedHandles) { + Registry registry("authority"); + const auto first = registry.add(participant("old-source")); + ASSERT_TRUE(first.token.has_value()); + EXPECT_EQ(registry.add(participant("ambiguous-source")).result, Registry::Result::Conflict); + EXPECT_EQ(registry.setAvailability(*first.token, Registry::Availability::Departed), Registry::Result::Applied); + EXPECT_EQ(registry.setAvailability(*first.token, Registry::Availability::Available), Registry::Result::Stale); + EXPECT_EQ(registry.publish(*first.token, 1, 100, format()), Registry::Result::Stale); + EXPECT_EQ(registry.add(participant("old-source")).result, Registry::Result::Conflict); + EXPECT_EQ(registry.add(participant("new-person-source")).result, Registry::Result::Applied); + EXPECT_EQ(registry.replace(*first.token, participant("old-source")).result, Registry::Result::Conflict); + auto nextMeeting = participant("old-source"); + nextMeeting.processEpoch = "meeting-2"; + EXPECT_EQ(registry.replace(*first.token, nextMeeting).result, Registry::Result::Applied); +} + +TEST(SourceRegistry, DeviceReplacementAndCameraShareAreSeparateInstances) { + Registry registry("authority"); + auto device = participant("device-slot"); + device.kind = Registry::Kind::Device; + device.externalId = "device-path-a"; + const auto first = registry.add(device); + ASSERT_TRUE(first.token.has_value()); + device.externalId = "device-path-b"; + const auto next = registry.replace(*first.token, device); + ASSERT_TRUE(next.token.has_value()); + EXPECT_EQ(registry.publish(*first.token, 1, 1, format()), Registry::Result::Stale); + EXPECT_EQ(registry.snapshot()->sources[0].externalId, "device-path-b"); + EXPECT_EQ(registry.add(participant("camera")).result, Registry::Result::Applied); + auto share = participant("share"); + share.kind = Registry::Kind::ParticipantShare; + EXPECT_EQ(registry.add(share).result, Registry::Result::Applied); + auto forged = *next.token; + forged.processEpoch = "old-process"; + EXPECT_EQ(registry.publish(forged, 1, 1, format()), Registry::Result::Stale); +} + +TEST(SourceRegistry, OutOfOrderPublicationCannotRefreshOrChangeFormat) { + Registry registry("authority"); + const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token.has_value()); + EXPECT_EQ(registry.publish(*added.token, 10, 100, format()), Registry::Result::Applied); + auto changed = format(); changed.width = 640; + EXPECT_EQ(registry.publish(*added.token, 10, 200, changed), Registry::Result::Stale); + EXPECT_EQ(registry.publish(*added.token, 11, 99, changed), Registry::Result::Stale); + EXPECT_EQ(registry.snapshot()->sources[0].format->width, 1920); + EXPECT_EQ(registry.snapshot()->sources[0].lastPublicationNs, 100); + registry.setAvailability(*added.token, Registry::Availability::Unavailable); + EXPECT_FALSE(registry.snapshot()->sources[0].hasPublication); + EXPECT_FALSE(registry.snapshot()->sources[0].format.has_value()); + EXPECT_EQ(registry.publish(*added.token, 12, 200, changed), Registry::Result::Stale); + EXPECT_EQ(registry.setSubscription(*added.token, true, true), Registry::Result::Invalid); +} + +TEST(SourceRegistry, ConcurrentReplacementHasExactlyOneWinnerAndFencesOldPublisher) { + Registry registry("authority"); + const auto first = registry.add(participant("camera")); + ASSERT_TRUE(first.token.has_value()); + std::atomic winners{0}; + const auto replace = [&] { + if (registry.replace(*first.token, participant("camera")).result == Registry::Result::Applied) ++winners; + }; + std::thread a(replace), b(replace); + a.join(); b.join(); + EXPECT_EQ(winners.load(), 1); + std::atomic accepted{0}; + std::thread stale([&] { + for (uint64_t i = 1; i < 100; ++i) + if (registry.publish(*first.token, i, static_cast(i), format()) == Registry::Result::Applied) ++accepted; + }); + for (int i = 0; i < 100; ++i) { + const auto snapshot = registry.snapshot(); + EXPECT_EQ(snapshot->sources[0].token.generation, 2u); + EXPECT_FALSE(snapshot->sources[0].hasPublication); + } + stale.join(); + EXPECT_EQ(accepted.load(), 0); +} + +TEST(SourceRegistry, ReplacedProcessEpochRetiresAllSourcesAndFencesEveryCallback) { + Registry registry("authority"); + const auto camera = registry.add(participant("camera", "42")); + auto shareRegistration = participant("share", "42"); + shareRegistration.kind = Registry::Kind::ParticipantShare; + const auto share = registry.add(shareRegistration); + ASSERT_TRUE(camera.token.has_value()); + ASSERT_TRUE(share.token.has_value()); + EXPECT_EQ(registry.setSubscription(*camera.token, true, true), Registry::Result::Applied); + const auto before = registry.snapshot()->revision; + EXPECT_EQ(registry.retireProcessEpoch("meeting-1"), Registry::Result::Applied); + const auto retired = registry.snapshot(); + EXPECT_EQ(retired->revision, before + 1); + ASSERT_EQ(retired->sources.size(), 2u); + for (const auto& source : retired->sources) { + EXPECT_EQ(source.availability, Registry::Availability::Departed); + EXPECT_FALSE(source.subscriptionRequested); + EXPECT_FALSE(source.subscriptionObserved.value_or(false)); + } + EXPECT_EQ(registry.publish(*camera.token, 1, 1, format()), Registry::Result::Stale); + EXPECT_EQ(registry.setSubscription(*share.token, true, true), Registry::Result::Stale); + EXPECT_EQ(registry.retireProcessEpoch("meeting-1"), Registry::Result::Unchanged); + EXPECT_EQ(registry.add(participant("late-source", "99")).result, Registry::Result::Invalid); + EXPECT_EQ(registry.replace(*camera.token, participant("camera", "77")).result, Registry::Result::Invalid); + + auto current = participant("camera", "77"); + current.processEpoch = "meeting-2"; + EXPECT_EQ(registry.replace(*camera.token, current).result, Registry::Result::Applied); + + EXPECT_EQ(registry.retireProcessEpoch("future-meeting"), Registry::Result::Applied); + auto delayed = participant("delayed", "88"); + delayed.processEpoch = "future-meeting"; + EXPECT_EQ(registry.add(delayed).result, Registry::Result::Invalid); +} + +TEST(SourceRegistry, SemanticNoOpsDoNotAdvanceRegistryRevision) { + Registry registry("authority"); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Alex"}), Registry::Result::Applied); + const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token.has_value()); + const auto revision = registry.snapshot()->revision; + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Alex"}), Registry::Result::Unchanged); + EXPECT_EQ(registry.setAvailability(*added.token, Registry::Availability::Available), Registry::Result::Unchanged); + EXPECT_EQ(registry.setSubscription(*added.token, false, false), Registry::Result::Unchanged); + EXPECT_EQ(registry.snapshot()->revision, revision); +} + +TEST(SourceRegistry, PersonGenerationIsExplicitAndCannotMoveBackward) { + Registry registry("authority"); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Alex", 2}), Registry::Result::Applied); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Renamed", 1}), Registry::Result::Stale); + EXPECT_EQ(registry.snapshot()->persons[0].displayName, "Alex"); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Renamed", 2}), Registry::Result::Applied); + EXPECT_EQ(registry.snapshot()->persons[0].generation, 2ULL); +} + +TEST(SourceRegistry, PublicationTimestampSupportsLongRunningMonotonicClocks) { + Registry registry("authority"); + const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token.has_value()); + constexpr int64_t afterOneHundredFourDays = 9'007'199'254'740'993LL; + EXPECT_EQ(registry.publish(*added.token, 1, afterOneHundredFourDays, format()), + Registry::Result::Applied); + EXPECT_EQ(registry.snapshot()->sources[0].lastPublicationNs, afterOneHundredFourDays); + EXPECT_EQ(registry.publish(*added.token, 2, (std::numeric_limits::max)(), format()), + Registry::Result::Applied); + EXPECT_EQ(registry.snapshot()->sources[0].lastPublicationNs, + (std::numeric_limits::max)()); + EXPECT_EQ(registry.publish(*added.token, 2, -1, format()), Registry::Result::Invalid); +} + +TEST(SourceRegistry, ProcessRetirementSerializesAgainstAddAndReplace) { + for (int iteration = 0; iteration < 100; ++iteration) { + Registry addRegistry("authority-add"); + std::thread add([&] { addRegistry.add(participant("camera")); }); + std::thread retire([&] { addRegistry.retireProcessEpoch("meeting-1"); }); + add.join(); + retire.join(); + const auto added = addRegistry.snapshot(); + for (const auto& source : added->sources) + EXPECT_NE(source.availability, Registry::Availability::Available); + + Registry replaceRegistry("authority-replace"); + const auto original = replaceRegistry.add(participant("camera")); + ASSERT_TRUE(original.token.has_value()); + std::thread replace([&] { replaceRegistry.replace(*original.token, participant("camera", "99")); }); + std::thread retireReplacement([&] { replaceRegistry.retireProcessEpoch("meeting-1"); }); + replace.join(); + retireReplacement.join(); + const auto replaced = replaceRegistry.snapshot(); + ASSERT_EQ(replaced->sources.size(), 1u); + EXPECT_EQ(replaced->sources[0].availability, Registry::Availability::Departed); + } +} + +TEST(SourceRegistry, IdentityAndEpochTombstoneAdmissionIsBounded) { + Registry registry("authority", 1, 1, 1); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "A"}), Registry::Result::Applied); + EXPECT_EQ(registry.upsertPerson({{"person-b"}, "B"}), Registry::Result::Exhausted); + EXPECT_EQ(registry.add(participant("camera-a", "1")).result, Registry::Result::Applied); + auto second = participant("camera-b", "2"); + second.processEpoch = "meeting-2"; + EXPECT_EQ(registry.add(second).result, Registry::Result::Exhausted); + EXPECT_EQ(registry.retireProcessEpoch("meeting-1"), Registry::Result::Applied); + EXPECT_EQ(registry.retireProcessEpoch("meeting-2"), Registry::Result::Exhausted); + EXPECT_EQ(registry.snapshot()->sources[0].availability, Registry::Availability::Departed); +} + +TEST(SourceRegistry, CameraReturnClearsReadinessButPreservesPublicationWatermarks) { + Registry registry("authority"); + const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token.has_value()); + const auto token = *added.token; + EXPECT_EQ(registry.publish(token, 10, 100, format()), Registry::Result::Applied); + EXPECT_EQ(registry.setSubscription(token, true, true), Registry::Result::Applied); + EXPECT_EQ(registry.setAvailability(token, Registry::Availability::Unavailable), Registry::Result::Applied); + const auto unavailable = registry.snapshot(); + EXPECT_FALSE(unavailable->sources[0].hasPublication); + EXPECT_FALSE(unavailable->sources[0].format.has_value()); + EXPECT_FALSE(unavailable->sources[0].subscriptionObserved.value_or(false)); + EXPECT_EQ(unavailable->sources[0].publicationSequence, 10ULL); + EXPECT_EQ(unavailable->sources[0].lastPublicationNs, 100); + EXPECT_EQ(registry.setAvailability(token, Registry::Availability::Available), Registry::Result::Applied); + const auto revision = registry.snapshot()->revision; + EXPECT_EQ(registry.publish(token, 9, 101, format()), Registry::Result::Stale); + EXPECT_EQ(registry.publish(token, 10, 101, format()), Registry::Result::Stale); + EXPECT_EQ(registry.publish(token, 11, 99, format()), Registry::Result::Stale); + EXPECT_EQ(registry.snapshot()->revision, revision); + EXPECT_FALSE(registry.snapshot()->sources[0].hasPublication); + EXPECT_EQ(registry.publish(token, 11, 100, format()), Registry::Result::Applied); + EXPECT_TRUE(registry.snapshot()->sources[0].hasPublication); + EXPECT_EQ(registry.setAvailability(token, Registry::Availability::Departed), Registry::Result::Applied); + EXPECT_EQ(registry.snapshot()->sources[0].publicationSequence, 11ULL); + EXPECT_FALSE(registry.snapshot()->sources[0].hasPublication); +} + +TEST(SourceRegistry, ZeroPublicationIsAcceptedOnceEvenAcrossCameraAvailabilityChanges) { + Registry registry("authority"); + const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token.has_value()); + const auto token = *added.token; + registry.setAvailability(token, Registry::Availability::Unavailable); + registry.setAvailability(token, Registry::Availability::Available); + EXPECT_EQ(registry.publish(token, 0, 0, format()), Registry::Result::Applied); + registry.setAvailability(token, Registry::Availability::Unavailable); + registry.setAvailability(token, Registry::Availability::Available); + EXPECT_EQ(registry.publish(token, 0, 0, format()), Registry::Result::Stale); + EXPECT_EQ(registry.publish(token, 1, 0, format()), Registry::Result::Applied); + const auto replacement = registry.replace(token, participant("camera")); + ASSERT_TRUE(replacement.token.has_value()); + EXPECT_EQ(registry.publish(*replacement.token, 0, 0, format()), Registry::Result::Applied); +} + +TEST(SourceRegistry, ExistingPersonUpdatesRemainAdmissibleAtCapacity) { + Registry registry("authority", 1, 1, 1); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "A", 1}), Registry::Result::Applied); + const auto initial = registry.snapshot(); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Renamed", 1}), Registry::Result::Applied); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Renamed", 2}), Registry::Result::Applied); + const auto updated = registry.snapshot(); + EXPECT_EQ(updated->revision, initial->revision + 2); + ASSERT_EQ(updated->persons.size(), 1u); + EXPECT_EQ(updated->persons[0].displayName, "Renamed"); + EXPECT_EQ(updated->persons[0].generation, 2ULL); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Renamed", 2}), Registry::Result::Unchanged); + EXPECT_EQ(registry.upsertPerson({{"person-a"}, "Old", 1}), Registry::Result::Stale); + EXPECT_EQ(registry.upsertPerson({{"person-b"}, "B", 1}), Registry::Result::Exhausted); + EXPECT_EQ(registry.snapshot()->revision, updated->revision); +} + +TEST(SourceRegistry, DisplayNameMetadataPreservesIncarnationAndFencesOldTokens) { + Registry registry("authority"); + const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token.has_value()); + registry.setSubscription(*added.token, true, true); + registry.publish(*added.token, 5, 100, format()); + const auto before = registry.snapshot(); + EXPECT_EQ(registry.setDisplayName(*added.token, "New name"), Registry::Result::Applied); + const auto renamed = registry.snapshot(); + EXPECT_EQ(renamed->revision, before->revision + 1); + EXPECT_EQ(renamed->sources[0].token.instanceId.value, before->sources[0].token.instanceId.value); + EXPECT_EQ(renamed->sources[0].token.generation, before->sources[0].token.generation); + EXPECT_EQ(renamed->sources[0].publicationSequence, 5ULL); + EXPECT_TRUE(renamed->sources[0].subscriptionObserved.value_or(false)); + EXPECT_EQ(before->sources[0].displayName, "Alex"); + EXPECT_EQ(registry.setDisplayName(*added.token, "New name"), Registry::Result::Unchanged); + EXPECT_EQ(registry.snapshot()->revision, renamed->revision); + EXPECT_EQ(registry.setDisplayName(*added.token, std::string(4097, 'x')), Registry::Result::Invalid); + EXPECT_EQ(registry.setDisplayName(*added.token, ""), Registry::Result::Applied); + const auto replacement = registry.replace(*added.token, participant("camera")); + ASSERT_TRUE(replacement.token.has_value()); + const auto replacedRevision = registry.snapshot()->revision; + EXPECT_EQ(registry.setDisplayName(*added.token, "Stale name"), Registry::Result::Stale); + EXPECT_EQ(registry.snapshot()->revision, replacedRevision); + EXPECT_EQ(registry.snapshot()->sources[0].displayName, "Alex"); + registry.retireProcessEpoch("meeting-1"); + EXPECT_EQ(registry.setDisplayName(*replacement.token, "Late name"), Registry::Result::Stale); +} + +TEST(SourceRegistry, ProviderInstanceIdsAreExactBoundedAndUniqueWhileLive) { + Registry registry("authority"); + auto a = participant("a", "1"); + a.instanceId = corevideo::core::SourceInstanceId{"provider-instance"}; + const auto first = registry.add(a); + ASSERT_TRUE(first.token.has_value()); + EXPECT_EQ(first.token->instanceId.value, "provider-instance"); + auto b = participant("b", "2"); b.instanceId = a.instanceId; + const auto revision = registry.snapshot()->revision; + EXPECT_EQ(registry.add(b).result, Registry::Result::Conflict); + b.instanceId = corevideo::core::SourceInstanceId{""}; + EXPECT_EQ(registry.add(b).result, Registry::Result::Invalid); + b.instanceId = corevideo::core::SourceInstanceId{std::string(513, 'x')}; + EXPECT_EQ(registry.add(b).result, Registry::Result::Invalid); + EXPECT_EQ(registry.snapshot()->revision, revision); + b.instanceId = corevideo::core::SourceInstanceId{std::string(512, 'x')}; + const auto second = registry.add(b); + ASSERT_TRUE(second.token.has_value()); + b.instanceId = a.instanceId; + EXPECT_EQ(registry.replace(*second.token, b).result, Registry::Result::Conflict); + registry.setAvailability(*first.token, Registry::Availability::Unavailable); + EXPECT_EQ(registry.replace(*second.token, b).result, Registry::Result::Conflict); + registry.setAvailability(*first.token, Registry::Availability::Departed); + EXPECT_EQ(registry.replace(*second.token, b).result, Registry::Result::Applied); +} + +TEST(SourceRegistry, RegistryAllocatedInstanceCannotCollideWithProviderInstance) { + Registry registry("authority"); + auto a = participant("a", "1"); + a.instanceId = corevideo::core::SourceInstanceId{"authority:2"}; + ASSERT_EQ(registry.add(a).result, Registry::Result::Applied); + EXPECT_EQ(registry.add(participant("b", "2")).result, Registry::Result::Conflict); +} + +TEST(SourceRegistry, ProviderGenerationAcceptsInitialAndForwardGapsButNeverRollsBack) { + Registry registry("authority"); auto registration = participant("camera"); + registration.requestedGeneration = 7; + const auto first = registry.add(registration); + ASSERT_TRUE(first.token.has_value()); + EXPECT_EQ(first.token->generation, 7ULL); + registration.requestedGeneration = 7; + EXPECT_EQ(registry.replace(*first.token, registration).result, Registry::Result::Stale); + registration.requestedGeneration = 6; + EXPECT_EQ(registry.replace(*first.token, registration).result, Registry::Result::Stale); + registration.requestedGeneration = 0; + EXPECT_EQ(registry.replace(*first.token, registration).result, Registry::Result::Invalid); + registration.requestedGeneration = 9007199254740992ULL; + EXPECT_EQ(registry.replace(*first.token, registration).result, Registry::Result::Invalid); + registration.requestedGeneration = 10; + const auto next = registry.replace(*first.token, registration); + ASSERT_TRUE(next.token.has_value()); + EXPECT_EQ(next.token->generation, 10ULL); + registration.requestedGeneration.reset(); + const auto allocated = registry.replace(*next.token, registration); + ASSERT_TRUE(allocated.token.has_value()); + EXPECT_EQ(allocated.token->generation, 11ULL); + auto invalid = participant("other", "99"); invalid.requestedGeneration = 0; + EXPECT_EQ(registry.add(invalid).result, Registry::Result::Invalid); +} + +TEST(SourceRegistry, DecisionRevisionIgnoresFrameTrafficAndMetadataButTracksEligibility) { + Registry registry("authority"); + registry.upsertPerson({{"person"}, "Name", 1}); + EXPECT_EQ(registry.snapshot()->decisionRevision, 1ULL); + registry.upsertPerson({{"person"}, "Renamed", 1}); + EXPECT_EQ(registry.snapshot()->decisionRevision, 1ULL); + registry.upsertPerson({{"person"}, "Renamed", 2}); + EXPECT_EQ(registry.snapshot()->decisionRevision, 2ULL); + const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token.has_value()); + EXPECT_EQ(registry.snapshot()->decisionRevision, 3ULL); + registry.setSubscription(*added.token, true, true); + EXPECT_EQ(registry.snapshot()->decisionRevision, 3ULL); + registry.publish(*added.token, 0, 0, format()); + EXPECT_EQ(registry.snapshot()->decisionRevision, 4ULL); + const auto auditBefore = registry.snapshot()->revision; + for (uint64_t frame = 1; frame <= 60; ++frame) + EXPECT_EQ(registry.publish(*added.token, frame, static_cast(frame), format()), Registry::Result::Applied); + EXPECT_EQ(registry.snapshot()->decisionRevision, 4ULL); + EXPECT_EQ(registry.snapshot()->revision, auditBefore + 60); + auto changedFormat = format(); changedFormat.width = 1280; + EXPECT_EQ(registry.publish(*added.token, 61, 61, changedFormat), Registry::Result::Applied); + EXPECT_EQ(registry.snapshot()->decisionRevision, 5ULL); + registry.setAvailability(*added.token, Registry::Availability::Unavailable); + EXPECT_EQ(registry.snapshot()->decisionRevision, 6ULL); + registry.setAvailability(*added.token, Registry::Availability::Available); + EXPECT_EQ(registry.snapshot()->decisionRevision, 7ULL); + registry.publish(*added.token, 62, 62, format()); + EXPECT_EQ(registry.snapshot()->decisionRevision, 8ULL); + const auto replaced = registry.replace(*added.token, participant("camera")); + ASSERT_TRUE(replaced.token.has_value()); + EXPECT_EQ(registry.snapshot()->decisionRevision, 9ULL); + registry.retireProcessEpoch("meeting-1"); + EXPECT_EQ(registry.snapshot()->decisionRevision, 10ULL); + registry.retireProcessEpoch("meeting-1"); + EXPECT_EQ(registry.snapshot()->decisionRevision, 10ULL); +} +TEST(SourceRegistry, IdentityAndFormatTextAreBoundedBeforeRetention) { + Registry registry("authority"); + EXPECT_EQ(registry.upsertPerson({{std::string(513,'p')},"",1}),Registry::Result::Invalid); + EXPECT_EQ(registry.upsertPerson({{"person"},std::string(4097,'n'),1}),Registry::Result::Invalid); + auto registration = participant(std::string(513,'s')); + EXPECT_EQ(registry.add(registration).result,Registry::Result::Invalid); + registration = participant("camera"); registration.externalId = std::string(513,'e'); + EXPECT_EQ(registry.add(registration).result,Registry::Result::Invalid); + registration = participant("camera"); const auto added = registry.add(registration); + ASSERT_TRUE(added.token.has_value()); + auto oversizedFormat = format(); oversizedFormat.pixelFormat = std::string(129,'f'); + EXPECT_EQ(registry.publish(*added.token,1,1,oversizedFormat),Registry::Result::Invalid); +} + +TEST(SourceRegistry, UnknownFrameRateAndAcknowledgementRemainDistinctFromKnownValues) { + Registry registry("authority"); const auto added = registry.add(participant("camera")); + ASSERT_TRUE(added.token); + EXPECT_EQ(registry.setSubscription(*added.token, true, std::nullopt), Registry::Result::Applied); + Registry::Format unknown{1920, 1080, std::nullopt, std::nullopt, "I420"}; + EXPECT_EQ(registry.publish(*added.token, 1, 100, unknown), Registry::Result::Applied); + auto snapshot = registry.snapshot(); + EXPECT_TRUE(snapshot->sources[0].hasPublication); + EXPECT_FALSE(snapshot->sources[0].subscriptionObserved.has_value()); + EXPECT_FALSE(snapshot->sources[0].format->fpsNumerator.has_value()); + EXPECT_FALSE(snapshot->sources[0].format->fpsDenominator.has_value()); + unknown.fpsDenominator = 1; + EXPECT_EQ(registry.publish(*added.token, 2, 101, unknown), Registry::Result::Invalid); + EXPECT_EQ(registry.snapshot()->revision, snapshot->revision); + EXPECT_EQ(registry.publish(*added.token, 2, 101, format()), Registry::Result::Applied); + EXPECT_EQ(registry.setSubscription(*added.token, true, false), Registry::Result::Applied); + EXPECT_EQ(registry.snapshot()->sources[0].subscriptionObserved, std::optional{false}); + EXPECT_EQ(registry.snapshot()->sources[0].format->fpsNumerator, std::optional{60}); +} From 7bbc4f5dd3b61ab2f0e8fd5af86946997543a68d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 19:07:35 -0400 Subject: [PATCH 06/20] feat(registry): admit composed sources, and never claim a subscription state for them Also fixes a dangling-reference bug in the ACaptureSourceStillCarriesItsCaptureFields test: `registry.snapshot()->sources.front()` as one expression drops the temporary shared_ptr (and its Snapshot) at the semicolon, leaving `source` a dangling reference. Bind the shared_ptr first, same pattern already used by the other new test in this file. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/CMakeLists.txt | 1 + native/src/core/SourceRegistry.cpp | 36 +++++++-- native/src/core/SourceRegistry.h | 16 ++-- native/tests/SourceRegistryComposedTest.cpp | 82 +++++++++++++++++++++ native/tests/SourceRegistryTest.cpp | 12 +-- 5 files changed, 131 insertions(+), 16 deletions(-) create mode 100644 native/tests/SourceRegistryComposedTest.cpp diff --git a/native/CMakeLists.txt b/native/CMakeLists.txt index 6964dd76..b945e236 100644 --- a/native/CMakeLists.txt +++ b/native/CMakeLists.txt @@ -621,6 +621,7 @@ if(BUILD_TESTING) tests/ContractParityTest.cpp tests/RouteSourcePolicyTest.cpp tests/SourceRegistryTest.cpp + tests/SourceRegistryComposedTest.cpp tests/EmptyRouteBlankTest.cpp tests/OutputLifecyclePolicyTest.cpp tests/MonitorShedPolicyTest.cpp diff --git a/native/src/core/SourceRegistry.cpp b/native/src/core/SourceRegistry.cpp index b5dd1f40..5a4f12f0 100644 --- a/native/src/core/SourceRegistry.cpp +++ b/native/src/core/SourceRegistry.cpp @@ -47,20 +47,32 @@ SourceRegistry::Result SourceRegistry::upsertPerson(Person person) { } bool SourceRegistry::validRegistration(const Registration& r) const { - const bool knownKind = r.kind == Kind::ParticipantVideo || r.kind == Kind::ParticipantShare || + const bool composed = r.kind == Kind::Composed; + const bool knownKind = composed || r.kind == Kind::ParticipantVideo || r.kind == Kind::ParticipantShare || r.kind == Kind::Device || r.kind == Kind::Media || r.kind == Kind::Browser; - return knownKind && (!r.requestedGeneration || (*r.requestedGeneration > 0 && *r.requestedGeneration <= kMaxRevision)) && + // A composed source is identified by its sourceId alone: it has no SDK handle, + // so requiring an externalId would reject every wall outright. + const bool externalIdOk = composed + ? r.externalId.empty() + : (!r.externalId.empty() && r.externalId.size() <= 512); + return knownKind && externalIdOk && + (!r.requestedGeneration || (*r.requestedGeneration > 0 && *r.requestedGeneration <= kMaxRevision)) && (!r.instanceId || (!r.instanceId->value.empty() && r.instanceId->value.size() <= 512)) && !r.sourceId.value.empty() && r.sourceId.value.size() <= 512 && !r.processEpoch.empty() && r.processEpoch.size() <= 512 && - !retiredProcessEpochs_.contains(r.processEpoch) && !r.externalId.empty() && - r.externalId.size() <= 512 && r.displayName.size() <= 4096 && + !retiredProcessEpochs_.contains(r.processEpoch) && + r.displayName.size() <= 4096 && ((!r.personId && r.personGeneration == 0) || (r.personId && persons_.contains(r.personId->value) && r.personGeneration > 0 && persons_.at(r.personId->value).generation == r.personGeneration)); } bool SourceRegistry::externalConflict(const Registration& r) const { + // A composed source has no externalId to collide on; two walls are distinct + // whenever their sourceIds differ. + if (r.kind == Kind::Composed) { + return false; + } for (const auto& entry : sources_) { const auto& source = entry.second; if (entry.first != r.sourceId.value && source.availability != Availability::Departed && @@ -84,7 +96,21 @@ SourceRegistry::Mutation SourceRegistry::install(Registration r, uint64_t genera source.personId = std::move(r.personId); source.personGeneration = r.personGeneration; source.displayName = std::move(r.displayName); - source.externalId = std::move(r.externalId); + if (r.kind == Kind::Composed) { + // A composed source has no SDK handle, no availability concept (it lives + // and dies with the core process), and is never subscribed. Leaving these + // nullopt is the whole point: a stray `false` here would let a consumer + // read "not subscribed" as an observed fact about a wall. + source.externalId.reset(); + source.availability.reset(); + source.subscriptionRequested.reset(); + source.subscriptionObserved.reset(); + } else { + source.externalId = std::move(r.externalId); + source.availability = Availability::Available; + source.subscriptionRequested = false; + source.subscriptionObserved = false; + } const auto token = source.token; sources_[r.sourceId.value] = std::move(source); ++revision_; diff --git a/native/src/core/SourceRegistry.h b/native/src/core/SourceRegistry.h index 7e1a4629..3c3575de 100644 --- a/native/src/core/SourceRegistry.h +++ b/native/src/core/SourceRegistry.h @@ -19,7 +19,11 @@ struct SourceInstanceId { std::string value; }; class SourceRegistry final { public: - enum class Kind { ParticipantVideo, ParticipantShare, Device, Media, Browser }; + // Composed: a source the CORE renders rather than captures (the Tiles wall + // today; lower-thirds and graphics in slice 3). It has no SDK handle, no + // person, and is never subscribed - so the capture-only fields below stay + // nullopt for it, and "nullopt" means NOT APPLICABLE, never false. + enum class Kind { ParticipantVideo, ParticipantShare, Device, Media, Browser, Composed }; enum class Availability { Available, Unavailable, Departed }; enum class Result { Applied, Unchanged, Invalid, NotFound, Conflict, Stale, Exhausted }; struct Format { @@ -55,10 +59,12 @@ class SourceRegistry final { Kind kind = Kind::ParticipantVideo; std::optional personId; uint64_t personGeneration = 0; - std::string displayName, externalId; - Availability availability = Availability::Available; - bool subscriptionRequested = false; - std::optional subscriptionObserved{false}; // nullopt means unacknowledged/unknown. + std::string displayName; + // nullopt = NOT APPLICABLE to this kind (e.g. a Composed wall). Never read as false. + std::optional externalId; + std::optional availability; + std::optional subscriptionRequested; + std::optional subscriptionObserved; // nullopt means unacknowledged/unknown/not applicable. std::optional format; bool hasPublication = false; bool hasPublicationWatermark = false; // Survives unavailable/departed until token replacement. diff --git a/native/tests/SourceRegistryComposedTest.cpp b/native/tests/SourceRegistryComposedTest.cpp new file mode 100644 index 00000000..21e90443 --- /dev/null +++ b/native/tests/SourceRegistryComposedTest.cpp @@ -0,0 +1,82 @@ +#include "core/SourceRegistry.h" + +#include + +namespace { +using corevideo::core::SourceRegistry; + +SourceRegistry::Registration wallRegistration(const std::string& id) { + SourceRegistry::Registration registration; + registration.sourceId = {id}; + registration.kind = SourceRegistry::Kind::Composed; + registration.displayName = "Gallery"; + // A wall lives and dies with the core process, so the core's epoch is its epoch. + registration.processEpoch = "core-epoch-1"; + // externalId deliberately left EMPTY: a wall has no SDK handle. + return registration; +} + +// A wall has no SDK handle, and validRegistration rejects an empty externalId +// today, so add() answers Invalid and the wall can never be registered. +TEST(SourceRegistryComposed, AWallIsAdmittedWithoutAnExternalId) { + SourceRegistry registry("registry-epoch-1"); + const auto mutation = registry.add(wallRegistration("tiles:scene-a")); + EXPECT_EQ(mutation.result, SourceRegistry::Result::Applied); + ASSERT_TRUE(mutation.token.has_value()); + EXPECT_EQ(mutation.token->sourceId.value, "tiles:scene-a"); +} + +// externalConflict matches on kind + processEpoch + externalId, so two walls +// that both have an EMPTY externalId look like duplicates of each other. +TEST(SourceRegistryComposed, TwoWallsWithNoExternalIdDoNotCollide) { + SourceRegistry registry("registry-epoch-1"); + ASSERT_EQ(registry.add(wallRegistration("tiles:scene-a")).result, + SourceRegistry::Result::Applied); + const auto second = registry.add(wallRegistration("tiles:scene-b")); + EXPECT_EQ(second.result, SourceRegistry::Result::Applied); +} + +// The load-bearing honesty test. subscriptionObserved is initialised ENGAGED +// with the value false, and its own comment says nullopt means unknown - so a +// registered wall would otherwise ASSERT "subscription observed = false" into +// ShowPlanGenerator, which consumes this snapshot. +TEST(SourceRegistryComposed, AComposedWallNeverClaimsASubscriptionState) { + SourceRegistry registry("registry-epoch-1"); + ASSERT_EQ(registry.add(wallRegistration("tiles:scene-a")).result, + SourceRegistry::Result::Applied); + + const auto snapshot = registry.snapshot(); + ASSERT_NE(snapshot, nullptr); + ASSERT_EQ(snapshot->sources.size(), 1U); + const auto& wall = snapshot->sources.front(); + + EXPECT_EQ(wall.kind, SourceRegistry::Kind::Composed); + EXPECT_FALSE(wall.personId.has_value()); + EXPECT_FALSE(wall.externalId.has_value()); + EXPECT_FALSE(wall.availability.has_value()); + EXPECT_FALSE(wall.subscriptionRequested.has_value()); + EXPECT_FALSE(wall.subscriptionObserved.has_value()); +} + +// A capture source is unchanged: it still carries every field it always did. +TEST(SourceRegistryComposed, ACaptureSourceStillCarriesItsCaptureFields) { + SourceRegistry registry("registry-epoch-1"); + SourceRegistry::Registration camera; + camera.sourceId = {"camera-alice"}; + camera.kind = SourceRegistry::Kind::ParticipantVideo; + camera.displayName = "Alice"; + camera.processEpoch = "zoom-process-1"; + camera.externalId = "alice-sdk-id"; + ASSERT_EQ(registry.add(camera).result, SourceRegistry::Result::Applied); + + // Bind the shared_ptr before taking a reference into it: `snapshot()->sources.front()` + // as one expression leaves `source` dangling the instant the temporary shared_ptr's + // refcount drops to zero at the semicolon. + const auto snapshot = registry.snapshot(); + const auto& source = snapshot->sources.front(); + ASSERT_TRUE(source.externalId.has_value()); + EXPECT_EQ(*source.externalId, "alice-sdk-id"); + ASSERT_TRUE(source.availability.has_value()); + EXPECT_EQ(*source.availability, SourceRegistry::Availability::Available); +} +} // namespace diff --git a/native/tests/SourceRegistryTest.cpp b/native/tests/SourceRegistryTest.cpp index 0106f9e1..0c35ac39 100644 --- a/native/tests/SourceRegistryTest.cpp +++ b/native/tests/SourceRegistryTest.cpp @@ -100,7 +100,7 @@ TEST(SourceRegistry, DeviceReplacementAndCameraShareAreSeparateInstances) { const auto next = registry.replace(*first.token, device); ASSERT_TRUE(next.token.has_value()); EXPECT_EQ(registry.publish(*first.token, 1, 1, format()), Registry::Result::Stale); - EXPECT_EQ(registry.snapshot()->sources[0].externalId, "device-path-b"); + EXPECT_EQ(*registry.snapshot()->sources[0].externalId, "device-path-b"); EXPECT_EQ(registry.add(participant("camera")).result, Registry::Result::Applied); auto share = participant("share"); share.kind = Registry::Kind::ParticipantShare; @@ -167,8 +167,8 @@ TEST(SourceRegistry, ReplacedProcessEpochRetiresAllSourcesAndFencesEveryCallback EXPECT_EQ(retired->revision, before + 1); ASSERT_EQ(retired->sources.size(), 2u); for (const auto& source : retired->sources) { - EXPECT_EQ(source.availability, Registry::Availability::Departed); - EXPECT_FALSE(source.subscriptionRequested); + EXPECT_EQ(*source.availability, Registry::Availability::Departed); + EXPECT_FALSE(*source.subscriptionRequested); EXPECT_FALSE(source.subscriptionObserved.value_or(false)); } EXPECT_EQ(registry.publish(*camera.token, 1, 1, format()), Registry::Result::Stale); @@ -232,7 +232,7 @@ TEST(SourceRegistry, ProcessRetirementSerializesAgainstAddAndReplace) { retire.join(); const auto added = addRegistry.snapshot(); for (const auto& source : added->sources) - EXPECT_NE(source.availability, Registry::Availability::Available); + EXPECT_NE(*source.availability, Registry::Availability::Available); Registry replaceRegistry("authority-replace"); const auto original = replaceRegistry.add(participant("camera")); @@ -243,7 +243,7 @@ TEST(SourceRegistry, ProcessRetirementSerializesAgainstAddAndReplace) { retireReplacement.join(); const auto replaced = replaceRegistry.snapshot(); ASSERT_EQ(replaced->sources.size(), 1u); - EXPECT_EQ(replaced->sources[0].availability, Registry::Availability::Departed); + EXPECT_EQ(*replaced->sources[0].availability, Registry::Availability::Departed); } } @@ -257,7 +257,7 @@ TEST(SourceRegistry, IdentityAndEpochTombstoneAdmissionIsBounded) { EXPECT_EQ(registry.add(second).result, Registry::Result::Exhausted); EXPECT_EQ(registry.retireProcessEpoch("meeting-1"), Registry::Result::Applied); EXPECT_EQ(registry.retireProcessEpoch("meeting-2"), Registry::Result::Exhausted); - EXPECT_EQ(registry.snapshot()->sources[0].availability, Registry::Availability::Departed); + EXPECT_EQ(*registry.snapshot()->sources[0].availability, Registry::Availability::Departed); } TEST(SourceRegistry, CameraReturnClearsReadinessButPreservesPublicationWatermarks) { From bb619082afa71d2f98aa5c9976a0a71c162421c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 19:15:41 -0400 Subject: [PATCH 07/20] fix(registry): skip composed sources in retirement/availability, and narrow the composed conflict bypass to externalId Fix round 1 review findings: 1. retireProcessEpoch and setAvailability reached a composed source's fields unconditionally, reintroducing the false claim install() was fixed to remove (nullopt flipping to concrete Departed/false). Both now SKIP a composed source instead of marking/mutating it: retirement has nothing to fence (no provider process, no callbacks) and availability does not apply to a wall at all. Not erased, not marked Departed - a wall's lifetime is scene-reference, released by a separate sweep in a later task. 2. externalConflict's composed bypass disabled the instanceId collision check too, not just the externalId one. Restructured so only the externalId clause is skipped for a composed registration; two composed registrations sharing an explicit instanceId still conflict. Adds 3 covering tests, two confirmed red (via a temporary guard revert) before the fix and green after. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/core/SourceRegistry.cpp | 24 +++++++-- native/tests/SourceRegistryComposedTest.cpp | 54 +++++++++++++++++++++ 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/native/src/core/SourceRegistry.cpp b/native/src/core/SourceRegistry.cpp index 5a4f12f0..4766cb05 100644 --- a/native/src/core/SourceRegistry.cpp +++ b/native/src/core/SourceRegistry.cpp @@ -68,15 +68,16 @@ bool SourceRegistry::validRegistration(const Registration& r) const { } bool SourceRegistry::externalConflict(const Registration& r) const { - // A composed source has no externalId to collide on; two walls are distinct - // whenever their sourceIds differ. - if (r.kind == Kind::Composed) { - return false; - } + const bool composed = r.kind == Kind::Composed; for (const auto& entry : sources_) { const auto& source = entry.second; if (entry.first != r.sourceId.value && source.availability != Availability::Departed && r.instanceId && source.token.instanceId.value == r.instanceId->value) return true; + // A composed source has no externalId to collide on; two walls are + // distinct whenever their sourceIds differ. Only skip THIS clause for a + // composed registration - the instanceId check above still applies, so + // two walls sharing an explicit instanceId still conflict. + if (composed) continue; if (entry.first != r.sourceId.value && source.availability != Availability::Departed && source.kind == r.kind && source.token.processEpoch == r.processEpoch && source.externalId == r.externalId) return true; @@ -162,6 +163,11 @@ SourceRegistry::Result SourceRegistry::setAvailability(const Token& token, Avail if (found == sources_.end()) return Result::NotFound; auto& source = found->second; if (!sameToken(source.token, token) || source.availability == Availability::Departed) return Result::Stale; + // A composed source has no availability concept - nullopt means NOT + // APPLICABLE, not "unknown Available/Unavailable". Applying this call to one + // would flip subscriptionObserved from nullopt to a concrete `false`, the + // exact false claim this registry exists to refuse to make. + if (source.kind == Kind::Composed) return Result::Invalid; if (availability != Availability::Available && availability != Availability::Unavailable && availability != Availability::Departed) return Result::Invalid; if (source.availability == availability) return Result::Unchanged; @@ -207,6 +213,14 @@ SourceRegistry::Result SourceRegistry::retireProcessEpoch(const std::string& pro for (auto& entry : sources_) { auto& source = entry.second; if (source.token.processEpoch != processEpoch) continue; + // A composed source has no provider process and no callbacks to fence - + // retirement exists to stop a dead helper's late callbacks from looking + // current, which does not apply to a wall. SKIP it, do not mark it + // Departed: that would flip its nullopt fields to concrete false/Departed, + // the exact false claim this task removed from install(). A wall's + // lifetime is scene-reference, released by a separate sweep (later task) - + // do not "fix" this skip into a mark. + if (source.kind == Kind::Composed) continue; source.availability = Availability::Departed; source.subscriptionRequested = false; source.subscriptionObserved = false; diff --git a/native/tests/SourceRegistryComposedTest.cpp b/native/tests/SourceRegistryComposedTest.cpp index 21e90443..1f9f0603 100644 --- a/native/tests/SourceRegistryComposedTest.cpp +++ b/native/tests/SourceRegistryComposedTest.cpp @@ -79,4 +79,58 @@ TEST(SourceRegistryComposed, ACaptureSourceStillCarriesItsCaptureFields) { ASSERT_TRUE(source.availability.has_value()); EXPECT_EQ(*source.availability, SourceRegistry::Availability::Available); } + +// Fix round 1, finding 1: retireProcessEpoch iterates every source under the +// retired epoch unconditionally (it exists to fence a dead PROVIDER PROCESS's +// late callbacks) - a wall has no provider process and no callbacks to fence, +// so it must be SKIPPED, not marked Departed. Marking it would flip its +// nullopt fields to concrete Departed/false, the exact false claim install() +// was fixed to stop making. +TEST(SourceRegistryComposed, RetiringItsEpochLeavesAWallsFieldsUntouched) { + SourceRegistry registry("registry-epoch-1"); + ASSERT_EQ(registry.add(wallRegistration("tiles:scene-a")).result, + SourceRegistry::Result::Applied); + + EXPECT_EQ(registry.retireProcessEpoch("core-epoch-1"), SourceRegistry::Result::Applied); + + const auto snapshot = registry.snapshot(); + ASSERT_EQ(snapshot->sources.size(), 1U); + const auto& wall = snapshot->sources.front(); + EXPECT_FALSE(wall.availability.has_value()); + EXPECT_FALSE(wall.subscriptionRequested.has_value()); + EXPECT_FALSE(wall.subscriptionObserved.has_value()); +} + +// Fix round 1, finding 1 (second path): setAvailability has the identical +// unconditional `subscriptionObserved = false` on any non-Available +// transition. A composed source has no availability concept at all, so this +// call does not apply to it and must be refused rather than silently +// asserting a subscription state. +TEST(SourceRegistryComposed, SetAvailabilityOnAWallLeavesSubscriptionObservedUnclaimed) { + SourceRegistry registry("registry-epoch-1"); + const auto added = registry.add(wallRegistration("tiles:scene-a")); + ASSERT_EQ(added.result, SourceRegistry::Result::Applied); + ASSERT_TRUE(added.token.has_value()); + + EXPECT_EQ(registry.setAvailability(*added.token, SourceRegistry::Availability::Unavailable), + SourceRegistry::Result::Invalid); + + const auto snapshot = registry.snapshot(); + ASSERT_EQ(snapshot->sources.size(), 1U); + EXPECT_FALSE(snapshot->sources.front().subscriptionObserved.has_value()); +} + +// Fix round 1, finding 2: externalConflict's composed bypass must skip only +// the externalId clause. The instanceId collision check is identity, not an +// SDK-handle concept, and still applies to a composed registration. +TEST(SourceRegistryComposed, TwoWallsSharingAnInstanceIdStillConflict) { + SourceRegistry registry("registry-epoch-1"); + auto first = wallRegistration("tiles:scene-a"); + first.instanceId = corevideo::core::SourceInstanceId{"shared-wall-instance"}; + ASSERT_EQ(registry.add(first).result, SourceRegistry::Result::Applied); + + auto second = wallRegistration("tiles:scene-b"); + second.instanceId = corevideo::core::SourceInstanceId{"shared-wall-instance"}; + EXPECT_EQ(registry.add(second).result, SourceRegistry::Result::Conflict); +} } // namespace From 7288aa38fc7747be255156ae571cf4fe83698c20 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 19:21:17 -0400 Subject: [PATCH 08/20] feat(tiles): a wall owns its own animation, keyed by wall id TilesWallSource wraps TilesPlanAnimation and bumps a generation whenever the wrapped animation reports it reset, giving the take record's proof that nothing restarted (spec section 5) as a number instead of an opinion. TilesWallSources maps wall id -> source so both buses asking for the same wall get the same object; lifetime follows "referenced by a scene" via releaseAllExcept, and a recreated wall starts at generation 0 rather than continuing a retired one's count. TilesPlanAnimation::advance now returns bool (true when it reset the animator) instead of void, so TilesWallSource::advance can detect a reset with no std::function/allocation on the render tick. Every existing caller (MediaCore, TilesAnimatorTest) already discards the return value as a statement, so this is a source-compatible widening. Not wired into MediaCore yet - that is the next task. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/CMakeLists.txt | 1 + native/src/compositor/TilesPlanAnimation.h | 13 +++- native/src/core/TilesWallSource.h | 85 ++++++++++++++++++++++ native/tests/TilesWallSourceTest.cpp | 58 +++++++++++++++ 4 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 native/src/core/TilesWallSource.h create mode 100644 native/tests/TilesWallSourceTest.cpp diff --git a/native/CMakeLists.txt b/native/CMakeLists.txt index b945e236..7d6d66b6 100644 --- a/native/CMakeLists.txt +++ b/native/CMakeLists.txt @@ -669,6 +669,7 @@ if(BUILD_TESTING) tests/TilesAnimatorTest.cpp tests/TilesLayerTest.cpp tests/TilesMembershipTest.cpp + tests/TilesWallSourceTest.cpp tests/TilesRenderPlanTest.cpp tests/UvcCaptureSupportTest.cpp tests/VstHostAbiTest.cpp diff --git a/native/src/compositor/TilesPlanAnimation.h b/native/src/compositor/TilesPlanAnimation.h index 1644aee9..e1272bd5 100644 --- a/native/src/compositor/TilesPlanAnimation.h +++ b/native/src/compositor/TilesPlanAnimation.h @@ -42,13 +42,17 @@ class TilesPlanAnimation { return true; } - void advance(modules::CompositorRenderPlan& plan, const std::string& wallKey, + // Returns true when this call RESET the animator (a departure/disable, or a + // different wall key arriving) - the caller's only truthful signal of "did + // this wall restart", with no std::function/allocation on the render tick. + bool advance(modules::CompositorRenderPlan& plan, const std::string& wallKey, bool present, bool enabled, double durationMs, double nowMs) { - if (!present || !enabled) { reset(); return; } + if (!present || !enabled) { reset(); return true; } // A DIFFERENT wall never inherits this one's geometry. (sampled_ is cleared // too: the all-stale guard below would otherwise let a new wall's first // frames be drawn at the previous wall's tile rects.) - if (key_ != wallKey) { animator_.reset(); key_ = wallKey; sampled_.clear(); } + bool didReset = false; + if (key_ != wallKey) { animator_.reset(); key_ = wallKey; sampled_.clear(); didReset = true; } std::vector targets; for (const auto& layer : plan.layers) { if (layer.kind == "participant-video" && layer.layerId.rfind("tile:", 0) == 0) @@ -64,9 +68,10 @@ class TilesPlanAnimation { // fix exists to remove. Only a wall that has actually drawn tiles // preserves them; a cold wall's first tick is untouched, so a genuinely // new wall behaves exactly as it always has. - if (targets.empty() && !sampled_.empty()) return; + if (targets.empty() && !sampled_.empty()) return didReset; sampled_ = animator_.sample(targets, nowMs, enabled, durationMs, plan.width, plan.height); applyLatest(plan, wallKey); + return didReset; } void applyLatest(modules::CompositorRenderPlan& plan, const std::string& wallKey) const { if (key_ != wallKey) return; diff --git a/native/src/core/TilesWallSource.h b/native/src/core/TilesWallSource.h new file mode 100644 index 00000000..2bb32851 --- /dev/null +++ b/native/src/core/TilesWallSource.h @@ -0,0 +1,85 @@ +#pragma once + +#include "compositor/TilesPlanAnimation.h" + +#include +#include +#include +#include + +namespace corevideo::core { + +// One wall's animation, owned by the WALL rather than by a bus. +// +// Before this existed, MediaCore held programTilesAnimation_ and +// previewTilesAnimation_, and a Take handed settled state from one to the other +// (TilesPlanAnimation::adoptSettledFrom). That hand-off REFUSED a wall whose +// tiles were still flying - "mid-flight state belongs to the bus that is flying +// it" - because with two animators there is no correct answer. So a wall taken +// mid-animation re-animated on the cut, which is #448. +// +// With one animator there is nothing to hand over: both buses sample the same +// object, and a cut changes only which bus is looking at it. +class TilesWallSource final { + public: + // Wraps the animation so a reset can never happen without the generation + // moving. TilesPlanAnimation::advance returns true when it reset the + // animator; a plain bool return keeps this allocation-free on the render tick + // (a std::function callback would not be). + void advance(modules::CompositorRenderPlan& plan, const std::string& wallId, bool present, + bool enabled, double durationMs, double nowMs) { + if (animation_.advance(plan, wallId, present, enabled, durationMs, nowMs)) { + noteReset(); + } + } + + void applyLatest(modules::CompositorRenderPlan& plan, const std::string& wallId) const { + animation_.applyLatest(plan, wallId); + } + + // The take record's proof that nothing restarted (spec section 5), so + // "did this wall restart?" is a number rather than an opinion. + [[nodiscard]] uint64_t generation() const { return generation_; } + void noteReset() { ++generation_; } + + private: + compositor::TilesPlanAnimation animation_; + uint64_t generation_ = 0; +}; + +// Wall id -> source. The id is the Tiles layer id, which the shell already +// emits as "tiles:" (TilesLayerPayloadBuilder.cs), so it is unique per +// scene and identical for the same gallery on either bus. +class TilesWallSources final { + public: + [[nodiscard]] TilesWallSource& forWall(const std::string& wallId) { + return sources_[wallId]; + } + + // Lifetime is "referenced by a scene" (parent spec section 2). Anything no + // live scene names is released; a recreated wall is a NEW wall and starts at + // generation 0, never continuing a retired one's count. + void releaseAllExcept(const std::vector& liveWallIds) { + for (auto it = sources_.begin(); it != sources_.end();) { + bool live = false; + for (const auto& id : liveWallIds) { + if (it->first == id) { live = true; break; } + } + it = live ? std::next(it) : sources_.erase(it); + } + } + + // Const lookup for readers (the take record). An unknown wall has never + // animated, so its caller reports generation 0 - which is true, not a guess. + [[nodiscard]] const TilesWallSource* find(const std::string& wallId) const { + const auto it = sources_.find(wallId); + return it == sources_.end() ? nullptr : &it->second; + } + + [[nodiscard]] std::size_t size() const { return sources_.size(); } + + private: + std::map sources_; +}; + +} // namespace corevideo::core diff --git a/native/tests/TilesWallSourceTest.cpp b/native/tests/TilesWallSourceTest.cpp new file mode 100644 index 00000000..08519fe8 --- /dev/null +++ b/native/tests/TilesWallSourceTest.cpp @@ -0,0 +1,58 @@ +#include "core/TilesWallSource.h" + +#include + +namespace { +using corevideo::core::TilesWallSources; + +// One wall id yields ONE source however many buses ask for it. This is the +// whole point of the slice: the animation belongs to the wall, not the bus. +TEST(TilesWallSources, BothBusesAskingForOneWallGetTheSameSource) { + TilesWallSources sources; + auto& fromProgram = sources.forWall("tiles:scene-a"); + auto& fromPreview = sources.forWall("tiles:scene-a"); + EXPECT_EQ(&fromProgram, &fromPreview); + EXPECT_EQ(sources.size(), 1U); +} + +TEST(TilesWallSources, DifferentWallsAreDifferentSources) { + TilesWallSources sources; + auto& a = sources.forWall("tiles:scene-a"); + auto& b = sources.forWall("tiles:scene-b"); + EXPECT_NE(&a, &b); + EXPECT_EQ(sources.size(), 2U); +} + +// Lifetime is "referenced by a scene", per the parent spec. A wall no scene +// references is released; a wall still referenced survives the sweep. +TEST(TilesWallSources, AWallNoSceneReferencesIsReleased) { + TilesWallSources sources; + sources.forWall("tiles:scene-a"); + sources.forWall("tiles:scene-b"); + + sources.releaseAllExcept({"tiles:scene-b"}); + + EXPECT_EQ(sources.size(), 1U); + EXPECT_EQ(&sources.forWall("tiles:scene-b"), &sources.forWall("tiles:scene-b")); +} + +// A generation that never moves proves nothing. It must move on a reset... +TEST(TilesWallSources, AResetBumpsTheGeneration) { + TilesWallSources sources; + auto& wall = sources.forWall("tiles:scene-a"); + const auto before = wall.generation(); + wall.noteReset(); + EXPECT_GT(wall.generation(), before); +} + +// ...and a released-then-recreated wall is a NEW wall, so its generation +// must not silently continue the old one's. +TEST(TilesWallSources, ARecreatedWallDoesNotInheritTheOldGeneration) { + TilesWallSources sources; + sources.forWall("tiles:scene-a").noteReset(); + const auto retired = sources.forWall("tiles:scene-a").generation(); + sources.releaseAllExcept({}); + EXPECT_EQ(sources.forWall("tiles:scene-a").generation(), 0U); + EXPECT_NE(sources.forWall("tiles:scene-a").generation(), retired); +} +} // namespace From aa35f4ff0961a3ca9ad7ca77169021d6dbf2bf16 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 19:28:09 -0400 Subject: [PATCH 09/20] fix(tiles): cover TilesWallSource::advance's reset-detection glue Review round 1 finding 1: the original 5 tests never called TilesWallSource::advance - they only exercised noteReset()/generation bookkeeping directly, leaving the actual glue (bump the generation iff the wrapped TilesPlanAnimation::advance reports a reset) completely untested. Add two tests driving advance() directly: a different wall key arriving moves the generation by exactly one (the reset path), and the same wall key arriving twice does not move it. Verified both are load-bearing by temporarily breaking the glue two ways (unconditional noteReset(), and an inverted condition) and confirming each break turns at least one test red before restoring the fix. Finding 2: add the missing include for std::next, previously compiling only because MSVC's transitively pulls it in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/core/TilesWallSource.h | 1 + native/tests/TilesWallSourceTest.cpp | 37 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/native/src/core/TilesWallSource.h b/native/src/core/TilesWallSource.h index 2bb32851..7fd84751 100644 --- a/native/src/core/TilesWallSource.h +++ b/native/src/core/TilesWallSource.h @@ -3,6 +3,7 @@ #include "compositor/TilesPlanAnimation.h" #include +#include #include #include #include diff --git a/native/tests/TilesWallSourceTest.cpp b/native/tests/TilesWallSourceTest.cpp index 08519fe8..2f8f4d8f 100644 --- a/native/tests/TilesWallSourceTest.cpp +++ b/native/tests/TilesWallSourceTest.cpp @@ -3,6 +3,7 @@ #include namespace { +using corevideo::core::TilesWallSource; using corevideo::core::TilesWallSources; // One wall id yields ONE source however many buses ask for it. This is the @@ -55,4 +56,40 @@ TEST(TilesWallSources, ARecreatedWallDoesNotInheritTheOldGeneration) { EXPECT_EQ(sources.forWall("tiles:scene-a").generation(), 0U); EXPECT_NE(sources.forWall("tiles:scene-a").generation(), retired); } + +// Review round 1, finding 1: the five tests above never call +// TilesWallSource::advance(...) - they only prove ++generation_ works, not the +// glue that decides WHEN to call it: +// if (animation_.advance(...)) { noteReset(); } +// This pins that glue in BOTH directions, driving the real advance() the way +// MediaCore will (one animation object per wall, sampled across two "bus" +// calls with different wall keys - the whole point of the slice). +// +// A different wall key arriving is TilesPlanAnimation::advance's reset path +// (`if (key_ != wallKey) { animator_.reset(); ... }`), and the FIRST call on a +// fresh source is also a "key changed" transition (from the empty initial +// key), so it too counts as a reset - a cold start is not continuity. +TEST(TilesWallSource, AdvanceWithADifferentWallKeyMovesTheGenerationByExactlyOne) { + TilesWallSource wall; + corevideo::modules::CompositorRenderPlan plan; + wall.advance(plan, "tiles:scene-a", /*present=*/true, /*enabled=*/true, 350, 0); + const auto before = wall.generation(); + + corevideo::modules::CompositorRenderPlan otherPlan; + wall.advance(otherPlan, "tiles:scene-b", /*present=*/true, /*enabled=*/true, 350, 16); + + EXPECT_EQ(wall.generation(), before + 1); +} + +TEST(TilesWallSource, AdvanceWithTheSameWallKeyDoesNotMoveTheGeneration) { + TilesWallSource wall; + corevideo::modules::CompositorRenderPlan plan; + wall.advance(plan, "tiles:scene-a", /*present=*/true, /*enabled=*/true, 350, 0); + const auto before = wall.generation(); + + corevideo::modules::CompositorRenderPlan samePlan; + wall.advance(samePlan, "tiles:scene-a", /*present=*/true, /*enabled=*/true, 350, 16); + + EXPECT_EQ(wall.generation(), before); +} } // namespace From 9d63c8fbd7d6065b3cd491aff90420f04ad1c741 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:10:51 -0400 Subject: [PATCH 10/20] fix(tiles): one animator per wall, so a wall taken mid-animation is continuous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MediaCore held programTilesAnimation_ and previewTilesAnimation_, and handed settled state from one to the other on a Take via TilesPlanAnimation::adoptSettledFrom. That hand-off refused a wall whose tiles were still flying (mid-flight state had no correct owner with two animators), so a wall taken mid-animation re-animated its entrance from alpha 0 on Program. MediaCore now keeps ONE core::TilesWallSource per wall id (tilesWallSources_, from Task 1/2), shared by whichever bus is looking at it, so there is nothing to hand over. adoptSettledFrom is deleted. TilesPlanAnimation::advance already reports whether it reset (bool return); TilesWallSource turns that into a generation bump. TakeRecordPolicy's wallAdoptedSettled is renamed wallContinuous: it no longer means "state was handed over" but "the wall's generation did not move across this take", computed in MediaCore by comparing tilesWallGeneration() before and after the take-tick's advance. The verdict string "adopted-settled" becomes "continuous". Rewrote the three TilesAnimatorTest.cpp hand-off tests that pinned the old per-bus behaviour: a wall settled on one bus is now trivially settled on the other (same object); the refused-hand-off case becomes "a wall never cued starts cold, at generation 0, in its own object, contaminated by nothing else"; the all-stale-beat guard test is kept as-is (it already tested the shared animator directly, and matters more now that a wipe hits every bus at once). Added the headline regression test (TilesRenderPlanTest.cpp, AWallTakenMidAnimationIsContinuous). A wall's own first tick with content pops in fully opaque by design (TilesAnimator treats a fresh animator's first non-empty sample as pre-existing, not entering — see DifferentWallCannotReuseAnotherWallsCachedGeometry), so the test instead joins a member onto an ALREADY-ESTABLISHED wall and takes it before the join settles. Verified red-before-green by temporarily restoring the base (aa35f4ff) per-bus mechanism: the same scenario failed with the new tile snapping straight to full opacity instead of continuing its entrance, confirming the test exercises the real defect. Closes #448 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/compositor/TilesPlanAnimation.h | 33 ------- native/src/core/MediaCore.cpp | 76 ++++++++++------ native/src/core/MediaCore.h | 14 ++- native/src/core/TakeRecordPolicy.h | 21 +++-- native/tests/RenderedSceneAttributionTest.cpp | 8 +- native/tests/TilesAnimatorTest.cpp | 91 ++++++++++++------- native/tests/TilesRenderPlanTest.cpp | 78 ++++++++++++++++ 7 files changed, 215 insertions(+), 106 deletions(-) diff --git a/native/src/compositor/TilesPlanAnimation.h b/native/src/compositor/TilesPlanAnimation.h index e1272bd5..29ffe229 100644 --- a/native/src/compositor/TilesPlanAnimation.h +++ b/native/src/compositor/TilesPlanAnimation.h @@ -9,39 +9,6 @@ class TilesPlanAnimation { public: void reset() { animator_.reset(); key_.clear(); sampled_.clear(); } - // Carry a SETTLED wall from one bus to the other (live-show defect, owner - // report 2026-09-09: "I can't have a total rerender from what is in preview - // to program like it is loading for the first time"). - // - // The wall key is sceneId + ":" + layerId and the layer id is derived from - // the scene id, so the gallery sitting settled in PREVIEW and the same - // gallery a Take puts on PROGRAM carry the IDENTICAL key — it is one wall - // continuing on another bus, not a new one. Without this, the program - // animation saw a key it had never held, reset its animator, and threw away - // spring positions and entry alpha that were fully settled an instant - // earlier on the other bus. - // - // Scoped so the two buses can never contaminate each other: - // * only on an EXACT key match (a different wall, or a wall the other bus - // never held, is refused and animates exactly as it does today), - // * only when the other bus's wall is SETTLED (every sampled tile atRest — - // mid-flight state belongs to the bus that is flying it), - // * the state is MOVED, and the source is reset — never aliased, so the - // next wall cued on the source bus starts clean. - // Returns true if the state was carried across. - bool adoptSettledFrom(TilesPlanAnimation& previous, const std::string& wallKey) { - if (wallKey.empty() || key_ == wallKey) return false; - if (previous.key_ != wallKey || previous.sampled_.empty()) return false; - for (const auto& tile : previous.sampled_) { - if (!tile.atRest) return false; - } - animator_ = std::move(previous.animator_); - sampled_ = std::move(previous.sampled_); - key_ = wallKey; - previous.reset(); - return true; - } - // Returns true when this call RESET the animator (a departure/disable, or a // different wall key arriving) - the caller's only truthful signal of "did // this wall restart", with no std::function/allocation on the render tick. diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 00690284..087a2f97 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -2049,8 +2049,13 @@ void MediaCore::armTakeRecord(const std::string& toSceneId) { pendingTakeRecord_ = std::move(record); } +uint64_t MediaCore::tilesWallGeneration(const std::string& wallId) const { + const auto* wall = tilesWallSources_.find(wallId); + return wall ? wall->generation() : 0; +} + void MediaCore::completeTakeRecord(const modules::CompositorRenderPlan& programPlan, - bool wallAdoptedSettled, + bool wallContinuous, const std::vector& frames) { if (!pendingTakeRecord_) return; TakeRecord record = std::move(*pendingTakeRecord_); @@ -2065,7 +2070,7 @@ void MediaCore::completeTakeRecord(const modules::CompositorRenderPlan& programP const std::string liveBackgroundLayerId = "tiles-source-bg:" + tilesLayer_.layerId; record.observation.hasWallAfter = tilesLayer_.present; - record.observation.wallAdoptedSettled = wallAdoptedSettled; + record.observation.wallContinuous = wallContinuous; record.observation.liveBackgroundExpected = tilesLayer_.present && !tilesLayer_.style.backgroundSourceId.empty() && tilesLayer_.style.backgroundSourceId != tilesLayer_.layerId; @@ -3631,7 +3636,12 @@ modules::CompositorRenderPlan MediaCore::buildMultiviewRenderPlan(const std::vec renderPlan.layers.push_back(std::move(marker)); } else { auto programPlan = buildCompositorRenderPlan(videoFrames); - programTilesAnimation_.applyLatest(programPlan, sceneId_ + ":" + tilesLayer_.layerId); + // This is a read-only path (buildMultiviewRenderPlan is const): use find(), + // never forWall(), which would insert a wall on a mere read. A wall with no + // TilesWallSource yet has never animated, so there is nothing to apply. + if (const auto* wall = tilesWallSources_.find(tilesLayer_.layerId)) { + wall->applyLatest(programPlan, tilesLayer_.layerId); + } renderPlan.layers.reserve(programPlan.layers.size() + static_cast(sourceCount) + 1); std::stable_sort(programPlan.layers.begin(), programPlan.layers.end(), [](const auto& a, const auto& b) { return a.order < b.order; }); @@ -3667,7 +3677,9 @@ modules::CompositorRenderPlan MediaCore::buildMultiviewRenderPlan(const std::vec // never reflected the preview and never swapped on Take.) if (hasPreviewScene()) { auto previewPlan = buildPreviewCompositorRenderPlan(videoFrames); - previewTilesAnimation_.applyLatest(previewPlan, previewSceneId_ + ":" + previewTilesLayer_.layerId); + if (const auto* wall = tilesWallSources_.find(previewTilesLayer_.layerId)) { + wall->applyLatest(previewPlan, previewTilesLayer_.layerId); + } std::stable_sort(previewPlan.layers.begin(), previewPlan.layers.end(), [](const auto& a, const auto& b) { return a.order < b.order; }); for (const auto& src : previewPlan.layers) { @@ -6252,30 +6264,42 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // THE TAKE HAND-OFF (owner report 2026-09-09: a gallery taken from preview to // program must be a CUT to something already rendered, never a redraw). // TransportCoordinator.TakeAsync swaps ActiveSceneId/PreviewSceneId and sends - // ONE sync, so the program wall key becomes the key preview held on the - // previous tick — the same wall, continuing on the other bus. Carry its - // settled animation state over before advancing, instead of letting the - // key change reset the animator. Refused unless the keys match EXACTLY and - // the preview wall is settled, and the state is MOVED (preview is reset), so - // the two buses cannot contaminate each other. Cost is a key compare plus a - // move of <=64 tiles, only on the tick a wall changes bus — no added - // coreMutex hold. - const std::string programWallKey = sceneId_ + ":" + tilesLayer_.layerId; - bool wallAdoptedSettled = false; + // ONE sync, so the program wall id becomes the id preview held on the + // previous tick — the SAME wall (#448: one animator per wall, not per bus), + // so there is nothing to hand over: both buses sample the same object, and a + // cut changes only which bus is looking at it. + const std::string programWallId = tilesLayer_.layerId; + const std::string previewWallId = previewTilesLayer_.layerId; + // The take record's proof that this wall did not restart across the take: + // its generation before this tick's advance, compared after (equal == + // continuous). Read via the const find() (never forWall(), which would + // insert on a mere read) — an unknown wall has never animated and reports + // 0, which is true, not a guess. Scoped to the same present+animated gate + // as the advance call below: a wall that is not animating has no animator + // state to be continuous ABOUT, so it stays "not proven continuous", same + // as the old adoptSettledFrom-gated default. + bool wallContinuous = false; if (tilesLayer_.present && tilesLayer_.style.animateLayout) { - wallAdoptedSettled = programTilesAnimation_.adoptSettledFrom(previewTilesAnimation_, programWallKey); - } - programTilesAnimation_.advance(renderPlan, programWallKey, - tilesLayer_.present, tilesLayer_.style.animateLayout, tilesLayer_.style.animationDurationMs, animationNowMs); - // Advance Preview on this same render clock even when its wall is empty. - // Snapshot/prefetch builds must not change entry/departure animation state. - if (previewTilesLayer_.present && previewTilesLayer_.style.animateLayout && hasPreviewScene()) { + const uint64_t generationBefore = tilesWallGeneration(programWallId); + tilesWallSources_.forWall(programWallId).advance(renderPlan, programWallId, + tilesLayer_.present, tilesLayer_.style.animateLayout, tilesLayer_.style.animationDurationMs, animationNowMs); + wallContinuous = tilesWallGeneration(programWallId) == generationBefore; + } + // Advance Preview on the SAME render clock even when its wall is empty, so + // snapshot/prefetch builds cannot change entry/departure animation state. + // When both buses show the SAME wall this is the same object, advanced once + // above — guard against double-advancing it on one tick. + if (previewTilesLayer_.present && previewTilesLayer_.style.animateLayout && + hasPreviewScene() && previewWallId != programWallId) { auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); - previewTilesAnimation_.advance(previewAnimationPlan, previewSceneId_ + ":" + previewTilesLayer_.layerId, + tilesWallSources_.forWall(previewWallId).advance(previewAnimationPlan, previewWallId, true, previewTilesLayer_.style.animateLayout, previewTilesLayer_.style.animationDurationMs, animationNowMs); - } else { - previewTilesAnimation_.reset(); } + // Lifetime: release any wall no live scene still names (parent spec section 2). + std::vector liveWallIds; + if (tilesLayer_.present) liveWallIds.push_back(programWallId); + if (previewTilesLayer_.present) liveWallIds.push_back(previewWallId); + tilesWallSources_.releaseAllExcept(liveWallIds); // Task 4: cache the plan the render tick actually built — lastRenderPlanForTest() // and the sessionState() `tiles` node both read THIS, so a consumer can never // observe a wall the compositor did not also receive (see modules_.compositor-> @@ -6374,7 +6398,7 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // is built, and judging continuity or "had a frame" before they arrive would // call every media source missing and read the previous tick's generations. if (pendingTakeRecord_) { - completeTakeRecord(renderPlan, wallAdoptedSettled, videoFrames); + completeTakeRecord(renderPlan, wallContinuous, videoFrames); } markStage(s_stagePlanUs, 1); auto producedFrame = modules_.compositor->render(renderPlan, videoFrames); @@ -6555,7 +6579,7 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim if (previewActive && previewDue) { const auto previewStartTp = std::chrono::steady_clock::now(); auto previewPlan = buildPreviewCompositorRenderPlan(videoFrames); - previewTilesAnimation_.applyLatest(previewPlan, previewSceneId_ + ":" + previewTilesLayer_.layerId); + tilesWallSources_.forWall(previewTilesLayer_.layerId).applyLatest(previewPlan, previewTilesLayer_.layerId); previewPlan.skipCpuReadback = true; lastProgramFrame_.previewSharedTexture = modules_.compositor->renderPreview(previewPlan, videoFrames); lastProgramFrame_.previewWidth = previewPlan.width; diff --git a/native/src/core/MediaCore.h b/native/src/core/MediaCore.h index 7496f1cd..788fe1b0 100644 --- a/native/src/core/MediaCore.h +++ b/native/src/core/MediaCore.h @@ -2,6 +2,7 @@ #include "compositor/TilesMembership.h" #include "compositor/TilesPlanAnimation.h" +#include "core/TilesWallSource.h" #include "core/Director.h" #include "core/MonitorShedPolicy.h" #include "core/OutputLifecyclePolicy.h" @@ -266,6 +267,12 @@ class MediaCore { // declaration for why sharing one field was a live-show bug, not just a // test-seam gap. const TilesLayerState& previewTilesLayerForTest() const { return previewTilesLayer_; } + // The take record's proof that a wall did not restart across a Take: read + // via the const find() (never forWall(), which would insert a wall on a + // mere read). An unknown wall reports generation 0 — true, not a guess. + // Also the test seam: the headline #448 regression test reads this directly + // to assert a mid-animation Take does not bump the wall's generation. + [[nodiscard]] uint64_t tilesWallGeneration(const std::string& wallId) const; const std::vector& sceneValidationWarningsForTest() const { return sceneValidationWarnings_; } @@ -519,8 +526,9 @@ class MediaCore { // applyPreviewScene. See tilesLayer_ above for why this must be a SEPARATE // field rather than shared. TilesLayerState previewTilesLayer_; - compositor::TilesPlanAnimation programTilesAnimation_; - compositor::TilesPlanAnimation previewTilesAnimation_; + // One animation per WALL, not per bus (#448). See core/TilesWallSource.h for + // why the per-bus pair and its hand-off were wrong. + core::TilesWallSources tilesWallSources_; // Task 4: per-member frame-age snapshot for the wall expansion, refreshed // every render tick from the live videoFrames gather (renderSyntheticTick, // under coreMutex — geometry bookkeeping, not pixel work). Covers members of @@ -752,7 +760,7 @@ class MediaCore { // `frames` is THIS tick's final gather (media frames included): a source the // take brought on air with no frame in it counts as missing. void completeTakeRecord(const modules::CompositorRenderPlan& programPlan, - bool wallAdoptedSettled, + bool wallContinuous, const std::vector& frames); static std::vector renderPlanSourceIds(const modules::CompositorRenderPlan& plan); // Lock-free mirror of lastProgramFrame_.frameNumber for the audio worker's diff --git a/native/src/core/TakeRecordPolicy.h b/native/src/core/TakeRecordPolicy.h index 4545cbac..405cb00c 100644 --- a/native/src/core/TakeRecordPolicy.h +++ b/native/src/core/TakeRecordPolicy.h @@ -9,11 +9,14 @@ namespace corevideo::core { // DID THE WALL CUT, OR DID IT REBUILD? // // A Take of a Tiles scene is supposed to be a CUT to something already -// composited on Preview: `TilesPlanAnimation::adoptSettledFrom` moves the -// settled animator across buses so the wall continues instead of replaying its -// entrance. When adoption is refused (keys differ, or the preview wall was not -// at rest) the program animator resets and every tile animates in from scratch -// — on air that reads as the wall redrawing itself. +// composited on Preview. Since #448, a wall owns exactly ONE animator +// (`core::TilesWallSource`, shared by both buses) instead of a per-bus pair +// with a hand-off between them — so there is nothing to adopt or refuse. +// `wallContinuous` is that animator's generation before this take's advance +// compared to after: equal means the wall kept animating in place; a bump +// means something reset it (a different wall arrived, or it went away and +// came back), and every tile would have animated in from scratch — on air +// that reads as the wall redrawing itself. // // Two other things can produce the identical picture and must not be confused // with it, which is why they are inputs here rather than a second guess later: @@ -42,7 +45,7 @@ struct TakeRecordPolicy { struct Observation { bool hasWallAfter = false; // the taken scene carries a Tiles wall - bool wallAdoptedSettled = false; // adoptSettledFrom() returned true + bool wallContinuous = false; // the wall's generation did not move across the take bool liveBackgroundExpected = false; // the wall declares a live source background bool liveBackgroundEmitted = false; // ...and it was in the first program frame std::uint64_t subscriptionChurnDelta = 0; // real re-subscribes across the take @@ -56,7 +59,7 @@ struct TakeRecordPolicy { struct Verdict { // How the wall arrived on Program. - const char* wall = "none"; // none | adopted-settled | reset + const char* wall = "none"; // none | continuous | reset // The one-word answer to "did the take rebuild or cut". const char* verdict = "no-wall"; // cut | rebuilt | no-wall // Whether anything other than the render plan could explain a rebuild. @@ -92,12 +95,12 @@ struct TakeRecordPolicy { else verdict.verdict = observation.sharedSources.empty() ? "no-wall" : "cut"; return verdict; } - if (!observation.wallAdoptedSettled) { + if (!observation.wallContinuous) { verdict.wall = "reset"; verdict.verdict = "rebuilt"; return verdict; } - verdict.wall = "adopted-settled"; + verdict.wall = "continuous"; // The animator cut cleanly. If the background never made the first frame, // a subscription was torn down in the same tick, or a source restarted or // cold-started, the operator can still have seen a rebuild — say so diff --git a/native/tests/RenderedSceneAttributionTest.cpp b/native/tests/RenderedSceneAttributionTest.cpp index c30e7d6b..4a19f624 100644 --- a/native/tests/RenderedSceneAttributionTest.cpp +++ b/native/tests/RenderedSceneAttributionTest.cpp @@ -239,12 +239,12 @@ TEST(TakeRecordPolicyRules, TheWallVerdictSeparatesACutFromARebuild) { EXPECT_EQ(std::string(TakeRecordPolicy::evaluate(observation).verdict), "no-wall"); observation.hasWallAfter = true; - observation.wallAdoptedSettled = false; + observation.wallContinuous = false; EXPECT_EQ(std::string(TakeRecordPolicy::evaluate(observation).wall), "reset"); EXPECT_EQ(std::string(TakeRecordPolicy::evaluate(observation).verdict), "rebuilt"); - observation.wallAdoptedSettled = true; - EXPECT_EQ(std::string(TakeRecordPolicy::evaluate(observation).wall), "adopted-settled"); + observation.wallContinuous = true; + EXPECT_EQ(std::string(TakeRecordPolicy::evaluate(observation).wall), "continuous"); EXPECT_EQ(std::string(TakeRecordPolicy::evaluate(observation).verdict), "cut"); // Adopted, but the wall's live background never made the first program frame. @@ -283,7 +283,7 @@ TEST(TakeRecordPolicyRules, ASharedSourceThatRestartedDeniesTheCut) { TEST(TakeRecordPolicyRules, SharedSourcesThatKeptTheirGenerationAllowACut) { TakeRecordPolicy::Observation observation; observation.hasWallAfter = true; - observation.wallAdoptedSettled = true; + observation.wallContinuous = true; observation.sharedSources.push_back({"background:bg", 1, 1, 40, 45}); const auto verdict = TakeRecordPolicy::evaluate(observation); EXPECT_FALSE(verdict.sharedSourceRestarted); diff --git a/native/tests/TilesAnimatorTest.cpp b/native/tests/TilesAnimatorTest.cpp index fd6092e8..e0453171 100644 --- a/native/tests/TilesAnimatorTest.cpp +++ b/native/tests/TilesAnimatorTest.cpp @@ -1,5 +1,6 @@ #include "compositor/TilesAnimator.h" #include "compositor/TilesPlanAnimation.h" +#include "core/TilesWallSource.h" #include using namespace corevideo::compositor; @@ -108,10 +109,10 @@ TEST(TilesAnimator, DifferentWallCannotReuseAnotherWallsCachedGeometry) { EXPECT_EQ(plan.layers[0].rect.x, .5f); EXPECT_EQ(plan.layers[0].opacity, 1.f); } -// The take hand-off (owner report 2026-09-09). A wall that changes BUS is the -// same wall: preview's settled state moves to program instead of being thrown -// away and re-adopted. Scoped by an exact key match + settledness, and MOVED so -// the two buses can never alias each other. +// The take hand-off (owner report 2026-09-09, #448). A wall that changes BUS +// is the SAME wall — and since #448 it is, literally: `core::TilesWallSource` +// gives a wall ONE animator shared by both buses, so a Take need not move or +// adopt anything. There is no per-bus pair left to hand off between. corevideo::modules::CompositorRenderPlan wallPlan(std::initializer_list tiles) { corevideo::modules::CompositorRenderPlan plan; for (const auto& tile : tiles) { @@ -123,43 +124,71 @@ corevideo::modules::CompositorRenderPlan wallPlan(std::initializer_list initialMembers{"capture:g1", "capture:g2"}; + (void)core.applyCommands(corevideo::rpc::Json::Array{ + wallLessScene("load-scene-graph", "solo"), + wallScene("set-preview-scene", "gallery", initialMembers)}); + ASSERT_TRUE(allSettled(tileLayers(compositor->lastPreviewPlan))) + << "precondition: the wall must be established before a member joins it"; + + // A THIRD member joins the SAME wall (same layerId, same scene) while it is + // already on air in preview. + (void)core.applyCommands(corevideo::rpc::Json::Array{ + wallScene("set-preview-scene", "gallery", wallMembers())}); + + const auto midFlight = tileLayers(compositor->lastPreviewPlan); + ASSERT_EQ(midFlight.size(), wallMembers().size()); + ASSERT_FALSE(allSettled(midFlight)) + << "precondition: the wall must still be animating for this test to mean anything"; + const auto midFlightSnapshot = snapshotTiles(compositor->lastPreviewPlan); + + const auto generationBefore = core.tilesWallGeneration("tiles:gallery"); + + take(core, wallScene("load-scene-graph", "gallery", wallMembers()), + wallLessScene("set-preview-scene", "solo")); + + // The wall did not restart... + EXPECT_EQ(core.tilesWallGeneration("tiles:gallery"), generationBefore) + << "the wall's generation moved across the take — it restarted instead of continuing"; + + // ...and its tiles continued from where they were, rather than snapping to + // a new state. Alpha is the sharpest signal, but ">= before" alone is too + // weak: an animator that RESETS (a fresh TilesAnimator's first non-empty + // sample call is itself treated as "already there" — see + // AWallThatWasNeverCuedStartsCold above) pops a still-entering tile straight + // to fully opaque, which technically satisfies ">=" too. The take happens on + // the very next tick with negligible additional wall-clock time, so a + // CONTINUING animation cannot have progressed far past where it was; a value + // that jumped essentially to 1.0 is a reset wearing an alpha that happens to + // be no smaller, not a continuation. + const auto onAir = tileLayers(compositor->lastPlan); + ASSERT_EQ(onAir.size(), wallMembers().size()) << "the taken wall lost tiles on its first program frame"; + for (const auto* tile : onAir) { + ASSERT_EQ(midFlightSnapshot.count(tile->layerId), 1u); + const auto& before = midFlightSnapshot.at(tile->layerId); + EXPECT_GE(tile->opacity, before.opacity) + << tile->layerId << " opacity regressed across the take"; + if (before.opacity < 1.f) { + EXPECT_LT(tile->opacity, 0.9f) + << tile->layerId << " snapped straight to fully opaque instead of continuing its entrance " + "— the wall was reset (popped in), not continued"; + } + } +} + // THE PROPERTY: a wall settled in preview, then taken, is at its settled state // on the first program frame — a cut, not a redraw. TEST(TilesRenderPlan, AWallSettledInPreviewIsAlreadySettledOnItsFirstProgramFrame) { From c11862d2f6cf5339821c536fdd304a8dd1944249 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:53:27 -0400 Subject: [PATCH 11/20] =?UTF-8?q?fix(tiles):=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20idempotent=20reset,=20read-path=20phantom-wall,=20f?= =?UTF-8?q?reeze=20fix,=20verdict/rect=20assertions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes five review findings on the #448 one-animator-per-wall change: - Finding 0: TilesPlanAnimation::advance's not-present/disabled early return now reports whether it actually reset (key_ was non-empty), not "true" unconditionally — a repeated disabled tick is a harmless no-op, not a generation tick-counter. - Finding A (Critical): the preview GPU-composite applyLatest call used forWall(), inserting a phantom ""-keyed wall for any preview scene with no wall, under coreMutex, every render tick. Switched to find() + null-check, matching the two multiview sites. - Finding B: the double-advance guard keyed on wall-id equality alone, so a wall with animateLayout=false on Program and true on Preview at the SAME id was advanced by neither branch (frozen). Hoisted `programAdvanced = present && animateLayout` and gated preview's advance on `!programAdvanced || previewWallId != programWallId`. - Finding C: advance() was only called when animateLayout was true, so a disabled wall's sampled_ was retained forever and kept feeding stale geometry to the multiview PGM cell and the preview composite. Restored the unconditional call whenever the wall is present, letting `enabled` decide reset-vs-sample inside advance() (safe now under Finding 0's guard). - Finding D: added an end-to-end assertion in the headline test that the take record itself reads wall=="continuous", verdict=="cut" — not just the raw generation number. - Finding E: added rect-continuity assertions for tiles whose opacity was already 1 at the mid-flight tick (only the newly-joined tile had a ramping alpha; the pre-existing two tiles' RECTS were mid-spring from the layout change, unasserted before this). Self-caught while implementing Finding C: widening the advance gate to "present" alone broke ATakeIsRecordedWithBothSidesOfTheWall — a wall id seen for the first time starts at generation 0, and with Finding 0's idempotence a disabled first call also reports "did not reset", so raw generation equality misread a brand-new non-animated wall as continuous. Fixed by requiring wall EXISTENCE (find() before forWall()), not just generation equality. Self-caught while implementing Finding E: the rect-continuity check first failed against the CORRECT implementation too, because the shared wall's 100ms clamped-floor duration converges audibly close to target within a few ms of real test-process overhead. Added wallSceneWithDuration() and drove the test at the animator's 2000ms clamp ceiling for a reliable margin. Re-verified red against the reverted base mechanism with both new teeth (Finding D's take-record assertion and Finding E's rect check both fail); re-verified green on the fixed tree, 968/968, three consecutive runs with no flakiness. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/compositor/TilesPlanAnimation.h | 6 +- native/src/core/MediaCore.cpp | 54 ++++++++++++++---- native/tests/TilesRenderPlanTest.cpp | 65 +++++++++++++++++++++- 3 files changed, 110 insertions(+), 15 deletions(-) diff --git a/native/src/compositor/TilesPlanAnimation.h b/native/src/compositor/TilesPlanAnimation.h index 29ffe229..abf4c5f8 100644 --- a/native/src/compositor/TilesPlanAnimation.h +++ b/native/src/compositor/TilesPlanAnimation.h @@ -14,7 +14,11 @@ class TilesPlanAnimation { // this wall restart", with no std::function/allocation on the render tick. bool advance(modules::CompositorRenderPlan& plan, const std::string& wallKey, bool present, bool enabled, double durationMs, double nowMs) { - if (!present || !enabled) { reset(); return true; } + // Idempotent: an ALREADY-reset wall (key_ empty) reports it did NOT reset + // on a repeated not-present/disabled tick — without this a caller that + // advances every tick regardless of presence would read "reset" forever, + // turning the generation into a tick counter instead of a restart signal. + if (!present || !enabled) { const bool had = !key_.empty(); reset(); return had; } // A DIFFERENT wall never inherits this one's geometry. (sampled_ is cleared // too: the all-stale guard below would otherwise let a new wall's first // frames be drawn at the previous wall's tile rects.) diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 087a2f97..6d50a91d 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -6270,27 +6270,54 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // cut changes only which bus is looking at it. const std::string programWallId = tilesLayer_.layerId; const std::string previewWallId = previewTilesLayer_.layerId; + // Review fix round 1, Finding C: advance() runs UNCONDITIONALLY whenever the + // wall is present — never additionally gated on animateLayout — and lets + // `enabled` (=animateLayout) decide reset-vs-sample INSIDE advance() (Finding + // 0's idempotent guard makes a repeated disabled tick a harmless no-op, not a + // tick-counter). Gating the call itself on animateLayout, as the first cut of + // this fix did, left a disabled wall's `sampled_` retained forever: the + // multiview PGM cell and the preview composite (both read via applyLatest, + // which does not know about "enabled") kept applying stale animated geometry + // that the Program plan itself no longer carried. + // // The take record's proof that this wall did not restart across the take: // its generation before this tick's advance, compared after (equal == // continuous). Read via the const find() (never forWall(), which would // insert on a mere read) — an unknown wall has never animated and reports - // 0, which is true, not a guess. Scoped to the same present+animated gate - // as the advance call below: a wall that is not animating has no animator - // state to be continuous ABOUT, so it stays "not proven continuous", same - // as the old adoptSettledFrom-gated default. + // 0, which is true, not a guess. + const bool programAdvanced = tilesLayer_.present && tilesLayer_.style.animateLayout; bool wallContinuous = false; - if (tilesLayer_.present && tilesLayer_.style.animateLayout) { - const uint64_t generationBefore = tilesWallGeneration(programWallId); + if (tilesLayer_.present) { + // "Continuous" requires the wall to have EXISTED before this tick, not + // just for its generation to be unchanged — a wall id that has NEVER been + // seen (a genuinely different wall, e.g. a plain scene-to-scene cut with + // no animation) starts at generation 0 and, with Finding 0's idempotent + // guard, a disabled first call also reports "did not reset" (nothing to + // reset) — so generation stays 0 on BOTH sides of a brand-new wall's + // first tick. Raw generation equality alone would misread that as + // continuous. find() first (never forWall() for this read — a lookup + // must not decide existence by the side effect of creating it). + const auto* existingWall = tilesWallSources_.find(programWallId); + const bool wallExistedBefore = existingWall != nullptr; + const uint64_t generationBefore = existingWall ? existingWall->generation() : 0; tilesWallSources_.forWall(programWallId).advance(renderPlan, programWallId, tilesLayer_.present, tilesLayer_.style.animateLayout, tilesLayer_.style.animationDurationMs, animationNowMs); - wallContinuous = tilesWallGeneration(programWallId) == generationBefore; + wallContinuous = wallExistedBefore && tilesWallGeneration(programWallId) == generationBefore; } // Advance Preview on the SAME render clock even when its wall is empty, so // snapshot/prefetch builds cannot change entry/departure animation state. - // When both buses show the SAME wall this is the same object, advanced once - // above — guard against double-advancing it on one tick. + // + // Review fix round 1, Finding B: gate on whether PROGRAM actually advanced + // this tick (`programAdvanced`, present && animateLayout — the shape of the + // call that can move this object), not on id equality alone. The two layer + // states are parsed independently (load-scene-graph vs set-preview-scene, + // and the preview draft is separately editable), so their `animateLayout` + // flags can disagree even when they name the SAME wall id. Gating only on + // `previewWallId != programWallId` meant a wall with `animateLayout=false` + // on Program and `true` on Preview, at the same id, got ADVANCED BY NEITHER + // branch — frozen, with applyLatest re-applying stale rects forever. if (previewTilesLayer_.present && previewTilesLayer_.style.animateLayout && - hasPreviewScene() && previewWallId != programWallId) { + hasPreviewScene() && (!programAdvanced || previewWallId != programWallId)) { auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); tilesWallSources_.forWall(previewWallId).advance(previewAnimationPlan, previewWallId, true, previewTilesLayer_.style.animateLayout, previewTilesLayer_.style.animationDurationMs, animationNowMs); @@ -6579,7 +6606,12 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim if (previewActive && previewDue) { const auto previewStartTp = std::chrono::steady_clock::now(); auto previewPlan = buildPreviewCompositorRenderPlan(videoFrames); - tilesWallSources_.forWall(previewTilesLayer_.layerId).applyLatest(previewPlan, previewTilesLayer_.layerId); + // Read-only: use find(), never forWall() — a preview scene with no wall + // has an empty layerId, and forWall("") would insert a phantom map node + // (allocate + free) under coreMutex on every such render tick. + if (const auto* wall = tilesWallSources_.find(previewTilesLayer_.layerId)) { + wall->applyLatest(previewPlan, previewTilesLayer_.layerId); + } previewPlan.skipCpuReadback = true; lastProgramFrame_.previewSharedTexture = modules_.compositor->renderPreview(previewPlan, videoFrames); lastProgramFrame_.previewWidth = previewPlan.width; diff --git a/native/tests/TilesRenderPlanTest.cpp b/native/tests/TilesRenderPlanTest.cpp index c0189056..4729dba8 100644 --- a/native/tests/TilesRenderPlanTest.cpp +++ b/native/tests/TilesRenderPlanTest.cpp @@ -937,6 +937,30 @@ corevideo::rpc::Json wallScene(const char* type, const char* sceneId, {"tiles", animatedTilesPayload(std::string("tiles:") + sceneId, members)}}}; } +// Same shape as wallScene(), but with a caller-chosen animationDurationMs. +// AWallTakenMidAnimationIsContinuous needs the SLOWEST duration the animator +// accepts (clamped at 2000ms in TilesAnimator) so its rect-continuity check +// (Finding E, review round 1) is not at the mercy of how many real +// milliseconds of test-process overhead land between two back-to-back +// applyCommands() calls: at the shared 100ms floor, this rig's own command +// handling was enough real wall-clock time for a critically-damped 6.6/duration-Hz +// spring to converge audibly close to its target on its own, with or without a +// bug — the same closed-form "rest" snap the reset path uses. At 2000ms the +// per-millisecond convergence is 20x slower, giving a reliable margin. +corevideo::rpc::Json wallSceneWithDuration(const char* type, const char* sceneId, + const std::vector& members, double durationMs) { + auto tiles = animatedTilesPayload(std::string("tiles:") + sceneId, members); + auto object = tiles.asObject(); + auto style = object.at("style").asObject(); + style.insert_or_assign("animationDurationMs", corevideo::rpc::Json{durationMs}); + object.insert_or_assign("style", corevideo::rpc::Json{style}); + return corevideo::rpc::Json{corevideo::rpc::Json::Object{ + {"type", corevideo::rpc::Json{type}}, + {"sceneId", corevideo::rpc::Json{sceneId}}, + {"routes", corevideo::rpc::Json{corevideo::rpc::Json::Array{}}}, + {"tiles", corevideo::rpc::Json{object}}}}; +} + // The same wall, but carrying a LIVE background feed (tiles-source-bg:). corevideo::rpc::Json wallSceneWithBackground(const char* type, const char* sceneId, const std::vector& members, @@ -1046,17 +1070,18 @@ TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { RecordingCompositor* compositor = nullptr; MediaCore core(wallModules(&compositor, nullptr)); + const double kDurationMs = 2000.0; // the animator's own clamp ceiling — see wallSceneWithDuration const std::vector initialMembers{"capture:g1", "capture:g2"}; (void)core.applyCommands(corevideo::rpc::Json::Array{ wallLessScene("load-scene-graph", "solo"), - wallScene("set-preview-scene", "gallery", initialMembers)}); + wallSceneWithDuration("set-preview-scene", "gallery", initialMembers, kDurationMs)}); ASSERT_TRUE(allSettled(tileLayers(compositor->lastPreviewPlan))) << "precondition: the wall must be established before a member joins it"; // A THIRD member joins the SAME wall (same layerId, same scene) while it is // already on air in preview. (void)core.applyCommands(corevideo::rpc::Json::Array{ - wallScene("set-preview-scene", "gallery", wallMembers())}); + wallSceneWithDuration("set-preview-scene", "gallery", wallMembers(), kDurationMs)}); const auto midFlight = tileLayers(compositor->lastPreviewPlan); ASSERT_EQ(midFlight.size(), wallMembers().size()); @@ -1066,7 +1091,7 @@ TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { const auto generationBefore = core.tilesWallGeneration("tiles:gallery"); - take(core, wallScene("load-scene-graph", "gallery", wallMembers()), + take(core, wallSceneWithDuration("load-scene-graph", "gallery", wallMembers(), kDurationMs), wallLessScene("set-preview-scene", "solo")); // The wall did not restart... @@ -1094,8 +1119,42 @@ TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { EXPECT_LT(tile->opacity, 0.9f) << tile->layerId << " snapped straight to fully opaque instead of continuing its entrance " "— the wall was reset (popped in), not continued"; + } else { + // Review fix round 1, Finding E: g1/g2 are already fully opaque at the + // midFlight tick (only g3, the newly joined member, has a ramping + // alpha) — the layout change from 2-up to 3-up put THEM mid-spring on + // their RECT instead. A reset snaps a tile's position straight to its + // new target (TilesAnimator: `if (added) state.position = goal;`), so + // an on-air rect far from the mid-flight rect is a reset wearing full + // opacity, not a continuation — the take happens on the very next tick + // with negligible additional wall-clock time, so a genuinely + // CONTINUING spring cannot have travelled far from where it was. + EXPECT_NEAR(tile->rect.x, before.rect.x, 0.05f) + << tile->layerId << " rect.x snapped to a new position instead of continuing its spring"; + EXPECT_NEAR(tile->rect.y, before.rect.y, 0.05f) + << tile->layerId << " rect.y snapped to a new position instead of continuing its spring"; + EXPECT_NEAR(tile->rect.width, before.rect.width, 0.05f) + << tile->layerId << " rect.width snapped to a new position instead of continuing its spring"; + EXPECT_NEAR(tile->rect.height, before.rect.height, 0.05f) + << tile->layerId << " rect.height snapped to a new position instead of continuing its spring"; } } + + // Review fix round 1, Finding D: the verdict this task exists to prove was + // never asserted end-to-end. A take record reading "continuous"/"cut" is + // the whole point of #448 — read the record the take above just completed + // and assert it directly, not just the raw generation number. + const auto snapshot = core.sessionState(); + const auto* takes = snapshot.get("takeRecords"); + ASSERT_NE(takes, nullptr); + ASSERT_NE(takes->get("records"), nullptr); + const auto& records = takes->get("records")->asArray(); + ASSERT_FALSE(records.empty()) << "the take above produced no record"; + const auto& record = records.back(); + EXPECT_EQ(record.getString("wall"), "continuous") + << "the take record still reads the wall as having reset"; + EXPECT_EQ(record.getString("verdict"), "cut") + << "the take record still reads this as a rebuild, not a cut"; } // THE PROPERTY: a wall settled in preview, then taken, is at its settled state From f3a646ab1e5b6943495c5ec144ea5ea85780c3c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 21:09:14 -0400 Subject: [PATCH 12/20] =?UTF-8?q?fix(tiles):=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20one=20advance=20per=20wall=20per=20tick,=20not=20tw?= =?UTF-8?q?o=20with=20contradictory=20enabled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review confirmed round 1's wallExistedBefore term and the 2000ms tolerance retune were both sound. It found one new Important finding introduced by Findings B and C combining: a wall present on both buses with DISAGREEING animateLayout (Program false, Preview true, or vice versa) was advanced TWICE in one tick on the SAME shared object with contradictory `enabled` values — Program's call (enabled=false) reset it, then Preview's separate call (enabled=true) saw an empty key and reset it AGAIN. Net per tick: generation +2, no animation ever completing, the object re-adopted from scratch every tick. Not a verdict inversion (it read the conservative "rebuilt"), but incoherent under the one-animator-per-wall model this task exists to establish. Restructured the take-tick block so exactly one advance() call happens per wall per tick, true by construction: - `sameWall` decides whether Program and Preview name one object or two. - When they share an id, `enabled` is the OR of both buses' animateLayout — a wall animates if EITHER bus wants it to — and there is exactly one advance() call to carry that decision. - Preview only gets its own separate advance() call when it names a DIFFERENT wall id (a genuinely different object). Kept everything the round-1 re-review confirmed: the `had` idempotence guard in TilesPlanAnimation::advance, find() on every read path, the wallExistedBefore existence check, and the unconditional-whenever-present call shape that fixed Finding C (now applied symmetrically to both branches). Added TilesRenderPlan.ASharedWallWithDisagreeingAnimateFlagsAdvancesAtMostOncePerTick — no prior coverage exercised this configuration, which is why it took two review rounds to surface. Verified red against c11862d2 (generation climbed every tick) and green after this restructure, across three consecutive full suite runs (969/969, 0 failed). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/core/MediaCore.cpp | 78 +++++++++++++++------------- native/tests/TilesRenderPlanTest.cpp | 43 +++++++++++++++ 2 files changed, 84 insertions(+), 37 deletions(-) diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 6d50a91d..2998d4d7 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -6270,57 +6270,61 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // cut changes only which bus is looking at it. const std::string programWallId = tilesLayer_.layerId; const std::string previewWallId = previewTilesLayer_.layerId; - // Review fix round 1, Finding C: advance() runs UNCONDITIONALLY whenever the - // wall is present — never additionally gated on animateLayout — and lets - // `enabled` (=animateLayout) decide reset-vs-sample INSIDE advance() (Finding - // 0's idempotent guard makes a repeated disabled tick a harmless no-op, not a - // tick-counter). Gating the call itself on animateLayout, as the first cut of - // this fix did, left a disabled wall's `sampled_` retained forever: the - // multiview PGM cell and the preview composite (both read via applyLatest, - // which does not know about "enabled") kept applying stale animated geometry - // that the Program plan itself no longer carried. - // + const bool programPresent = tilesLayer_.present; + const bool programEnabled = tilesLayer_.style.animateLayout; + const bool previewPresent = previewTilesLayer_.present && hasPreviewScene(); + const bool previewEnabled = previewTilesLayer_.style.animateLayout; + // Review round 2: ONE ADVANCE PER WALL PER TICK, true by construction rather + // than by a chain of conditions. Round 1 fixed the freeze (Finding B) and the + // stale-geometry retention (Finding C) separately, and their combination + // advanced the SAME shared object TWICE in one tick whenever both buses named + // it with contradictory `enabled` (Program false / Preview true, or vice + // versa): once with enabled=false (a real reset, since it had a key to lose) + // and again with enabled=true (a SECOND reset, since the first call just + // cleared key_) — generation +2/tick with no actual animation, and Preview's + // geometry re-adopted from scratch on every tick. `sameWall` decides whether + // there is one object or two to advance this tick; when it is one, `enabled` + // is the OR of both buses' flags — a shared wall animates if EITHER bus wants + // it to, and there is exactly one advance() call to carry that decision. + const bool sameWall = programPresent && previewPresent && !programWallId.empty() && + programWallId == previewWallId; // The take record's proof that this wall did not restart across the take: // its generation before this tick's advance, compared after (equal == // continuous). Read via the const find() (never forWall(), which would // insert on a mere read) — an unknown wall has never animated and reports - // 0, which is true, not a guess. - const bool programAdvanced = tilesLayer_.present && tilesLayer_.style.animateLayout; + // 0, which is true, not a guess. "Continuous" also requires the wall to have + // EXISTED before this tick — a wall id seen for the first time starts at + // generation 0, and with the idempotent guard below a disabled first call + // also reports "did not reset" (nothing to reset), so raw generation + // equality alone would misread a brand-new wall as continuous. bool wallContinuous = false; - if (tilesLayer_.present) { - // "Continuous" requires the wall to have EXISTED before this tick, not - // just for its generation to be unchanged — a wall id that has NEVER been - // seen (a genuinely different wall, e.g. a plain scene-to-scene cut with - // no animation) starts at generation 0 and, with Finding 0's idempotent - // guard, a disabled first call also reports "did not reset" (nothing to - // reset) — so generation stays 0 on BOTH sides of a brand-new wall's - // first tick. Raw generation equality alone would misread that as - // continuous. find() first (never forWall() for this read — a lookup - // must not decide existence by the side effect of creating it). + if (programPresent) { const auto* existingWall = tilesWallSources_.find(programWallId); const bool wallExistedBefore = existingWall != nullptr; const uint64_t generationBefore = existingWall ? existingWall->generation() : 0; + // advance() runs UNCONDITIONALLY whenever the wall is present — never + // additionally gated on `enabled` — and lets `enabled` decide reset-vs- + // sample INSIDE advance() (the idempotent guard there makes a repeated + // disabled tick a harmless no-op, not a tick-counter). Gating the call + // itself on `enabled` left a disabled wall's `sampled_` retained forever: + // the multiview PGM cell and the preview composite (both read via + // applyLatest, which does not know about "enabled") kept applying stale + // animated geometry the Program plan itself no longer carried. + const bool enabled = sameWall ? (programEnabled || previewEnabled) : programEnabled; tilesWallSources_.forWall(programWallId).advance(renderPlan, programWallId, - tilesLayer_.present, tilesLayer_.style.animateLayout, tilesLayer_.style.animationDurationMs, animationNowMs); + programPresent, enabled, tilesLayer_.style.animationDurationMs, animationNowMs); wallContinuous = wallExistedBefore && tilesWallGeneration(programWallId) == generationBefore; } // Advance Preview on the SAME render clock even when its wall is empty, so - // snapshot/prefetch builds cannot change entry/departure animation state. - // - // Review fix round 1, Finding B: gate on whether PROGRAM actually advanced - // this tick (`programAdvanced`, present && animateLayout — the shape of the - // call that can move this object), not on id equality alone. The two layer - // states are parsed independently (load-scene-graph vs set-preview-scene, - // and the preview draft is separately editable), so their `animateLayout` - // flags can disagree even when they name the SAME wall id. Gating only on - // `previewWallId != programWallId` meant a wall with `animateLayout=false` - // on Program and `true` on Preview, at the same id, got ADVANCED BY NEITHER - // branch — frozen, with applyLatest re-applying stale rects forever. - if (previewTilesLayer_.present && previewTilesLayer_.style.animateLayout && - hasPreviewScene() && (!programAdvanced || previewWallId != programWallId)) { + // snapshot/prefetch builds cannot change entry/departure animation state — + // but ONLY when it names a wall DIFFERENT from Program's (a genuinely + // separate object). When `sameWall`, the program branch above already + // advanced this exact object once this tick, carrying Preview's `enabled` + // into the OR — a second call here would be the round-2 double-advance bug. + if (previewPresent && !sameWall) { auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); tilesWallSources_.forWall(previewWallId).advance(previewAnimationPlan, previewWallId, - true, previewTilesLayer_.style.animateLayout, previewTilesLayer_.style.animationDurationMs, animationNowMs); + true, previewEnabled, previewTilesLayer_.style.animationDurationMs, animationNowMs); } // Lifetime: release any wall no live scene still names (parent spec section 2). std::vector liveWallIds; diff --git a/native/tests/TilesRenderPlanTest.cpp b/native/tests/TilesRenderPlanTest.cpp index 4729dba8..dde347e7 100644 --- a/native/tests/TilesRenderPlanTest.cpp +++ b/native/tests/TilesRenderPlanTest.cpp @@ -1157,6 +1157,49 @@ TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { << "the take record still reads this as a rebuild, not a cut"; } +// Review round 2: a wall present on BOTH buses with DISAGREEING `animateLayout` +// (Program false, Preview true) has NO coverage before this test — which is +// why it took two review rounds to find. Round 1 fixed the freeze (Finding B: +// the double-advance guard keyed on wall-id equality, so this exact +// configuration was advanced by NEITHER branch) and the stale-geometry +// retention (Finding C: advance() must run unconditionally for a present +// wall) as two separate, correct fixes — but combined, they advance the SAME +// shared TilesWallSource TWICE in one tick with contradictory `enabled`: +// Program's own call (enabled=false) resets it (a real reset once it has a +// key to lose), then Preview's separate call (enabled=true) sees an EMPTY key +// and resets it AGAIN. Net per tick: generation +2, no animation ever +// actually completes, and the shared object churns forever. This test must +// FAIL against commit c11862d2 (round 1) and pass after round 2's "one +// advance per wall per tick" restructure. +TEST(TilesRenderPlan, ASharedWallWithDisagreeingAnimateFlagsAdvancesAtMostOncePerTick) { + MediaCore core; + const std::vector members{"zoom:1", "zoom:2"}; + + // PROGRAM: layerId "tiles:s", animateLayout=false (loadWall()'s default — + // it sends no "animateLayout" key at all). + loadWall(core, members); + // PREVIEW: the SAME scene id "s" -> the SAME layerId "tiles:s", + // animateLayout=true (wallScene()/animatedTilesPayload()'s default). + (void)core.applyCommands(corevideo::rpc::Json::Array{ + wallScene("set-preview-scene", "s", members)}); + + const auto generationAfterFirstTick = core.tilesWallGeneration("tiles:s"); + ASSERT_GT(generationAfterFirstTick, 0u) + << "precondition: the shared wall must have animated at least once by now"; + + // Several more ticks with nothing changing: a correctly-shared object + // settles and its generation goes flat. The round-2 bug climbs it by 2 + // EVERY tick — this loop catches that on the very first iteration. + for (int tick = 0; tick < 5; ++tick) { + (void)core.applyCommands(corevideo::rpc::Json::Array{}); + const auto generationNow = core.tilesWallGeneration("tiles:s"); + EXPECT_LE(generationNow, generationAfterFirstTick + 1) + << "tick " << tick << ": the shared wall's generation is still climbing — " + "it is being advanced more than once per tick with contradictory `enabled`, " + "the same object being reset and re-adopted every tick instead of animating once"; + } +} + // THE PROPERTY: a wall settled in preview, then taken, is at its settled state // on the first program frame — a cut, not a redraw. TEST(TilesRenderPlan, AWallSettledInPreviewIsAlreadySettledOnItsFirstProgramFrame) { From c171559b7153d7b442ccfb576bdff74fb698ca7c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 21:27:03 -0400 Subject: [PATCH 13/20] =?UTF-8?q?fix(tiles):=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20no=20wasted=20preview=20plan=20build,=20Program=20w?= =?UTF-8?q?ins=20for=20a=20shared=20wall,=20drop=20empty-id=20exemption?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes from re-review of round 2's restructure: Finding 1 (performance): the preview branch's `!sameWall` gate lost its `animateLayout` check, so buildPreviewCompositorRenderPlan(videoFrames) — a full buildRenderPlanForScene deep build, ~13 strings per layer, plus the paused-clip-cue pass — ran every render tick under coreMutex for any disabled preview wall (the DEFAULT) with a different id from Program, then was thrown away: advance() returns at its very first line when !enabled and never touches the plan. Same class of waste the "Task 4 review fix (I6)" comment a few lines below already exists to prevent. Now the deep plan is only built when previewEnabled is true; the disabled path passes the already-built program `renderPlan` instead (advance() never reads it there). Finding 2 (behaviour, RULING): `enabled = programEnabled || previewEnabled` let a PREVIEW-side animateLayout toggle start motion on PROGRAM for a shared wall, with no take involved, because the shared advance samples straight into the program renderPlan. This codebase's bedrock rule is that an off-air Preview look never changes what is on air (CLAUDE.md: "an off-air Preview look can never take video ... from a Program source") — the preview scene is an operator-editable draft (S2b), so a draft edit reaching Program is a live-show hazard, and Program is inherited by the vcam, every recording and every stream. Changed to `enabled = programEnabled` — Program always wins for a shared wall. Cost-free thanks to round 1's Finding C: a disabled shared wall still resets (idempotently), sampled_ clears, and applyLatest no-ops on the mismatched key_, so Preview simply falls back to raw (non-animated) geometry rather than stale rects — nothing on air moves because of an off-air edit. Finding 3 (minor): dropped the `!programWallId.empty()` term from `sameWall` — two empty ids are the SAME map entry (TilesWallSources keys on the string), so excluding them re-opened the exact two-advance-on-one-object shape round 2 closed. No generation effect, but the wrong shape regardless. Rewrote the round-2 test to assert the NEW property directly: with Program's animateLayout=false, a wall shared with an animateLayout=true Preview never leaves generation 0 — not "settles after climbing once" (round 2's OR behaviour), which the rewritten test now catches if ever restored (verified by temporarily reintroducing the OR: the test fails on tick 0, as expected). Verified: covering suites green (TilesRenderPlan 29, TilesAnimator 11, TakeRecord 11, RenderedSceneAttribution 3), full binary 969/969 across three consecutive runs, check-string-escapes.py clean. Deferred per coordinator instruction, NOT fixed: forWall() admitting an empty wall id on the write path; the rect-tolerance/duration headroom. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/core/MediaCore.cpp | 58 ++++++++++++++++++++++------ native/tests/TilesRenderPlanTest.cpp | 41 ++++++++++++++------ 2 files changed, 76 insertions(+), 23 deletions(-) diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 2998d4d7..4292221e 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -6283,11 +6283,16 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // and again with enabled=true (a SECOND reset, since the first call just // cleared key_) — generation +2/tick with no actual animation, and Preview's // geometry re-adopted from scratch on every tick. `sameWall` decides whether - // there is one object or two to advance this tick; when it is one, `enabled` - // is the OR of both buses' flags — a shared wall animates if EITHER bus wants - // it to, and there is exactly one advance() call to carry that decision. - const bool sameWall = programPresent && previewPresent && !programWallId.empty() && - programWallId == previewWallId; + // there is one object or two to advance this tick; when it is one, there is + // exactly one advance() call to carry the decision (see Finding 2 below for + // which bus's `enabled` that call uses). + // Review round 3, Finding 3: no `!programWallId.empty()` term — two empty + // ids are the SAME map entry (TilesWallSources keys on the string), so + // excluding them re-opens the two-advance-on-one-object path this whole + // restructure exists to close. It has no generation effect (an empty + // wallKey never trips `key_ != wallKey` or the idempotent guard's + // `!key_.empty()`), but the shape is wrong regardless. + const bool sameWall = programPresent && previewPresent && programWallId == previewWallId; // The take record's proof that this wall did not restart across the take: // its generation before this tick's advance, compared after (equal == // continuous). Read via the const find() (never forWall(), which would @@ -6310,7 +6315,24 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // the multiview PGM cell and the preview composite (both read via // applyLatest, which does not know about "enabled") kept applying stale // animated geometry the Program plan itself no longer carried. - const bool enabled = sameWall ? (programEnabled || previewEnabled) : programEnabled; + // Review round 3, Finding 2 (RULING): Program wins for a shared wall. + // `enabled = programEnabled || previewEnabled` let a PREVIEW-side toggle + // start motion on PROGRAM for a shared wall with no take involved, since + // the shared advance samples straight into the program `renderPlan`. This + // codebase's bedrock rule is that an off-air Preview look never changes + // what is on air (CLAUDE.md: "an off-air Preview look can never take + // video ... from a Program source") — the preview scene is an + // operator-editable draft (S2b), so a draft edit reaching Program is a + // live-show hazard, and Program is inherited by the virtual camera, every + // recording and every stream. Do not restore the OR. + // + // This is cost-free thanks to the Finding C fix: a disabled shared wall + // still resets every tick (idempotently after the first), `sampled_` + // clears, and `applyLatest` no-ops on the mismatched `key_` — so Preview + // falls back to raw (non-animated) plan geometry rather than stale rects. + // Preview simply does not animate; nothing on air moves because of an + // off-air edit. + const bool enabled = programEnabled; tilesWallSources_.forWall(programWallId).advance(renderPlan, programWallId, programPresent, enabled, tilesLayer_.style.animationDurationMs, animationNowMs); wallContinuous = wallExistedBefore && tilesWallGeneration(programWallId) == generationBefore; @@ -6319,12 +6341,26 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // snapshot/prefetch builds cannot change entry/departure animation state — // but ONLY when it names a wall DIFFERENT from Program's (a genuinely // separate object). When `sameWall`, the program branch above already - // advanced this exact object once this tick, carrying Preview's `enabled` - // into the OR — a second call here would be the round-2 double-advance bug. + // advanced this exact object once this tick (with Program's `enabled` — + // Finding 2) — a second call here would be the round-2 double-advance bug. if (previewPresent && !sameWall) { - auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); - tilesWallSources_.forWall(previewWallId).advance(previewAnimationPlan, previewWallId, - true, previewEnabled, previewTilesLayer_.style.animationDurationMs, animationNowMs); + // Review round 3, Finding 1: build the deep preview plan ONLY when + // advance() will actually read it. `advance()` returns at its very first + // line when `!enabled`, before touching the plan at all — the same class + // of waste the "Task 4 review fix (I6)" comment below exists to prevent. + // A disabled preview wall (animateLayout=false, the DEFAULT) with a + // different id from Program used to pay a full buildRenderPlanForScene + // deep build (a layer vector, ~13 strings per layer, the paused-clip-cue + // pass) under coreMutex, EVERY tick, for nothing. Pass the already-built + // program `renderPlan` instead — advance() never reads it on this path. + if (previewEnabled) { + auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); + tilesWallSources_.forWall(previewWallId).advance(previewAnimationPlan, previewWallId, + true, true, previewTilesLayer_.style.animationDurationMs, animationNowMs); + } else { + tilesWallSources_.forWall(previewWallId).advance(renderPlan, previewWallId, + true, false, previewTilesLayer_.style.animationDurationMs, animationNowMs); + } } // Lifetime: release any wall no live scene still names (parent spec section 2). std::vector liveWallIds; diff --git a/native/tests/TilesRenderPlanTest.cpp b/native/tests/TilesRenderPlanTest.cpp index dde347e7..05c4f0fa 100644 --- a/native/tests/TilesRenderPlanTest.cpp +++ b/native/tests/TilesRenderPlanTest.cpp @@ -1171,7 +1171,26 @@ TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { // actually completes, and the shared object churns forever. This test must // FAIL against commit c11862d2 (round 1) and pass after round 2's "one // advance per wall per tick" restructure. -TEST(TilesRenderPlan, ASharedWallWithDisagreeingAnimateFlagsAdvancesAtMostOncePerTick) { +// Review round 2 caught: this configuration (same wall id, disagreeing +// `animateLayout`) had NO coverage before it, which is why it took two review +// rounds to find the double-advance bug it exposed. +// +// Review round 3, Finding 2 (RULING) changed what "correct" means here: the +// original version of this test asserted the shared wall's generation settled +// after climbing once (the old `enabled = programEnabled || previewEnabled` +// behaviour) — i.e. Preview's animateLayout=true was allowed to start motion +// on Program. That is a live-show hazard (an off-air Preview draft edit +// reaching Program — CLAUDE.md: "an off-air Preview look can never take video +// ... from a Program source"), so the rule is now PROGRAM WINS: a wall shared +// by both buses uses ONLY Program's `enabled`, never an OR. This test now +// asserts that property directly: with Program's animateLayout=false, the +// shared wall's generation NEVER moves off 0, no matter how many ticks pass +// or what Preview wants — it never even acquires a real key (`advance()` +// takes the early-return branch every tick since `enabled` is `false` +// throughout). If the OR were ever restored, the first tick would establish +// a real key and settle the generation at 1 instead of 0 — this assertion +// would catch that. +TEST(TilesRenderPlan, AProgramDisabledSharedWallNeverAnimatesEvenWhenPreviewWantsIt) { MediaCore core; const std::vector members{"zoom:1", "zoom:2"}; @@ -1183,20 +1202,18 @@ TEST(TilesRenderPlan, ASharedWallWithDisagreeingAnimateFlagsAdvancesAtMostOncePe (void)core.applyCommands(corevideo::rpc::Json::Array{ wallScene("set-preview-scene", "s", members)}); - const auto generationAfterFirstTick = core.tilesWallGeneration("tiles:s"); - ASSERT_GT(generationAfterFirstTick, 0u) - << "precondition: the shared wall must have animated at least once by now"; + EXPECT_EQ(core.tilesWallGeneration("tiles:s"), 0u) + << "the shared wall animated on its very first tick even though Program's " + "animateLayout is false — Preview must never be able to start it"; - // Several more ticks with nothing changing: a correctly-shared object - // settles and its generation goes flat. The round-2 bug climbs it by 2 - // EVERY tick — this loop catches that on the very first iteration. + // Several more ticks with nothing changing: the generation must stay + // pinned at 0 forever, not merely "settle" at some nonzero value (which + // would mean Preview's flag won at least once). for (int tick = 0; tick < 5; ++tick) { (void)core.applyCommands(corevideo::rpc::Json::Array{}); - const auto generationNow = core.tilesWallGeneration("tiles:s"); - EXPECT_LE(generationNow, generationAfterFirstTick + 1) - << "tick " << tick << ": the shared wall's generation is still climbing — " - "it is being advanced more than once per tick with contradictory `enabled`, " - "the same object being reset and re-adopted every tick instead of animating once"; + EXPECT_EQ(core.tilesWallGeneration("tiles:s"), 0u) + << "tick " << tick << ": the shared wall animated even though Program's " + "animateLayout is false — a Preview-only toggle must never move Program"; } } From 909d1010bd5e0b05cbc907ecd1f5b77a4e29438e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 21:41:22 -0400 Subject: [PATCH 14/20] =?UTF-8?q?fix(tiles):=20review=20round=204=20?= =?UTF-8?q?=E2=80=94=20remove=20the=20foreign-plan=20foot-gun,=20add=20a?= =?UTF-8?q?=20real=20precondition?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 1 (Important, coordinator's own prescribed shape): the disabled preview-wall branch passed the PROGRAM render plan into a PREVIEW wall's advance() call. Correct today only because advance() returns at its very first statement when !enabled — an invariant living in a different file from the call site depending on it. If that early return is ever reordered or gains code above it, applyLatest would silently rewrite rect/opacity on every tile:* layer of the PROGRAM plan (the same object handed to compositor->render() and cached into lastRenderPlan_): on-air geometry corruption inherited by the virtual camera, every recording and every stream. Removed the hazard rather than documenting it further: - TilesPlanAnimation::releaseIfIdle() — plan-free by construction, cannot ever read a plan because it takes none. Same idempotent generation contract as advance()'s early return (had = !key_.empty() before reset). - advance()'s early return now just calls releaseIfIdle(). - TilesWallSource::releaseIfIdle() wraps it and calls noteReset() on a real release, matching advance()'s generation semantics exactly. - MediaCore's disabled-preview branch now calls tilesWallSources_.forWall(previewWallId).releaseIfIdle() — no plan argument exists to misuse. - Documented advance()'s `plan` parameter as read ONLY on the present-and-enabled path, so the next person knows releaseIfIdle() is the only correct call when there is no real plan for a wall. - Left the all-stale early return (`targets.empty() && !sampled_.empty()`) untouched — it runs after the key_ assignment and was never part of this hazard. Finding 2 (Minor): the round-3 test was entirely negative assertions (generation == 0), which pass vacuously if the shared configuration itself stopped existing. Added preconditions: Program's wall must actually render "tile:zoom:1" (forced via the existing setTilesMemberFrameAgesForTest seam, which only rebuilds lastRenderPlan_ and cannot disturb tilesWallSources_/generation), and Preview's scene must have landed on the SAME "tiles:s" wall id with animateLayout true — before the zero-generation assertions run. Verified: covering suites green (TilesRenderPlan 29, TilesAnimator 11, TakeRecord 11, RenderedSceneAttribution 3), full binary 969/969 across two consecutive runs, check-string-escapes.py clean. Generation semantics are unchanged by the refactor by construction (releaseIfIdle IS the old early return, extracted) — the round-2/3 tests pass for the same reason as before. Still deferred per coordinator instruction: forWall() admitting an empty wall id on the write path; the rect-tolerance/duration headroom. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/compositor/TilesPlanAnimation.h | 26 +++++++++++++++++----- native/src/core/MediaCore.cpp | 16 +++++++++---- native/src/core/TilesWallSource.h | 12 ++++++++++ native/tests/TilesRenderPlanTest.cpp | 24 ++++++++++++++++++++ 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/native/src/compositor/TilesPlanAnimation.h b/native/src/compositor/TilesPlanAnimation.h index abf4c5f8..c593e371 100644 --- a/native/src/compositor/TilesPlanAnimation.h +++ b/native/src/compositor/TilesPlanAnimation.h @@ -9,16 +9,32 @@ class TilesPlanAnimation { public: void reset() { animator_.reset(); key_.clear(); sampled_.clear(); } + // Release a wall that is present but not animating (or not present at all). + // Plan-free BY DESIGN (review round 4, Finding 1): the caller has no real + // plan to give this wall on this path, and passing a FOREIGN one (e.g. the + // program plan, on behalf of a preview wall that shares its object) would + // only be safe as long as advance() returns before ever touching `plan` — + // an invariant that lives in a different file from the call site depending + // on it, and silently breaks into on-air geometry corruption the moment + // advance() is reordered or gains code above its early return. Removing the + // hazard is cheaper than documenting it: this takes no plan and cannot ever + // read one. Idempotent, matching advance()'s early-return semantics + // exactly: an ALREADY-released wall (key_ empty) reports it did NOT reset — + // without this a caller that releases every tick regardless of presence + // would read "reset" forever, turning the generation into a tick counter + // instead of a restart signal. + bool releaseIfIdle() { const bool had = !key_.empty(); reset(); return had; } + // Returns true when this call RESET the animator (a departure/disable, or a // different wall key arriving) - the caller's only truthful signal of "did // this wall restart", with no std::function/allocation on the render tick. + // `plan` is read ONLY on this present-and-enabled path (target extraction + + // applyLatest at the end) — a caller with no real plan for this wall must + // use releaseIfIdle() above instead of passing one in, never a plan built + // for a DIFFERENT wall. bool advance(modules::CompositorRenderPlan& plan, const std::string& wallKey, bool present, bool enabled, double durationMs, double nowMs) { - // Idempotent: an ALREADY-reset wall (key_ empty) reports it did NOT reset - // on a repeated not-present/disabled tick — without this a caller that - // advances every tick regardless of presence would read "reset" forever, - // turning the generation into a tick counter instead of a restart signal. - if (!present || !enabled) { const bool had = !key_.empty(); reset(); return had; } + if (!present || !enabled) return releaseIfIdle(); // A DIFFERENT wall never inherits this one's geometry. (sampled_ is cleared // too: the all-stale guard below would otherwise let a new wall's first // frames be drawn at the previous wall's tile rects.) diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 4292221e..27e01946 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -6351,15 +6351,23 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // A disabled preview wall (animateLayout=false, the DEFAULT) with a // different id from Program used to pay a full buildRenderPlanForScene // deep build (a layer vector, ~13 strings per layer, the paused-clip-cue - // pass) under coreMutex, EVERY tick, for nothing. Pass the already-built - // program `renderPlan` instead — advance() never reads it on this path. + // pass) under coreMutex, EVERY tick, for nothing. + // + // Review round 4, Finding 1: the disabled branch used to pass the + // PROGRAM `renderPlan` here — safe only because advance() returns before + // ever reading `plan` on the disabled path, an invariant that lives in a + // different file from this call and would silently start rewriting + // Program's `tile:*` layer rects/opacity (the exact object handed to + // compositor->render() and cached into lastRenderPlan_) the moment that + // early return is reordered. releaseIfIdle() takes no plan and cannot + // ever read one, so the hazard does not exist rather than being merely + // documented. if (previewEnabled) { auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); tilesWallSources_.forWall(previewWallId).advance(previewAnimationPlan, previewWallId, true, true, previewTilesLayer_.style.animationDurationMs, animationNowMs); } else { - tilesWallSources_.forWall(previewWallId).advance(renderPlan, previewWallId, - true, false, previewTilesLayer_.style.animationDurationMs, animationNowMs); + tilesWallSources_.forWall(previewWallId).releaseIfIdle(); } } // Lifetime: release any wall no live scene still names (parent spec section 2). diff --git a/native/src/core/TilesWallSource.h b/native/src/core/TilesWallSource.h index 7fd84751..c23e9c1e 100644 --- a/native/src/core/TilesWallSource.h +++ b/native/src/core/TilesWallSource.h @@ -34,6 +34,18 @@ class TilesWallSource final { } } + // Review round 4, Finding 1: release a wall that is present but not + // animating, with no plan argument at all — see + // TilesPlanAnimation::releaseIfIdle for why a caller must never substitute + // a plan built for a DIFFERENT wall on this path. Same generation contract + // as advance(): a reset (key_ was non-empty) bumps it, an already-idle wall + // does not. + void releaseIfIdle() { + if (animation_.releaseIfIdle()) { + noteReset(); + } + } + void applyLatest(modules::CompositorRenderPlan& plan, const std::string& wallId) const { animation_.applyLatest(plan, wallId); } diff --git a/native/tests/TilesRenderPlanTest.cpp b/native/tests/TilesRenderPlanTest.cpp index 05c4f0fa..634d2726 100644 --- a/native/tests/TilesRenderPlanTest.cpp +++ b/native/tests/TilesRenderPlanTest.cpp @@ -1202,6 +1202,30 @@ TEST(TilesRenderPlan, AProgramDisabledSharedWallNeverAnimatesEvenWhenPreviewWant (void)core.applyCommands(corevideo::rpc::Json::Array{ wallScene("set-preview-scene", "s", members)}); + // Review round 4, Finding 2: prove the SHARED configuration this test is + // about actually exists before asserting on it — an all-negative test + // (generation == 0 forever) passes VACUOUSLY if the configuration silently + // stops existing (set-preview-scene stops populating previewTilesLayer_, + // hasPreviewScene() goes false, or the layerId derivation changes so the + // two buses no longer share "tiles:s"). Program's own wall must be present + // and drawing real tiles — a plain MediaCore() has no real source to admit + // zoom:1/zoom:2, so force admission the same way + // EachAdmittedMemberBecomesOneTileLayer does. This only rebuilds + // lastRenderPlan_ via buildCompositorRenderPlan (see the seam's own + // comment) — it does NOT touch tilesWallSources_/generation, so it cannot + // disturb the property under test below. + core.setTilesMemberFrameAgesForTest({{"zoom:1", true, 0}, {"zoom:2", true, 0}}); + ASSERT_NE(findLayer(core.lastRenderPlanForTest(), "tile:zoom:1"), nullptr) + << "precondition: Program's wall never rendered its tiles"; + // ...and Preview's scene must have been accepted onto the SAME wall id, not + // silently rejected or parsed onto some other layerId. + ASSERT_TRUE(core.previewTilesLayerForTest().present) + << "precondition: the preview scene was not accepted"; + ASSERT_EQ(core.previewTilesLayerForTest().layerId, "tiles:s") + << "precondition: preview did not land on the SAME wall id as Program"; + ASSERT_TRUE(core.previewTilesLayerForTest().style.animateLayout) + << "precondition: Preview's animateLayout must be true for this test to mean anything"; + EXPECT_EQ(core.tilesWallGeneration("tiles:s"), 0u) << "the shared wall animated on its very first tick even though Program's " "animateLayout is false — Preview must never be able to start it"; From bfc08edba951e1235be3a2422e9cbfb96f60c1fc Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 22:08:24 -0400 Subject: [PATCH 15/20] feat(tiles): register the wall as a composed source, releasing it when unreferenced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wall is the first real consumer of SourceRegistry's Kind::Composed (#448 slice 2, task 4). Registers as Composed the tick a wall becomes live (sourceId = its layerId, externalId empty, a fixed core-process epoch) and releases it the tick nothing on either bus names it any longer, in lockstep with tilesWallSources_.releaseAllExcept — the same "named by a live scene" lifetime, one level up. registeredWallIds_ is the idempotence guard so a live wall's steady-state tick never touches the registry mutex. SourceRegistry had no way to actually remove a composed source (real sources are tombstoned via retireProcessEpoch/setAvailability, which explicitly SKIP composed sources with a comment deferring removal to "a separate sweep (later task)"). Added SourceRegistry::removeComposed, which erases outright (never tombstones) and refuses non-Composed kinds - the wall has no provider process to fence, so a tombstone would be a permanent, meaningless entry that also makes add() answer Conflict forever for a freed wall id. Per explicit ruling, this task does NOT touch TakeRecordPolicy or the take record's sources[] - that continuity fact already exists via wallContinuous, and a second representation of it is exactly what this codebase forbids elsewhere (one status machine per fact). Tests: SourceRegistryComposedTest.cpp (removeComposed: erases, frees the id for reuse, refuses non-composed, NotFound on unknown) and TilesRenderPlanTest.cpp (a live wall registers as Composed with all five capture-only fields nullopt; an unreferenced wall is gone from the registry; a released-then-re-cued wall gets a new instanceId; a wall staying live across many ticks holds exactly one registry entry). Full suite: 977 passed, 0 failed (was 969). Red-before-green verified by reverting MediaCore.cpp/.h to HEAD with the new tests in place (compile failure), then restoring. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/src/core/MediaCore.cpp | 45 +++++++++ native/src/core/MediaCore.h | 24 +++++ native/src/core/SourceRegistry.cpp | 14 +++ native/src/core/SourceRegistry.h | 11 +++ native/tests/SourceRegistryComposedTest.cpp | 48 ++++++++++ native/tests/TilesRenderPlanTest.cpp | 100 ++++++++++++++++++++ 6 files changed, 242 insertions(+) diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 27e01946..7bcdb667 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -6375,6 +6375,51 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim if (tilesLayer_.present) liveWallIds.push_back(programWallId); if (previewTilesLayer_.present) liveWallIds.push_back(previewWallId); tilesWallSources_.releaseAllExcept(liveWallIds); + // Task 4: the wall is a SOURCE (parent spec section 2), so it registers like + // one — the first real consumer of SourceRegistry's Kind::Composed. Its + // lifetime mirrors tilesWallSources_'s own exactly (same liveWallIds set, + // same "named by a live scene" rule), one level up. registeredWallIds_ is + // the idempotence guard: add() answers Conflict on a repeat, so without it + // a live wall's steady-state tick would take the registry mutex and get + // refused every single frame — this keeps that cost to liveness + // TRANSITIONS only. A composed registration carries no externalId (a wall + // has no SDK handle) and never claims a subscription state — see + // SourceRegistry::Kind::Composed and validRegistration. + for (const auto& wallId : liveWallIds) { + // A deferred edge case from Task 3 (forWall admits an empty wall id): + // skip it here rather than registering a nameless source that + // validRegistration would refuse as Invalid anyway. + if (wallId.empty()) continue; + if (registeredWallIds_.insert(wallId).second) { + SourceRegistry::Registration registration; + registration.sourceId = {wallId}; + registration.kind = SourceRegistry::Kind::Composed; + registration.displayName = "Tiles wall " + wallId; + registration.processEpoch = kCoreProcessEpoch; + const auto mutation = sourceRegistry_.add(std::move(registration)); + if (mutation.result != SourceRegistry::Result::Applied) { + // Only reachable via Conflict/Exhausted — an ordinary liveness + // transition never hits this. Loud, since a wall that fails to + // register is silently invisible to any future registry consumer. + ::corevideo::core::nativeLogf("[source-registry] wall '%s' failed to register (result=%d)\n", + wallId.c_str(), static_cast(mutation.result)); + } + } + } + // Release from the registry whatever the sweep above just released from + // tilesWallSources_. A wall has no provider process to fence, so this is a + // genuine erase (SourceRegistry::removeComposed), not a tombstone — and the + // id must come out of registeredWallIds_ too, or a wall recreated under the + // same scene id would find add() answering Conflict against a guard entry + // for a wall that no longer exists in the registry at all. + for (auto it = registeredWallIds_.begin(); it != registeredWallIds_.end();) { + if (std::find(liveWallIds.begin(), liveWallIds.end(), *it) != liveWallIds.end()) { + ++it; + continue; + } + sourceRegistry_.removeComposed(SourceId{*it}); + it = registeredWallIds_.erase(it); + } // Task 4: cache the plan the render tick actually built — lastRenderPlanForTest() // and the sessionState() `tiles` node both read THIS, so a consumer can never // observe a wall the compositor did not also receive (see modules_.compositor-> diff --git a/native/src/core/MediaCore.h b/native/src/core/MediaCore.h index 788fe1b0..1a09e5ca 100644 --- a/native/src/core/MediaCore.h +++ b/native/src/core/MediaCore.h @@ -11,6 +11,7 @@ #include "core/RenderedProgramSources.h" #include "core/RenderedSceneAttributionPolicy.h" #include "core/SourceContinuityLedger.h" +#include "core/SourceRegistry.h" #include "core/TakeRecordPolicy.h" #include "core/ProgramAudioDelay.h" #include "core/PluginHostScan.h" @@ -273,6 +274,12 @@ class MediaCore { // Also the test seam: the headline #448 regression test reads this directly // to assert a mid-animation Take does not bump the wall's generation. [[nodiscard]] uint64_t tilesWallGeneration(const std::string& wallId) const; + // Task 4: the SourceRegistry snapshot, for tests asserting the wall's own + // Kind::Composed registration/release — the same snapshot() a real consumer + // (a future ShowPlanGenerator) would read. + [[nodiscard]] std::shared_ptr sourceRegistrySnapshotForTest() const { + return sourceRegistry_.snapshot(); + } const std::vector& sceneValidationWarningsForTest() const { return sceneValidationWarnings_; } @@ -529,6 +536,23 @@ class MediaCore { // One animation per WALL, not per bus (#448). See core/TilesWallSource.h for // why the per-bus pair and its hand-off were wrong. core::TilesWallSources tilesWallSources_; + // Task 4: the wall is the first real consumer of SourceRegistry. Registered + // as Kind::Composed the tick a wall becomes live, released the tick nothing + // names it any longer - the SAME "referenced by a live scene" lifetime + // tilesWallSources_ already implements (releaseAllExcept above), one level + // up. registeredWallIds_ is the idempotence guard: add() answers Conflict on + // a repeat, so without it every render tick for a live wall would take the + // registry mutex and get refused - a per-tick cost for a value that only + // actually changes on liveness transitions. + // + // kCoreProcessEpoch is a fixed label, not a fresh-per-instance token: a wall + // has no provider process to fence (SourceRegistry::removeComposed erases it + // outright instead of tombstoning by epoch), so nothing here ever calls + // retireProcessEpoch against it, and a stable constant is honest - it names + // "this core process," not an incarnation that could be replaced mid-run. + static constexpr const char* kCoreProcessEpoch = "core-process"; + core::SourceRegistry sourceRegistry_{"core-registry"}; + std::unordered_set registeredWallIds_; // Task 4: per-member frame-age snapshot for the wall expansion, refreshed // every render tick from the live videoFrames gather (renderSyntheticTick, // under coreMutex — geometry bookkeeping, not pixel work). Covers members of diff --git a/native/src/core/SourceRegistry.cpp b/native/src/core/SourceRegistry.cpp index 4766cb05..21f986df 100644 --- a/native/src/core/SourceRegistry.cpp +++ b/native/src/core/SourceRegistry.cpp @@ -232,6 +232,20 @@ SourceRegistry::Result SourceRegistry::retireProcessEpoch(const std::string& pro return Result::Applied; } +SourceRegistry::Result SourceRegistry::removeComposed(const SourceId& sourceId) { + std::lock_guard lock(mutex_); + const auto found = sources_.find(sourceId.value); + if (found == sources_.end()) return Result::NotFound; + // Refuse for any non-composed kind - see the header comment for why a real + // source's record must be tombstoned, never erased outright. + if (found->second.kind != Kind::Composed) return Result::Invalid; + if (revision_ == kMaxRevision) return Result::Exhausted; + sources_.erase(found); + ++revision_; + ++decisionRevision_; + return Result::Applied; +} + SourceRegistry::Result SourceRegistry::publish(const Token& token, uint64_t sequence, int64_t observedNs, Format format) { std::lock_guard lock(mutex_); const auto found = sources_.find(token.sourceId.value); diff --git a/native/src/core/SourceRegistry.h b/native/src/core/SourceRegistry.h index 3c3575de..982724f2 100644 --- a/native/src/core/SourceRegistry.h +++ b/native/src/core/SourceRegistry.h @@ -94,6 +94,17 @@ class SourceRegistry final { // Retire every source owned by a replaced helper process as one registry // transaction. This fences callbacks even when a provider changes its source IDs. Result retireProcessEpoch(const std::string& processEpoch); + // Composed-only: erases the source outright rather than tombstoning it. + // Every other kind's departure is Availability::Departed, kept deliberately + // (retireProcessEpoch, setAvailability) so a late callback or a diagnostic + // read can still see what a provider incarnation was. A composed source has + // no provider process and no callback to fence against - its lifetime is + // "named by a live scene" (parent spec section 2), so once nothing names it + // the tombstone would just be a permanent, meaningless entry, and it would + // make add() answer Conflict forever for a wall id that is free to reuse. + // Refuses (Invalid) for any other kind: erasing a real source's record is + // the false-erasure this registry exists to prevent from the other direction. + Result removeComposed(const SourceId& sourceId); Result publish(const Token& token, uint64_t sequence, int64_t observedNs, Format format); [[nodiscard]] std::shared_ptr snapshot() const; // Discovery only: zero/multiple matches remain explicit, with no auto-binding. diff --git a/native/tests/SourceRegistryComposedTest.cpp b/native/tests/SourceRegistryComposedTest.cpp index 1f9f0603..d814d62a 100644 --- a/native/tests/SourceRegistryComposedTest.cpp +++ b/native/tests/SourceRegistryComposedTest.cpp @@ -133,4 +133,52 @@ TEST(SourceRegistryComposed, TwoWallsSharingAnInstanceIdStillConflict) { second.instanceId = corevideo::core::SourceInstanceId{"shared-wall-instance"}; EXPECT_EQ(registry.add(second).result, SourceRegistry::Result::Conflict); } + +// Task 4: removeComposed ERASES a wall outright (never tombstones it) - a +// wall's lifetime is "named by a live scene," not a provider process to fence. +TEST(SourceRegistryComposed, RemoveComposedErasesTheWallFromTheSnapshot) { + SourceRegistry registry("registry-epoch-1"); + ASSERT_EQ(registry.add(wallRegistration("tiles:scene-a")).result, + SourceRegistry::Result::Applied); + + EXPECT_EQ(registry.removeComposed(corevideo::core::SourceId{"tiles:scene-a"}), + SourceRegistry::Result::Applied); + + const auto snapshot = registry.snapshot(); + EXPECT_TRUE(snapshot->sources.empty()); +} + +// A wall id freed by removeComposed is genuinely free to reuse - add() must +// not answer Conflict against a tombstone that no longer exists. +TEST(SourceRegistryComposed, ARemovedWallCanBeReRegisteredAsANewSource) { + SourceRegistry registry("registry-epoch-1"); + ASSERT_EQ(registry.add(wallRegistration("tiles:scene-a")).result, + SourceRegistry::Result::Applied); + ASSERT_EQ(registry.removeComposed(corevideo::core::SourceId{"tiles:scene-a"}), + SourceRegistry::Result::Applied); + + const auto second = registry.add(wallRegistration("tiles:scene-a")); + EXPECT_EQ(second.result, SourceRegistry::Result::Applied); +} + +TEST(SourceRegistryComposed, RemoveComposedRefusesANonComposedSource) { + SourceRegistry registry("registry-epoch-1"); + SourceRegistry::Registration camera; + camera.sourceId = {"camera-alice"}; + camera.kind = SourceRegistry::Kind::ParticipantVideo; + camera.displayName = "Alice"; + camera.processEpoch = "zoom-process-1"; + camera.externalId = "alice-sdk-id"; + ASSERT_EQ(registry.add(camera).result, SourceRegistry::Result::Applied); + + EXPECT_EQ(registry.removeComposed(corevideo::core::SourceId{"camera-alice"}), + SourceRegistry::Result::Invalid); + EXPECT_EQ(registry.snapshot()->sources.size(), 1U); +} + +TEST(SourceRegistryComposed, RemoveComposedOnAnUnknownIdIsNotFound) { + SourceRegistry registry("registry-epoch-1"); + EXPECT_EQ(registry.removeComposed(corevideo::core::SourceId{"tiles:never-added"}), + SourceRegistry::Result::NotFound); +} } // namespace diff --git a/native/tests/TilesRenderPlanTest.cpp b/native/tests/TilesRenderPlanTest.cpp index 634d2726..f710baf3 100644 --- a/native/tests/TilesRenderPlanTest.cpp +++ b/native/tests/TilesRenderPlanTest.cpp @@ -1438,3 +1438,103 @@ TEST(TilesRenderPlan, AStaleBackgroundIsHeldButAnAbsentOneIsNeverFabricated) { // is above the admission gate and is what program falls back to. EXPECT_NE(findLayer(core.lastRenderPlanForTest(), "tiles-bg:tiles:pinned"), nullptr); } + +namespace { +using corevideo::core::SourceRegistry; + +const SourceRegistry::Source* findRegisteredSource( + const std::shared_ptr& snapshot, const std::string& sourceId) { + if (!snapshot) return nullptr; + for (const auto& source : snapshot->sources) { + if (source.token.sourceId.value == sourceId) return &source; + } + return nullptr; +} +} // namespace + +// Task 4: the wall is the first real consumer of SourceRegistry — it registers +// as a Kind::Composed source the tick it becomes live, with its five +// capture-only fields left nullopt (a wall has no SDK handle, no availability +// concept, and is never subscribed — SourceRegistry::Kind::Composed). +TEST(TilesRenderPlan, ALiveWallRegistersAsAComposedSourceInTheRegistry) { + MediaCore core; + loadWall(core, {"zoom:1", "zoom:2"}); + + // Bind the shared_ptr before taking a pointer into it (SourceRegistryComposedTest's + // own rule) - `findRegisteredSource(core.sourceRegistrySnapshotForTest(), ...)` as + // one expression leaves `wall` dangling the instant the temporary shared_ptr's + // refcount drops to zero at the semicolon. + const auto snapshot = core.sourceRegistrySnapshotForTest(); + const auto* wall = findRegisteredSource(snapshot, "tiles:s"); + ASSERT_NE(wall, nullptr) << "a live wall must appear in the SourceRegistry snapshot"; + EXPECT_EQ(wall->kind, SourceRegistry::Kind::Composed); + EXPECT_FALSE(wall->personId.has_value()); + EXPECT_FALSE(wall->externalId.has_value()); + EXPECT_FALSE(wall->availability.has_value()); + EXPECT_FALSE(wall->subscriptionRequested.has_value()); + EXPECT_FALSE(wall->subscriptionObserved.has_value()); +} + +// Lifetime is "named by a live scene" (parent spec section 2) — the SAME rule +// tilesWallSources_.releaseAllExcept already implements one level down. Once +// no scene on either bus names the wall, it must be genuinely gone from the +// registry, not tombstoned (a wall has no provider process to fence). +TEST(TilesRenderPlan, AWallNoSceneReferencesIsGoneFromTheRegistry) { + MediaCore core; + loadWall(core, {"zoom:1"}); + ASSERT_NE(findRegisteredSource(core.sourceRegistrySnapshotForTest(), "tiles:s"), nullptr); + + (void)core.applyCommands(corevideo::rpc::Json::Array{wallLessScene("load-scene-graph", "solo")}); + + EXPECT_EQ(findRegisteredSource(core.sourceRegistrySnapshotForTest(), "tiles:s"), nullptr) + << "a wall no scene still names must be gone from the registry, not merely departed"; +} + +// A wall released and then re-cued under the SAME scene id is a NEW source, +// never a resurrection of the old registry entry — mirroring +// core::TilesWallSource's own "a recreated wall starts at generation 0, never +// continuing a retired one's count." The registry has no generation counter +// per composed source to reuse, so the discriminator is the registry-minted +// instanceId: install() mints a fresh one from the CURRENT revision every +// time, so a genuinely new install() call can never mint the same value twice. +TEST(TilesRenderPlan, AWallReleasedAndReCuedRegistersAsANewSource) { + MediaCore core; + loadWall(core, {"zoom:1"}); + // Bind each snapshot before taking a pointer into it — see the comment on + // the headline registration test above for why the unbound one-expression + // form dangles. + const auto firstSnapshot = core.sourceRegistrySnapshotForTest(); + const auto* first = findRegisteredSource(firstSnapshot, "tiles:s"); + ASSERT_NE(first, nullptr); + const auto firstInstanceId = first->token.instanceId.value; + + (void)core.applyCommands(corevideo::rpc::Json::Array{wallLessScene("load-scene-graph", "solo")}); + ASSERT_EQ(findRegisteredSource(core.sourceRegistrySnapshotForTest(), "tiles:s"), nullptr); + + loadWall(core, {"zoom:1"}); + const auto secondSnapshot = core.sourceRegistrySnapshotForTest(); + const auto* second = findRegisteredSource(secondSnapshot, "tiles:s"); + ASSERT_NE(second, nullptr) << "re-cueing the same scene id must register again"; + EXPECT_NE(second->token.instanceId.value, firstInstanceId) + << "a re-cued wall is a NEW registry entry, not the old one come back"; +} + +// The idempotence guard (registeredWallIds_) exists so a live wall's +// steady-state tick never touches the registry mutex again once registered — +// but the observable contract this test pins is simpler and still real: +// however many render ticks a wall stays live, the registry holds exactly +// ONE entry for it, never zero (a lost registration) and never more than one. +TEST(TilesRenderPlan, ALiveWallHoldsExactlyOneRegistryEntryAcrossManyTicks) { + MediaCore core; + loadWall(core, {"zoom:1"}); + + for (int tick = 0; tick < 25; ++tick) { + (void)core.applyCommands(corevideo::rpc::Json::Array{}); + } + + const auto snapshot = core.sourceRegistrySnapshotForTest(); + ASSERT_NE(snapshot, nullptr); + const auto count = std::count_if(snapshot->sources.begin(), snapshot->sources.end(), + [](const auto& source) { return source.token.sourceId.value == "tiles:s"; }); + EXPECT_EQ(count, 1); +} From b2e8ce4f6fc29f738c76c887a8b3ba93af2e3f7d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 22:24:30 -0400 Subject: [PATCH 16/20] fix(tiles): review round 1 - falsifiable idempotence test, retry-on-failure, alloc-free by construction Fix round 1 of the Task 4 wall-registers-as-composed-source review. Finding 1 (Important): ALiveWallHoldsExactlyOneRegistryEntryAcrossManyTicks asserted a count std::map makes structurally impossible to violate, and was near-unfalsifiable - dropping the idempotence guard entirely (add() answering Conflict on every tick, taking the registry mutex 60x/s on the render path) still passed it. Replaced with ALiveWallKeepsTheSameRegistryIdentityAcrossManyTicks, asserting the wall's instanceId is identical across 25 ticks - the realistic regression (remove-and-re-add every tick) mints a fresh instanceId, which this catches and a count cannot. Verified genuinely falsifiable by simulating that exact regression and confirming red, then restoring and confirming green. Finding 2 (Minor): a registration that failed (Invalid is reachable, not just Conflict/Exhausted) was remembered as registered anyway, so it was never retried while the wall stayed live. Now the guard id is only added on Result::Applied, so the next tick retries a failed wall. Finding 3 (Minor): registeredWallIds_.insert(id).second is not guaranteed allocation-free on an already-present key on every implementation (MSVC's unordered_set has historically built the node before detecting the duplicate). Switched to contains()-then-insert so the render-path no-allocation rule holds by construction. Finding 4 (Minor): three SourceRegistry comment fixes - removeComposed takes a bare SourceId deliberately (sole-owner lifetime, no replace()- style fence); retireProcessEpoch's composed-skip comment names removeComposed instead of "later task" and explains why a tombstone was never actually reachable (setAvailability refuses Composed, and a permanently-nullopt availability makes externalConflict's Departed check permanently true); Snapshot::sources notes composed sources vanish rather than tombstone. Finding 5 (Minor, merge-blocking): added a CLAUDE.md section documenting the composed-source erase-vs-tombstone asymmetry and both reasons for it, per this repo's docs-updated-is-part-of-done rule. Full suite: 977 passed, 0 failed (x2 consecutive). Escape scanner clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- CLAUDE.md | 57 ++++++++++++++++++++++++++++ native/src/core/MediaCore.cpp | 38 ++++++++++++------- native/src/core/SourceRegistry.cpp | 13 ++++++- native/src/core/SourceRegistry.h | 11 +++++- native/tests/TilesRenderPlanTest.cpp | 38 ++++++++++++++----- 5 files changed, 130 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 914dcd1c..2d5fdb8c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1325,6 +1325,63 @@ red/green, the policies, and the take record end to end), `native/tests/SourceContinuityLedgerTest.cpp`, and `ZoomEngineRuntime.SubscriptionChurnNamesResolutionChangesAndTeardowns`. +## The Tiles wall is a composed source, and composed sources are ERASED not tombstoned (#448 slice 2 task 4, 2026-09-12) + +`SourceRegistry` (`native/src/core/SourceRegistry.h`, carved out of #419 unwired +onto main) gained `Kind::Composed` for sources the CORE renders rather than +captures — the Tiles wall is the first one. `MediaCore::renderSyntheticTick` +registers a live wall as `Kind::Composed` (sourceId = its layerId, `externalId` +empty, a fixed `kCoreProcessEpoch`) and releases it the tick nothing on either +bus names it any longer, in lockstep with `tilesWallSources_.releaseAllExcept` — +the same "referenced by a live scene" lifetime, one level up. `registeredWallIds_` +is the idempotence guard so a live wall's steady-state tick never touches the +registry mutex (`unordered_set::contains` before `insert`, not `insert().second` +— MSVC's `unordered_set::insert` has historically built the node before +detecting the duplicate, so this file's render-path no-allocation rule holds by +construction, not by implementation detail). A registration that fails +(`Invalid`/`Conflict`/`Exhausted`) is NOT remembered as registered, so the next +liveness transition retries it rather than abandoning the wall silently forever. + +**A composed source carries no SDK handle and never claims a subscription +state.** `personId`, `externalId`, `availability`, `subscriptionRequested`, +`subscriptionObserved` are all `nullopt` for it — `nullopt` means NOT +APPLICABLE, never false — because a wall has no provider process and nothing +ever subscribes to it. `setAvailability`/`setSubscription` refuse `Composed` +outright for exactly this reason. + +**A composed source is ERASED (`SourceRegistry::removeComposed`), never +tombstoned — and this is not a simplification, it is the only mechanism that +actually works.** Every other kind's departure is `Availability::Departed` +(kept for diagnostics via `retireProcessEpoch`/`setAvailability`). A composed +entry CANNOT be tombstoned that way even in principle: `setAvailability` +refuses `Composed`, so nothing can ever flip it to `Departed`, and because its +`availability` stays `nullopt` forever, `externalConflict`'s +`availability != Departed` test reads true for it PERMANENTLY — a tombstoned +wall id could never be reused by `add()` again. `removeComposed` erases the +`sources_` entry outright and refuses (`Invalid`) for any non-`Composed` kind. +It takes a bare `SourceId`, deliberately not a `Token`: the caller must be the +SOLE owner of a composed source's lifetime (`replace()` exists precisely so an +OLD callback cannot retire a NEW instance it no longer owns via compare-and- +replace; removal has no such fence and must never grow a second writer). + +**Its lifetime is scene-reference, exactly like `TilesWallSources` +(`releaseAllExcept`) one level down** — a wall no live scene names is gone from +the registry the same render tick `tilesWallSources_` releases its animation +object, and a wall released then re-cued under the same scene id is a +genuinely NEW registry entry (a fresh `instanceId`, minted from the registry's +own revision counter), never the old one resurrected. + +Tests: `native/tests/SourceRegistryComposedTest.cpp` (`removeComposed`: erases, +frees the id for reuse, refuses non-composed, `NotFound` on an unknown id) and +`native/tests/TilesRenderPlanTest.cpp` (a live wall registers as `Composed` +with all five capture-only fields `nullopt`; an unreferenced wall is gone from +the registry; a released-then-re-cued wall gets a new `instanceId`; a wall +staying live across many ticks keeps the SAME registry identity — pinned by +`instanceId` equality, not a source count, because `sources_` is a +`std::map` keyed by sourceId where a count assertion cannot distinguish "the +guard works" from "every tick refuses `Conflict` while quietly taking the +registry mutex 60x/s"). + ## Media is a persistent source (slice 1, 2026-09-10) Slice 1 of `docs/superpowers/specs/2026-09-10-persistent-sources-design.md`: a diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 7bcdb667..931fbd5a 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -6390,20 +6390,30 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // skip it here rather than registering a nameless source that // validRegistration would refuse as Invalid anyway. if (wallId.empty()) continue; - if (registeredWallIds_.insert(wallId).second) { - SourceRegistry::Registration registration; - registration.sourceId = {wallId}; - registration.kind = SourceRegistry::Kind::Composed; - registration.displayName = "Tiles wall " + wallId; - registration.processEpoch = kCoreProcessEpoch; - const auto mutation = sourceRegistry_.add(std::move(registration)); - if (mutation.result != SourceRegistry::Result::Applied) { - // Only reachable via Conflict/Exhausted — an ordinary liveness - // transition never hits this. Loud, since a wall that fails to - // register is silently invisible to any future registry consumer. - ::corevideo::core::nativeLogf("[source-registry] wall '%s' failed to register (result=%d)\n", - wallId.c_str(), static_cast(mutation.result)); - } + // Review round 1, Finding 3: check-then-insert, not insert-and-read-.second. + // unordered_set::insert on an already-present key is not guaranteed + // allocation-free on every implementation (MSVC's has historically built + // the node before discovering the duplicate) — this file's render-path + // allocation rule should hold BY CONSTRUCTION, not by implementation detail. + if (registeredWallIds_.contains(wallId)) continue; + SourceRegistry::Registration registration; + registration.sourceId = {wallId}; + registration.kind = SourceRegistry::Kind::Composed; + registration.displayName = "Tiles wall " + wallId; + registration.processEpoch = kCoreProcessEpoch; + const auto mutation = sourceRegistry_.add(std::move(registration)); + if (mutation.result == SourceRegistry::Result::Applied) { + registeredWallIds_.insert(wallId); + } else { + // Review round 1, Finding 2: do NOT remember this id as registered on + // failure — Invalid (e.g. an over-length wire layerId, or a retired + // processEpoch) is genuinely reachable, not just Conflict/Exhausted, and + // a wall that fails to register must be RETRIED on the next liveness + // transition, not silently abandoned for the rest of its life. Loud, + // since a wall invisible to the registry is invisible to any future + // registry consumer too. + ::corevideo::core::nativeLogf("[source-registry] wall '%s' failed to register (result=%d)\n", + wallId.c_str(), static_cast(mutation.result)); } } // Release from the registry whatever the sweep above just released from diff --git a/native/src/core/SourceRegistry.cpp b/native/src/core/SourceRegistry.cpp index 21f986df..1d3a0891 100644 --- a/native/src/core/SourceRegistry.cpp +++ b/native/src/core/SourceRegistry.cpp @@ -218,8 +218,17 @@ SourceRegistry::Result SourceRegistry::retireProcessEpoch(const std::string& pro // current, which does not apply to a wall. SKIP it, do not mark it // Departed: that would flip its nullopt fields to concrete false/Departed, // the exact false claim this task removed from install(). A wall's - // lifetime is scene-reference, released by a separate sweep (later task) - - // do not "fix" this skip into a mark. + // lifetime is scene-reference, released by removeComposed() (the caller's + // own liveness sweep, not this one) - do not "fix" this skip into a mark. + // A tombstone would also be a dead end here: setAvailability() refuses + // Composed outright (it cannot flip subscriptionObserved's nullopt to a + // concrete false), so there is no legal way to mark one Departed even if + // this loop tried - and because a composed entry's availability stays + // nullopt forever, externalConflict()'s `availability != Departed` test + // reads true for it PERMANENTLY, meaning a tombstoned wall id could never + // be reused by add() again. Erasing outright (removeComposed) is not a + // simplification of tombstoning - it is the only mechanism that actually + // frees the id. if (source.kind == Kind::Composed) continue; source.availability = Availability::Departed; source.subscriptionRequested = false; diff --git a/native/src/core/SourceRegistry.h b/native/src/core/SourceRegistry.h index 982724f2..54485aa0 100644 --- a/native/src/core/SourceRegistry.h +++ b/native/src/core/SourceRegistry.h @@ -76,7 +76,11 @@ class SourceRegistry final { uint64_t revision = 0; uint64_t decisionRevision = 0; // Binding/readiness changes, independent of frame traffic. std::vector persons; - std::vector sources; // Stable SourceId order, includes departure tombstones. + // Stable SourceId order, includes departure tombstones for every non-Composed + // kind (Availability::Departed, kept for diagnostics). A Composed source is + // the exception: it never tombstones, it VANISHES via removeComposed() - + // see that method's comment for why a wall cannot be tombstoned at all. + std::vector sources; }; struct Mutation { Result result; std::optional token; }; @@ -104,6 +108,11 @@ class SourceRegistry final { // make add() answer Conflict forever for a wall id that is free to reuse. // Refuses (Invalid) for any other kind: erasing a real source's record is // the false-erasure this registry exists to prevent from the other direction. + // Takes a bare SourceId, deliberately not a Token: the caller must be the + // SOLE owner of a composed source's lifetime. replace() exists in this class + // precisely so an old callback cannot retire a new instance it no longer + // owns (compare-and-replace against the expected Token) - removal has no + // such fence, so it must never grow a second writer. Result removeComposed(const SourceId& sourceId); Result publish(const Token& token, uint64_t sequence, int64_t observedNs, Format format); [[nodiscard]] std::shared_ptr snapshot() const; diff --git a/native/tests/TilesRenderPlanTest.cpp b/native/tests/TilesRenderPlanTest.cpp index f710baf3..c3bf11b7 100644 --- a/native/tests/TilesRenderPlanTest.cpp +++ b/native/tests/TilesRenderPlanTest.cpp @@ -1519,22 +1519,40 @@ TEST(TilesRenderPlan, AWallReleasedAndReCuedRegistersAsANewSource) { << "a re-cued wall is a NEW registry entry, not the old one come back"; } -// The idempotence guard (registeredWallIds_) exists so a live wall's -// steady-state tick never touches the registry mutex again once registered — -// but the observable contract this test pins is simpler and still real: -// however many render ticks a wall stays live, the registry holds exactly -// ONE entry for it, never zero (a lost registration) and never more than one. -TEST(TilesRenderPlan, ALiveWallHoldsExactlyOneRegistryEntryAcrossManyTicks) { +// Review round 1, Finding 1: a plain "exactly one entry" count is near- +// unfalsifiable here. `sources_` is a std::map keyed by sourceId, so ONE key +// can never hold two entries by construction, and "never zero" is already +// covered by ALiveWallRegistersAsAComposedSourceInTheRegistry above. Worse: +// delete registeredWallIds_ entirely and add() answers Conflict on every +// tick (taking the registry mutex 60x/s on the render path) while this count +// assertion STILL passes, because a refused add() neither creates a second +// entry nor bumps any counter this test reads. +// +// The falsifiable property is identity, not count: install() mints a fresh +// instanceId as `epoch + ":" + revision` on every real add() call, so the +// realistic regression this guards against — someone drops the guard and +// instead removes-and-re-adds every tick — mints a NEW instanceId every +// frame, which this assertion catches and a count assertion cannot. +TEST(TilesRenderPlan, ALiveWallKeepsTheSameRegistryIdentityAcrossManyTicks) { MediaCore core; loadWall(core, {"zoom:1"}); + const auto firstSnapshot = core.sourceRegistrySnapshotForTest(); + const auto* first = findRegisteredSource(firstSnapshot, "tiles:s"); + ASSERT_NE(first, nullptr); + const auto firstInstanceId = first->token.instanceId.value; + for (int tick = 0; tick < 25; ++tick) { (void)core.applyCommands(corevideo::rpc::Json::Array{}); } - const auto snapshot = core.sourceRegistrySnapshotForTest(); - ASSERT_NE(snapshot, nullptr); - const auto count = std::count_if(snapshot->sources.begin(), snapshot->sources.end(), + const auto laterSnapshot = core.sourceRegistrySnapshotForTest(); + const auto count = std::count_if(laterSnapshot->sources.begin(), laterSnapshot->sources.end(), [](const auto& source) { return source.token.sourceId.value == "tiles:s"; }); - EXPECT_EQ(count, 1); + ASSERT_EQ(count, 1) << "never zero (a lost registration) across a live wall's steady state"; + const auto* later = findRegisteredSource(laterSnapshot, "tiles:s"); + ASSERT_NE(later, nullptr); + EXPECT_EQ(later->token.instanceId.value, firstInstanceId) + << "a live wall's registration must be the SAME entry across ticks, " + << "never removed-and-re-added (which would mint a fresh instanceId)"; } From 646d8d1c4353973e1a20dbbd37ea70f6b3879c2e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 23:20:24 -0400 Subject: [PATCH 17/20] test(rig): make the fake engine's motion measurable by the soak's luma gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `live-meeting-soak.mjs` gates "did the picture move" on frame-to-frame YAVG exceeding 0.05, because 8995 frames of flat luma once passed every validator that counted frames. The fake engine's motion was a ONE-ROW band that brightens row N and restores row N-1, which changes the frame mean by ~0.0007 — so a rig delivering a measured 60.0/s scored 0% of frames moving, and the gate could never pass on the only engine that can drive this soak headlessly. A gate no harness can satisfy is unfalsifiable, which is worse than no gate. Pulse a BLOCK instead: h/8 rows stepping 7 luma per frame is ~0.9 YAVG at the frame level, 18x the threshold, at 1/8 the cost of the full repaint this function deliberately avoids (that repaint made the PRODUCER the bottleneck at ~22fps/source). Offset by participant id so a wall's tiles carry different values while all advancing together. Measured after: 5 minutes, 20 takes, 8 sources — movingFrameRatio 0.0 -> 0.998, LIVE-MEETING SOAK PASS, with the take property intact (20 cut, 0 rebuilt) and 60.0/s render, 0 deadline misses, 0 dropped video. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- native/zoom-engine/fake/fake-engine.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/native/zoom-engine/fake/fake-engine.cpp b/native/zoom-engine/fake/fake-engine.cpp index 7c1fa3fb..e1d810f0 100644 --- a/native/zoom-engine/fake/fake-engine.cpp +++ b/native/zoom-engine/fake/fake-engine.cpp @@ -429,6 +429,24 @@ static void produce_frame_locked(Target& t, uint64_t tick) { t.luma.data() + static_cast(prev) * w, w); std::memset(yp + static_cast(band) * w, 235, w); + // ...but a ONE-ROW band is not measurable MOTION. Any judge that asks "did + // the picture move" reads the frame's MEAN luma, and a band that brightens + // one row of h while restoring the row behind it changes that mean by + // ~0.0007 - so `live-meeting-soak.mjs`'s motion gate (>0.05 YAVG of + // frame-to-frame change, which exists because 8995 frames of FLAT luma once + // passed every validator that counted frames) scored 0% of frames moving on + // a rig that was in fact delivering 60fps. A harness that cannot satisfy a + // gate makes that gate unfalsifiable, which is worse than not having it. + // So pulse a BLOCK: h/8 rows stepping 7 luma per frame is ~0.9 YAVG at the + // frame level, 18x the threshold, for 1/8 of a full repaint (the cost this + // function deliberately avoids - see the comment above). Offset by pid so + // the tiles of one wall carry different values while all advancing together. + const uint32_t pulseRows = h / 8; + if (pulseRows > 0) { + const auto pulse = static_cast(40 + (tick * 7 + pid) % 180); + std::memset(yp, pulse, static_cast(pulseRows) * w); + } + // A/V clap: one full-white frame. Restored on the very next frame, so the // event is exactly one frame long and unambiguous to find in the recording. if (t.clapRestore) { From c98da242a7406d4c99bb61b9ab672bd612e053ca Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 23:30:15 -0400 Subject: [PATCH 18/20] fix(#448): refuse setSubscription on a composed source, bound the wall-registration retry, correct the defect narrative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final-review findings on the #448 slice, plus the documentation defect the review surfaced. setSubscription refused Kind::Composed nowhere, while CLAUDE.md asserted it did. An observed:true call was already refused as a side effect (a nullopt availability is not Available), but requested:true with observed nullopt applied cleanly and turned a NOT-APPLICABLE field into a concrete claim — the exact lie the nullopt fields exist to prevent. Nothing in the tree calls it for a wall, so only the docs were wrong, which is how an invariant rots. Red-verified first. The wall registration loop retried a PERMANENTLY invalid id forever. A failed add is deliberately not remembered as registered so transient failures retry on the next liveness transition — but an id refused on its SPELLING can never become valid, so that retry became a registry-mutex acquisition plus an unbounded log line on every render tick. SourceRegistry::kMaxIdBytes is now public (one declared bound, used by validRegistration) so a caller can tell the two cases apart; unregisterableWallIds_ skips the permanent ones once and loudly, and warnedWallRegistrationIds_ bounds the retryable failures' line to once per id while keeping the retry. Both are pruned on the same liveness rule as registeredWallIds_. THE DEFECT NARRATIVE WAS BACKWARDS in CLAUDE.md, the spec, the plan and the headline test's own comment: a reset does not "replay from alpha 0". TilesAnimator treats a reset animator's next non-empty sample() as an ADOPTION, so the wall SNAPS TO ITS FINAL STATE — alpha pops to 1, mid-spring rects jump to settled. The consequence is not cosmetic: EXPECT_GE(alpha_after, alpha_before) is satisfied by a snap just as well as by continuity and catches NOTHING (the first draft of that test passed against the unfixed code). The falsifying assertions are the ones bounding the other side — post-take alpha below 0.9, and a tile already at opacity 1 keeping its mid-spring rect — and those are what was verified red. Documented at all four sites, with the honest limit that the generation-equality check is not independently falsified by that revert. Also: CLAUDE.md claimed build-dev is a single-config generator. It is Visual Studio 18 2026 with CMAKE_RUNTIME_OUTPUT_DIRECTORY pinned for every config, so Debug and Release write the same exe path and a missing --config Release silently installs a Debug core. That cost a false regression report on 2026-09-12 (coreMutex 1% -> 81%); the tell was uniform inflation across trivial stages and an 8,322,560-byte binary against Release's 2,168,832. Documented with the check. Plus: the registry section now says it has a production writer and no production reader yet, and the shared-wall applyLatest coupling is commented at both multiview cell sites and filed to plan 2. Tests: 978 passed, 0 failed on the Release core. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- CLAUDE.md | 102 +++++++++++++----- .../2026-09-12-tiles-wall-one-animator.md | 20 ++-- ...-12-tiles-wall-persistent-source-design.md | 15 +++ native/src/core/MediaCore.cpp | 51 ++++++++- native/src/core/MediaCore.h | 11 ++ native/src/core/SourceRegistry.cpp | 15 ++- native/src/core/SourceRegistry.h | 7 ++ native/tests/SourceRegistryComposedTest.cpp | 26 +++++ native/tests/TilesRenderPlanTest.cpp | 31 ++++-- 9 files changed, 234 insertions(+), 44 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2d5fdb8c..43b6e516 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -107,14 +107,25 @@ dotnet build native-shell/CoreVideoPro.WinUI/CoreVideoPro.WinUI.csproj -c Releas runs `scripts/app.ps1`; the dev launcher is `scripts/run-studio.ps1` (now respects a pre-set `COREVIDEO_ZOOM_ENGINE_PATH`). -**Run the binary the build just wrote.** `native/build-dev/` is a single-config -generator — the current binaries are `native/build-dev/corevideo-native.exe` and -`corevideo-native-tests.exe`. A `native/build-dev/Release/` directory also exists, -left by an older VS-generator build, and **nothing updates it**: a test run from -there reported a confident "380 tests passed" from a binary a MONTH old, which -silently omitted every test file added since. The real suite is 529 tests. If a -newly added test does not appear in the output, check which binary you ran before -suspecting CMake. +**Run the binary the build just wrote, and ALWAYS pass `--config Release`.** +`native/build-dev/` is a **MULTI-CONFIG** generator (`CMAKE_GENERATOR: Visual +Studio 18 2026`) whose `CMAKE_RUNTIME_OUTPUT_DIRECTORY` is pinned to the binary +dir for EVERY config (`native/CMakeLists.txt:46-48`). So the exes have no +per-config suffix: **Debug and Release write to the exact same path**, +`native/build-dev/corevideo-native.exe`, and `cmake --build native/build-dev +--target …` with no `--config` silently builds DEBUG over your Release core. +This cost a full false regression on 2026-09-12 — a drill reported `coreMutex` +over-budget 1% -> 81% and was reported to the owner as a real regression caused +by the branch. The tell was uniform inflation across trivial stages (emit 32x, +plan 19x) and the binary SIZE: 8,322,560 bytes Debug vs 2,168,832 Release. With +`--config Release` every metric matched baseline and the drill passed. **Check +the size, or `--config`, before believing any native perf number.** (Libraries +DO get a per-config dir — `build-dev/Release/corevideo_native.lib` — so a +`Release/` subdirectory existing proves nothing about the exes.) Separately, a +test run from a STALE `build-dev/Release/*.exe` left by an older layout once +reported a confident "380 tests passed" from a binary a MONTH old, silently +omitting every test file added since. If a newly added test does not appear in +the output, check which binary you ran before suspecting CMake. Logs: `%LOCALAPPDATA%\CoreVideoPro\launch.log` (WinUI) and `media-core.log` (core). Support bundle (Diagnostics → "Export support bundle"): writes redacted JSON **and a @@ -852,20 +863,30 @@ comment at the code site; this is the index. what I can't have is a total rerender from what is in preview to program like it is loading for the first time." The wall key is `sceneId + ":" + layerId` and the layer id is derived from the scene id, so the SAME gallery has the - SAME key on both buses — `MediaCore` holds two animation objects - (`programTilesAnimation_` / `previewTilesAnimation_`) and the program one used - to reset its animator the moment the key it had never held arrived. Two - corrections, both in `compositor/TilesPlanAnimation.h`: - `adoptSettledFrom()` MOVES a settled wall's state from preview to program on - the take tick (exact key match + every sampled tile `atRest` only; the source - is reset, never aliased, so the next wall cued in preview starts clean), and - `advance()` no longer samples an EMPTY target set for a wall that is still - present and has already drawn tiles. That second one is what actually produced - the reported replay: an all-stale beat (`kTilesStaleFrameMs`, an ordinary - state — see the empty-plan rule above) erased every retained tile AND consumed - the animator's adoption, so the instant frames returned the whole wall faded in - from alpha 0. A COLD wall's first tick is untouched, so a wall that was never - in preview behaves exactly as before. Not a contributor, measured: preview and + SAME key on both buses. **This was first fixed with a HAND-OVER and is now + fixed STRUCTURALLY — the hand-over is DELETED. See "ONE ANIMATOR PER WALL" + below; `TilesPlanAnimation::adoptSettledFrom` no longer exists.** The original + shape: `MediaCore` held two animation objects + (`programTilesAnimation_` / `previewTilesAnimation_`) and the program one reset + its animator the moment the key it had never held arrived. `adoptSettledFrom()` + moved a SETTLED wall's state across on the take tick — settled only, because + with two animators mid-flight state had no correct owner. The second + correction survives and still matters: `advance()` does not sample an EMPTY + target set for a wall that is still present and has already drawn tiles. That + is what produced the reported rebuild — an all-stale beat + (`kTilesStaleFrameMs`, an ordinary state — see the empty-plan rule above) + erased every retained tile AND consumed the animator's adoption. + **What a reset actually looks like, because the direction is counter-intuitive + and the docs had it backwards: it does NOT replay from alpha 0. `TilesAnimator` + treats a reset animator's next non-empty `sample()` as an ADOPTION (content + already present, not entering), so the wall SNAPS TO ITS FINAL STATE** — alpha + pops to 1, mid-spring rects jump to their settled positions. On air that is a + wall that stops moving and jumps, which is what "loading for the first time" + looked like. The practical consequence for tests: `EXPECT_GE(after, before)` on + alpha is satisfied by a snap just as well as by continuity and therefore + catches NOTHING — a falsifying assertion has to bound the other side (alpha + stays below 0.9, rects stay near their mid-spring values). A COLD wall's first + tick is untouched, so a wall that was never in preview behaves as before. Not a contributor, measured: preview and program share one device and one `sourceTextures_` cache keyed by `participantId` (`D3D11CompositorAdapter`), so tile textures are already warm across a take. Tests: `TilesRenderPlan.AWallSettledInPreviewIsAlreadySettledOnItsFirstProgramFrame`, @@ -1249,7 +1270,11 @@ measurement rather than from the product. ONE sync, so that IS the take on this wire) and completed on the first program render tick after it — the only place the "after" half exists. Carries scene id and renderPlanId on both sides, the layer ids on both sides, the wall keys, - whether `TilesPlanAnimation::adoptSettledFrom` **adopted or reset**, whether the + whether the wall was **adopted or reset** (since #448: whether the ONE + `core::TilesWallSource` for that wall id survived the take with its generation + intact — `wallAdoptedSettled` is computed as `wallExistedBefore && + generationAfter == generationBefore`, not from the deleted + `TilesPlanAnimation::adoptSettledFrom`), whether the wall's live background (`tiles-source-bg:`) made the first program frame, and the subscription-churn delta across the take. `core/TakeRecordPolicy.h` turns those into the one-word answer to "did the wall rebuild or cut" — and it will @@ -1327,6 +1352,16 @@ red/green, the policies, and the take record end to end), ## The Tiles wall is a composed source, and composed sources are ERASED not tombstoned (#448 slice 2 task 4, 2026-09-12) +**It has a production WRITER and, as of this slice, no production READER.** +`MediaCore::renderSyntheticTick` registers and releases walls for real, and the +only consumers are tests (`sourceRegistrySnapshotForTest`). That is deliberate — +per `docs/BACKLOG.md`, a #419 foundation lands on `main` only together with a real +consumer, and wall registration IS that consumer for the registry's write side — +but it means nothing in the product yet behaves differently because of these +entries. Do not describe the registry as "wired" beyond that, and expect the +first real reader (the multiview PVW cell, plan 2) to be where its snapshot shape +gets its first genuine test. + `SourceRegistry` (`native/src/core/SourceRegistry.h`, carved out of #419 unwired onto main) gained `Kind::Composed` for sources the CORE renders rather than captures — the Tiles wall is the first one. `MediaCore::renderSyntheticTick` @@ -1347,7 +1382,26 @@ state.** `personId`, `externalId`, `availability`, `subscriptionRequested`, `subscriptionObserved` are all `nullopt` for it — `nullopt` means NOT APPLICABLE, never false — because a wall has no provider process and nothing ever subscribes to it. `setAvailability`/`setSubscription` refuse `Composed` -outright for exactly this reason. +outright for exactly this reason — `setSubscription`'s refusal was MISSING and +this file asserted it anyway for a day (final-review finding, fixed with +`SourceRegistryComposed.SetSubscriptionOnAWallIsRefusedOutright`): an +`observed:true` call was already refused as a side effect, because a nullopt +availability is not `Available`, but `requested:true, observed:nullopt` applied +cleanly and turned a NOT-APPLICABLE field into a concrete claim. Nothing in the +tree called it for a wall, so only the documentation was wrong — which is exactly +how an invariant rots. + +**A wall id too long to register is SKIPPED, not retried** (same finding). +`SourceRegistry::kMaxIdBytes` (512) is the one declared bound on every id-shaped +field, and it is public precisely so a caller can tell a PERMANENTLY refusable id +from a transiently refused one: the registration loop deliberately does not +remember a failed add as registered (so a transient failure retries on the next +liveness transition), which turned a spelling-based refusal into a registry-mutex +acquisition plus a log line on EVERY render tick. `unregisterableWallIds_` skips +those once and loudly; `warnedWallRegistrationIds_` bounds the retryable +failures' log line to once per id while keeping the retry. Both are pruned on the +same liveness rule as `registeredWallIds_`, or a wall re-cued under a corrected +id would stay skipped or silent for the life of the process. **A composed source is ERASED (`SourceRegistry::removeComposed`), never tombstoned — and this is not a simplification, it is the only mechanism that diff --git a/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md b/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md index 2506d93e..5db8c24e 100644 --- a/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md +++ b/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md @@ -466,8 +466,16 @@ Add to `native/tests/TilesRenderPlanTest.cpp`: ```cpp // #448. adoptSettledFrom refuses a wall whose tiles are still flying, because // with two animators mid-flight state has no correct owner. So a wall taken -// MID-ANIMATION re-animated from alpha 0 on Program. With one animator per wall -// there is nothing to hand over and the cut is continuous. +// MID-ANIMATION lost its animation on Program. With one animator per wall there +// is nothing to hand over and the cut is continuous. +// +// CORRECTION (post-implementation): a reset does NOT replay from alpha 0. The +// animator treats a reset's next non-empty sample() as an ADOPTION, so the wall +// SNAPS TO ITS FINAL STATE - alpha pops to 1, mid-spring rects jump to settled. +// Therefore the EXPECT_GE below is NOT a regression test (a snap satisfies it); +// see the as-built test in TilesRenderPlanTest.cpp, which additionally asserts +// the post-take alpha stays BELOW 0.9 and that any tile already at opacity 1 +// keeps its mid-spring rect. Those are the assertions verified red. // // This test MUST FAIL before Task 3. Verify that by reverting, not by assuming. TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { @@ -489,14 +497,14 @@ TEST(TilesRenderPlan, AWallTakenMidAnimationIsContinuous) { // The wall did not restart... EXPECT_EQ(harness.wallGeneration("tiles:scene-a"), generationBefore); - // ...and its tiles continued from where they were, rather than snapping back - // to the entry state. Alpha is the sharpest signal: a replayed entrance - // restarts at 0. + // ...and its tiles continued from where they were, rather than SNAPPING + // FORWARD to the settled state (see the correction above - this direction is + // the opposite of what the first draft of this plan assumed). const auto afterTake = harness.sampledProgramTiles(); ASSERT_EQ(afterTake.size(), midFlight.size()); for (size_t i = 0; i < afterTake.size(); ++i) { EXPECT_GE(afterTake[i].alpha, midFlight[i].alpha) - << "tile " << i << " replayed its entrance instead of continuing"; + << "tile " << i << " lost its in-flight animation instead of continuing"; } } ``` diff --git a/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md index d20e3c78..a5f4b126 100644 --- a/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md +++ b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md @@ -12,6 +12,21 @@ which #419 foundations slices 2-3 build on. A Tiles wall cued in Preview and taken to Program re-animates on the cut. The owner's words, live show 2026-09-09: +> **CORRECTION (post-implementation, 2026-09-12).** Every "re-animates" / +> "replays from alpha 0" reading below describes the SYMPTOM in the wrong +> direction, and it matters to the tests this spec implies. A reset animator does +> not replay an entrance: `TilesAnimator` treats a reset animator's next +> non-empty `sample()` as an ADOPTION — content already present, not entering — +> so the wall **SNAPS TO ITS FINAL STATE**: alpha pops to 1 and mid-spring rects +> jump to their settled positions. On air this reads as a wall that stops moving +> and jumps, which is what the owner saw as "loading for the first time". The +> consequence: an `EXPECT_GE(alpha_after, alpha_before)` assertion is satisfied by +> a snap just as well as by continuity and catches NOTHING; a falsifying test has +> to bound the other side (post-take alpha stays below 0.9 for a tile that was +> mid-ramp, and a tile already at opacity 1 keeps its mid-spring rect). +> Read "re-animates" throughout as "loses its in-flight animation". + + > I am ok if panelists leave and join the video but what I can't have is a total > rerender from what is in preview to program like it is loading for the first > time. diff --git a/native/src/core/MediaCore.cpp b/native/src/core/MediaCore.cpp index 931fbd5a..cccc63b0 100644 --- a/native/src/core/MediaCore.cpp +++ b/native/src/core/MediaCore.cpp @@ -3639,6 +3639,18 @@ modules::CompositorRenderPlan MediaCore::buildMultiviewRenderPlan(const std::vec // This is a read-only path (buildMultiviewRenderPlan is const): use find(), // never forWall(), which would insert a wall on a mere read. A wall with no // TilesWallSource yet has never animated, so there is nothing to apply. + // SHARED-WALL COUPLING, deliberate and scoped (plan 2, the wall-texture + // slice, is where it goes away). One wall id now has ONE animator shared by + // both buses, so when the same gallery is cued in Preview and live on + // Program these two cells read the SAME animation state — the PVW cell shows + // Program's geometry for that wall, not an independent Preview animation. + // That is the correct trade today and the whole point of #448: the operator's + // complaint was the wall REBUILDING across the cut, and one shared animator + // is what makes the cut continuous. It is only visible at all while a wall + // is mid-flight on both buses at once, and in that window Program is the + // authority on what the wall looks like (see `enabled = programEnabled`). + // applyLatest is a pure read of the settled state and cannot itself advance + // or reset anything, so neither cell can corrupt the other's wall. if (const auto* wall = tilesWallSources_.find(tilesLayer_.layerId)) { wall->applyLatest(programPlan, tilesLayer_.layerId); } @@ -3677,6 +3689,11 @@ modules::CompositorRenderPlan MediaCore::buildMultiviewRenderPlan(const std::vec // never reflected the preview and never swapped on Take.) if (hasPreviewScene()) { auto previewPlan = buildPreviewCompositorRenderPlan(videoFrames); + // Same shared-wall coupling as the PGM cell above: when Preview and + // Program carry the SAME wall id this reads the one shared animator, so + // the PVW cell mirrors Program's wall geometry rather than animating on + // its own. Pure read; it cannot advance or reset the wall. Owned by plan 2 + // (the wall-texture slice), where the PVW cell gets the wall's own texture. if (const auto* wall = tilesWallSources_.find(previewTilesLayer_.layerId)) { wall->applyLatest(previewPlan, previewTilesLayer_.layerId); } @@ -6390,6 +6407,21 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // skip it here rather than registering a nameless source that // validRegistration would refuse as Invalid anyway. if (wallId.empty()) continue; + // Final-review finding: an id validRegistration refuses on its SPELLING can + // never become valid, yet a failed add is deliberately NOT remembered as + // registered (below) - so the retry that exists for transient failures + // turned a permanent refusal into a registry-mutex acquisition plus a log + // line on every single render tick. Skip those, once and loudly, against the + // registry's own declared bound rather than a second copy of the number. + if (unregisterableWallIds_.contains(wallId)) continue; + if (wallId.size() > SourceRegistry::kMaxIdBytes) { + unregisterableWallIds_.insert(wallId); + ::corevideo::core::nativeLogf( + "[source-registry] wall id of %zu bytes exceeds the %zu-byte limit and can never " + "register; not retrying\n", + wallId.size(), static_cast(SourceRegistry::kMaxIdBytes)); + continue; + } // Review round 1, Finding 3: check-then-insert, not insert-and-read-.second. // unordered_set::insert on an already-present key is not guaranteed // allocation-free on every implementation (MSVC's has historically built @@ -6412,8 +6444,12 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim // transition, not silently abandoned for the rest of its life. Loud, // since a wall invisible to the registry is invisible to any future // registry consumer too. - ::corevideo::core::nativeLogf("[source-registry] wall '%s' failed to register (result=%d)\n", - wallId.c_str(), static_cast(mutation.result)); + // RETRYABLE (Conflict / Exhausted / a retired epoch): the retry above + // stands, but say it ONCE per id - see warnedWallRegistrationIds_. + if (warnedWallRegistrationIds_.insert(wallId).second) { + ::corevideo::core::nativeLogf("[source-registry] wall '%s' failed to register (result=%d)\n", + wallId.c_str(), static_cast(mutation.result)); + } } } // Release from the registry whatever the sweep above just released from @@ -6430,6 +6466,17 @@ void MediaCore::renderSyntheticTick(bool videoOnly, int64_t mediaPresentationTim sourceRegistry_.removeComposed(SourceId{*it}); it = registeredWallIds_.erase(it); } + // The two failure-side guards are pruned on the SAME rule, or a wall refused + // once and then legitimately re-cued under a corrected id would stay skipped + // (unregisterableWallIds_) or silent (warnedWallRegistrationIds_) for the life + // of the process. This is also what bounds both sets by the live wall set. + for (auto* guard : {&unregisterableWallIds_, &warnedWallRegistrationIds_}) { + for (auto it = guard->begin(); it != guard->end();) { + it = std::find(liveWallIds.begin(), liveWallIds.end(), *it) == liveWallIds.end() + ? guard->erase(it) + : std::next(it); + } + } // Task 4: cache the plan the render tick actually built — lastRenderPlanForTest() // and the sessionState() `tiles` node both read THIS, so a consumer can never // observe a wall the compositor did not also receive (see modules_.compositor-> diff --git a/native/src/core/MediaCore.h b/native/src/core/MediaCore.h index 1a09e5ca..7af1ff46 100644 --- a/native/src/core/MediaCore.h +++ b/native/src/core/MediaCore.h @@ -553,6 +553,17 @@ class MediaCore { static constexpr const char* kCoreProcessEpoch = "core-process"; core::SourceRegistry sourceRegistry_{"core-registry"}; std::unordered_set registeredWallIds_; + // Wall ids SourceRegistry refuses on their SPELLING (over kMaxIdBytes), which + // no retry can ever change. Skipped outright: a failed add is deliberately not + // remembered as registered, so without this a permanently-Invalid id would + // take the registry mutex and log on EVERY render tick. + std::unordered_set unregisterableWallIds_; + // Wall ids whose RETRYABLE registration failure has already been logged once. + // The retry itself is kept (it is a liveness-transition cost); only the line + // is bounded, because an unbounded render-tick line rolls the diagnosis out + // of the bounded log it exists to land in. Both sets are pruned with + // registeredWallIds_ when a wall stops being live. + std::unordered_set warnedWallRegistrationIds_; // Task 4: per-member frame-age snapshot for the wall expansion, refreshed // every render tick from the live videoFrames gather (renderSyntheticTick, // under coreMutex — geometry bookkeeping, not pixel work). Covers members of diff --git a/native/src/core/SourceRegistry.cpp b/native/src/core/SourceRegistry.cpp index 1d3a0891..56904c81 100644 --- a/native/src/core/SourceRegistry.cpp +++ b/native/src/core/SourceRegistry.cpp @@ -54,12 +54,12 @@ bool SourceRegistry::validRegistration(const Registration& r) const { // so requiring an externalId would reject every wall outright. const bool externalIdOk = composed ? r.externalId.empty() - : (!r.externalId.empty() && r.externalId.size() <= 512); + : (!r.externalId.empty() && r.externalId.size() <= kMaxIdBytes); return knownKind && externalIdOk && (!r.requestedGeneration || (*r.requestedGeneration > 0 && *r.requestedGeneration <= kMaxRevision)) && - (!r.instanceId || (!r.instanceId->value.empty() && r.instanceId->value.size() <= 512)) && - !r.sourceId.value.empty() && r.sourceId.value.size() <= 512 && - !r.processEpoch.empty() && r.processEpoch.size() <= 512 && + (!r.instanceId || (!r.instanceId->value.empty() && r.instanceId->value.size() <= kMaxIdBytes)) && + !r.sourceId.value.empty() && r.sourceId.value.size() <= kMaxIdBytes && + !r.processEpoch.empty() && r.processEpoch.size() <= kMaxIdBytes && !retiredProcessEpochs_.contains(r.processEpoch) && r.displayName.size() <= 4096 && ((!r.personId && r.personGeneration == 0) || @@ -190,6 +190,13 @@ SourceRegistry::Result SourceRegistry::setSubscription(const Token& token, bool if (found == sources_.end()) return Result::NotFound; auto& source = found->second; if (!sameToken(source.token, token) || source.availability == Availability::Departed) return Result::Stale; + // Symmetric with setAvailability above: NOTHING subscribes to a composed + // source - it has no provider process and no SDK handle - so it may never + // carry a subscription state at all. The `observed` clause below already + // refuses observed:true for one (a nullopt availability is not Available), + // but requested:true with observed nullopt applied cleanly and turned a + // NOT-APPLICABLE field into a concrete claim. + if (source.kind == Kind::Composed) return Result::Invalid; if (observed.value_or(false) && source.availability != Availability::Available) return Result::Invalid; if (source.subscriptionRequested == requested && source.subscriptionObserved == observed) return Result::Unchanged; diff --git a/native/src/core/SourceRegistry.h b/native/src/core/SourceRegistry.h index 54485aa0..a0d59be1 100644 --- a/native/src/core/SourceRegistry.h +++ b/native/src/core/SourceRegistry.h @@ -119,6 +119,13 @@ class SourceRegistry final { // Discovery only: zero/multiple matches remain explicit, with no auto-binding. [[nodiscard]] std::vector peopleNamed(const std::string& displayName) const; + // The one declared bound on every id-shaped field (sourceId, externalId, + // instanceId, processEpoch). Public because a CALLER cannot otherwise tell a + // permanently-refusable id from a transiently-refused one: MediaCore's wall + // registration loop retries failures on every liveness transition, and an + // over-length id would be retried forever. + static constexpr std::size_t kMaxIdBytes = 512; + private: static bool sameToken(const Token& left, const Token& right); bool validRegistration(const Registration& registration) const; diff --git a/native/tests/SourceRegistryComposedTest.cpp b/native/tests/SourceRegistryComposedTest.cpp index d814d62a..71e7d7cd 100644 --- a/native/tests/SourceRegistryComposedTest.cpp +++ b/native/tests/SourceRegistryComposedTest.cpp @@ -176,6 +176,32 @@ TEST(SourceRegistryComposed, RemoveComposedRefusesANonComposedSource) { EXPECT_EQ(registry.snapshot()->sources.size(), 1U); } +// setAvailability refuses Composed; setSubscription did NOT, and nothing in the +// tree noticed because the only writer never calls it for a wall. The honesty +// rule is symmetric: nothing subscribes to a wall, so a wall may never carry a +// subscription state. An `observed:true` call was already refused as a side +// effect (nullopt availability is not Available), but `requested:true` with +// observed nullopt applied cleanly and turned a NOT-APPLICABLE field into a +// concrete claim - the exact lie the nullopt fields exist to prevent. +TEST(SourceRegistryComposed, SetSubscriptionOnAWallIsRefusedOutright) { + SourceRegistry registry("registry-epoch-1"); + const auto mutation = registry.add(wallRegistration("tiles:scene-a")); + ASSERT_TRUE(mutation.token.has_value()); + + EXPECT_EQ(registry.setSubscription(*mutation.token, true, std::nullopt), + SourceRegistry::Result::Invalid); + EXPECT_EQ(registry.setSubscription(*mutation.token, true, true), + SourceRegistry::Result::Invalid); + EXPECT_EQ(registry.setSubscription(*mutation.token, false, false), + SourceRegistry::Result::Invalid); + + const auto snapshot = registry.snapshot(); + ASSERT_EQ(snapshot->sources.size(), 1u); + const auto& wall = snapshot->sources.front(); + EXPECT_FALSE(wall.subscriptionRequested.has_value()); + EXPECT_FALSE(wall.subscriptionObserved.has_value()); +} + TEST(SourceRegistryComposed, RemoveComposedOnAnUnknownIdIsNotFound) { SourceRegistry registry("registry-epoch-1"); EXPECT_EQ(registry.removeComposed(corevideo::core::SourceId{"tiles:never-added"}), diff --git a/native/tests/TilesRenderPlanTest.cpp b/native/tests/TilesRenderPlanTest.cpp index c3bf11b7..05e452b0 100644 --- a/native/tests/TilesRenderPlanTest.cpp +++ b/native/tests/TilesRenderPlanTest.cpp @@ -1047,15 +1047,30 @@ void take(MediaCore& core, const corevideo::rpc::Json& program, const corevideo: // #448. `TilesPlanAnimation::adoptSettledFrom` refused a wall whose tiles were // still flying, because with two per-bus animators mid-flight state had no // correct owner ("mid-flight state belongs to the bus that is flying it"). -// So a wall taken MID-ANIMATION re-animated from alpha 0 on Program — the -// live-show defect this file is named for, caught here at its actual root: -// with one animator PER WALL (core::TilesWallSource, shared by both buses) -// there is nothing to hand over, so the cut is continuous even mid-flight. +// So a wall taken MID-ANIMATION lost its motion on Program — the live-show +// defect this file is named for, caught here at its actual root: with one +// animator PER WALL (core::TilesWallSource, shared by both buses) there is +// nothing to hand over, so the cut is continuous even mid-flight. // -// This test MUST FAIL before Task 3. Verified by reverting the MediaCore.cpp/ -// TilesPlanAnimation.h changes (not assumed): with the old per-bus pair, this -// wall's key had never been held by programTilesAnimation_, so it reset and -// EXPECT_GE(onAir alpha, midFlight alpha) failed as the wall replayed from 0. +// WHAT THE OLD MECHANISM ACTUALLY DID, because the direction matters to every +// assertion below: a reset does NOT replay from alpha 0. `TilesAnimator` +// treats a reset animator's next non-empty sample() as an ADOPTION — content +// already present, not entering — so the wall SNAPS STRAIGHT TO ITS FINAL +// STATE: alpha pops to 1 and mid-spring rects jump to their settled positions. +// That is why `EXPECT_GE(onAir alpha, midFlight alpha)` alone is NOT a +// regression test — a snap to 1 satisfies it just as well as continuity does, +// and the first draft of this test passed against the unfixed code. The +// falsifying assertions are the ones that bound the OTHER side: the post-take +// alpha of a tile that was mid-ramp must stay BELOW 0.9, and any tile already +// at opacity 1 must keep its mid-spring RECT (EXPECT_NEAR, 0.05). Both were +// verified red by reverting the MediaCore.cpp / TilesPlanAnimation.h changes, +// not assumed. +// +// One honest limit on that revert: the generation-equality check is not +// independently falsified by it, because the reverted take path never touches +// tilesWallSources_ at all — it reads 0 == 0 either way. The alpha and rect +// assertions are the proven-red ones; the generation assertion holds forward, +// by construction of the new API. // // Getting a wall genuinely MID-FLIGHT deterministically (no real-time // polling): TilesAnimator treats an animator's truly first-ever sample() call From 71e4d4dce831ef2defb9ab0c07d1974d20613122 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 23:42:05 -0400 Subject: [PATCH 19/20] docs: close the #448 slice-2 ledger, file the residual minors, update the backlog The final gates re-run on the post-fix binary (978 tests, SHOW DRILL PASSED, LIVE-MEETING SOAK PASS at 20 takes / 0 rebuilt), the soak motion gate's unfalsifiability and its ruling, the environmental cause of the first soak failure (we join as host, so killing the app ended the meeting), and the record that the defect narrative was backwards in four places. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 01d2c9eb..70e20000 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -146,7 +146,7 @@ untracked. The tier is a proposal, not an owner ruling. | ID | Item | Clause | Size | |---|---|---|---| | [T5.1](https://github.com/iamfatness/CoreVideoPro/issues/447) | Reconcile persistent sources with the rearch: which #419 foundations slices 2-3 build on (likely `SourceRegistry`, atomic Take, `DeliveredProgramPacket`) | 3 | S | -| [T5.2](https://github.com/iamfatness/CoreVideoPro/issues/448) | Tiles wall stops re-animating on the cut (persistent-sources slice 2, on T5.1) | 3 | M-L | +| [T5.2](https://github.com/iamfatness/CoreVideoPro/issues/448) | Tiles wall stops re-animating on the cut (persistent-sources slice 2, on T5.1) — **plan 1 of 2 in PR #511**: one animator per wall + the #419 `SourceRegistry` carve, its first real consumer (soak: 20 takes, 20 cut, 0 rebuilt). Plan 2 = the wall texture / one-layer slice, Metal + CPU parity, the PVW cell as the registry's first reader, `AtomicTakeCoordinator`. Residual minors: #512. | 3 | M-L | | ~~T5.3~~ | ~~A clip entering Program cold-starts with a placeholder flash~~ — **moved to T1.11** (owner, 2026-09-11) | | | | [T5.4](https://github.com/iamfatness/CoreVideoPro/issues/450) | OHG: redesign on screens first, integrate into existing tabs; cheap parity gaps (preview tally, gallery order, black/bars/FTB, on-air clock, nameplates) | 3 | L | | [T5.5](https://github.com/iamfatness/CoreVideoPro/issues/451) | The scene canvas editor shows live GPU video (redesign, not a whitelist) | 3 | L | From 821f950e7db22e04f376b1bed4bfefa91dc2c617 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 06:47:17 -0400 Subject: [PATCH 20/20] =?UTF-8?q?docs:=20record=20the=20finalizer-thread?= =?UTF-8?q?=20XAML=20release=20crash=20(#513)=20=E2=80=94=20mechanism=20co?= =?UTF-8?q?nfirmed,=20trigger=20not=20reproduced?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_014yuH7EMvWCWdkhvevrtMyJ --- CLAUDE.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 43b6e516..ca14274e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -354,6 +354,38 @@ off-thread guards never fired). Confirmed and suspected triggers: The proof is a scripted close-cycle loop on the real app: zero new `CoreVideoPro.WinUI.exe.*.dmp` and zero Application Error 1000 events. +- **The GC FINALIZER THREAD releasing a XAML object (#513, 2026-09-13) — the first + member of this family that is ASYNCHRONOUS and TIME-DELAYED, and it is NOT + reproduced yet.** The app died IDLE, 58 min into a live meeting, 43 min after the + last operator action, with `launch.log` silent the whole time. Dump + (`CoreVideoPro.WinUI.exe.19580.dmp`, full memory): crashing thread is the CLR + **Finalizer** (MTA); stack `GC.RunFinalizers -> WinRT.IObjectReference.Finalize + -> Microsoft_UI_Xaml!ctl::ComObject::Release -> + FailFastWithStowedExceptions`, stowed `0x8000000E` = **E_ILLEGAL_METHOD_CALL**. + The wrapper was a PLAIN `WinRT.ObjectReference` (not + `ObjectReferenceWithContext`) with `_referenceTrackerPtr` set, so the release + had no UI context to marshal to; the UI thread was idle in `GetMessage`, so a + marshaled release would have landed. **What this is NOT:** a finalizer-thread + release is the ORDINARY path — a forced full GC (`dotnet-gcdump collect -p`) + on a healthy run finalized ~2,700 wrappers and a couple of Borders with no + incident, three times (fresh app; after 12 takes; after a record/stop cycle). + The dead population at the crash (66 Borders, 1,845 wrappers) was the SAME size + as a healthy run's. So the trigger is a specific object STATE, not volume, and + it did not reproduce on demand. Our code has no manual CsWinRT marshaling and + no element-building control touches XAML off-thread (checked). Framework: + WinAppSDK Runtime 2.4.0 / WinUI 2.3.6, CsWinRT 2.2.0. The four code-behind + element factories that `Children.Clear()` (`ShowMultiviewHost` overlays, + `ScenePreviewControl`, `AudioLevelMeter`, `SceneCanvasEditorControl` — which + hooks 4 handlers and unhooks 0) are the likely POPULATION, not a proven cause; + pooling them reduces exposure and cannot be claimed to eliminate the crash. + **Two rules it teaches.** (1) A stability claim is bounded by the window you + watched: 25 clean minutes of takes/drill/soak said nothing about hour 1, and a + crash with NO application code on the stack is invisible to every log we write + — only the dump sees it, so `setup-crash-dumps.ps1` full dumps are not optional + on a test box. (2) Analyze a WinUI dump BEFORE rebuilding the shell (same PDB + rule as the core); `!dumpobj` on the finalizer frame's `this` is what + distinguishes a marshaled release from an unmarshaled one. + Rules of thumb: never replace a bound collection at frame rate (sync in place / diff); keep one stable swap chain per surface (program, preview, one multiview); present with **skip-present** (only on a new keyed-mutex frame) — smooth-present crashes