diff --git a/CLAUDE.md b/CLAUDE.md index 914dcd1c..ca14274e 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 @@ -343,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 @@ -852,20 +895,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 +1302,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 @@ -1325,6 +1382,92 @@ 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) + +**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` +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 — `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 +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/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 | 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..5db8c24e --- /dev/null +++ b/docs/superpowers/plans/2026-09-12-tiles-wall-one-animator.md @@ -0,0 +1,784 @@ +# 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 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) { + 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 + // 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 << " lost its in-flight animation 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. 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..a5f4b126 --- /dev/null +++ b/docs/superpowers/specs/2026-09-12-tiles-wall-persistent-source-design.md @@ -0,0 +1,340 @@ +# 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: + +> **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. + +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 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 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. 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 +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 +NOT-APPLICABLE, never as false. + +**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. +- **`AComposedWallNeverClaimsASubscriptionState`** — a registered wall reports + `personId`, `externalId`, `availability`, `subscriptionRequested` and + `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. + +## 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. diff --git a/native/CMakeLists.txt b/native/CMakeLists.txt index a0ff73c3..7d6d66b6 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,8 @@ if(BUILD_TESTING) tests/CompositorFramingTest.cpp tests/ContractParityTest.cpp tests/RouteSourcePolicyTest.cpp + tests/SourceRegistryTest.cpp + tests/SourceRegistryComposedTest.cpp tests/EmptyRouteBlankTest.cpp tests/OutputLifecyclePolicyTest.cpp tests/MonitorShedPolicyTest.cpp @@ -666,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..c593e371 100644 --- a/native/src/compositor/TilesPlanAnimation.h +++ b/native/src/compositor/TilesPlanAnimation.h @@ -9,46 +9,37 @@ 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; - } + // 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; } - 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. + // `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) { - if (!present || !enabled) { reset(); return; } + 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.) - 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 +55,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/MediaCore.cpp b/native/src/core/MediaCore.cpp index 00690284..cccc63b0 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,24 @@ 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. + // 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); + } 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 +3689,14 @@ 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); + // 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); + } 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,29 +6281,201 @@ 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; - 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()) { - auto previewAnimationPlan = buildPreviewCompositorRenderPlan(videoFrames); - previewTilesAnimation_.advance(previewAnimationPlan, previewSceneId_ + ":" + previewTilesLayer_.layerId, - true, previewTilesLayer_.style.animateLayout, previewTilesLayer_.style.animationDurationMs, animationNowMs); - } else { - previewTilesAnimation_.reset(); + // 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; + 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, 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 + // insert on a mere read) — an unknown wall has never animated and reports + // 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 (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. + // 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; + } + // Advance Preview on the SAME render clock even when its wall is empty, so + // 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 (with Program's `enabled` — + // Finding 2) — a second call here would be the round-2 double-advance bug. + if (previewPresent && !sameWall) { + // 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. + // + // 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).releaseIfIdle(); + } + } + // 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: 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; + // 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 + // 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. + // 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 + // 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); + } + // 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 @@ -6374,7 +6575,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 +6756,12 @@ 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); + // 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/src/core/MediaCore.h b/native/src/core/MediaCore.h index 7496f1cd..7af1ff46 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" @@ -10,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" @@ -266,6 +268,18 @@ 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; + // 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_; } @@ -519,8 +533,37 @@ 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: 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_; + // 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 @@ -752,7 +795,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/SourceRegistry.cpp b/native/src/core/SourceRegistry.cpp new file mode 100644 index 00000000..56904c81 --- /dev/null +++ b/native/src/core/SourceRegistry.cpp @@ -0,0 +1,308 @@ +#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 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() <= kMaxIdBytes); + return knownKind && externalIdOk && + (!r.requestedGeneration || (*r.requestedGeneration > 0 && *r.requestedGeneration <= kMaxRevision)) && + (!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) || + (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 { + 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; + } + 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); + 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_; + ++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; + // 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; + 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; + // 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; + 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; + // 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 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; + source.subscriptionObserved = false; + source.format.reset(); + source.hasPublication = false; + // Retired tokens retain their publication watermark for diagnostics. + } + ++revision_; + 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); + 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..a0d59be1 --- /dev/null +++ b/native/src/core/SourceRegistry.h @@ -0,0 +1,145 @@ +#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: + // 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 { + 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; + // 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. + 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; + // 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; }; + + 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); + // 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. + // 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; + // 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; + 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/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/src/core/TilesWallSource.h b/native/src/core/TilesWallSource.h new file mode 100644 index 00000000..c23e9c1e --- /dev/null +++ b/native/src/core/TilesWallSource.h @@ -0,0 +1,98 @@ +#pragma once + +#include "compositor/TilesPlanAnimation.h" + +#include +#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(); + } + } + + // 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); + } + + // 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/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/SourceRegistryComposedTest.cpp b/native/tests/SourceRegistryComposedTest.cpp new file mode 100644 index 00000000..71e7d7cd --- /dev/null +++ b/native/tests/SourceRegistryComposedTest.cpp @@ -0,0 +1,210 @@ +#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); +} + +// 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); +} + +// 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); +} + +// 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"}), + SourceRegistry::Result::NotFound); +} +} // namespace diff --git a/native/tests/SourceRegistryTest.cpp b/native/tests/SourceRegistryTest.cpp new file mode 100644 index 00000000..0c35ac39 --- /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}); +} 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& 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, @@ -1020,6 +1044,218 @@ void take(MediaCore& core, const corevideo::rpc::Json& program, const corevideo: } // namespace +// #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 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. +// +// 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 +// as an "adoption" — its content is already-present, not entering (see +// DifferentWallCannotReuseAnotherWallsCachedGeometry / TilesAnimatorTest.cpp, +// unrelated to and unchanged by #448) — so a wall's very first tick is +// SETTLED instantly and cannot exercise this test. A member JOINING an +// ALREADY-ESTABLISHED wall does animate in, and its alpha is exactly 0 on the +// single tick it is added (entryElapsed only advances on LATER ticks) — the +// everyday case that actually produces a mid-flight take. +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"), + 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{ + wallSceneWithDuration("set-preview-scene", "gallery", wallMembers(), kDurationMs)}); + + 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, wallSceneWithDuration("load-scene-graph", "gallery", wallMembers(), kDurationMs), + 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"; + } 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"; +} + +// 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. +// 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"}; + + // 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)}); + + // 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"; + + // 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{}); + 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"; + } +} + // 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) { @@ -1217,3 +1453,121 @@ 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"; +} + +// 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 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"; }); + 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)"; +} diff --git a/native/tests/TilesWallSourceTest.cpp b/native/tests/TilesWallSourceTest.cpp new file mode 100644 index 00000000..2f8f4d8f --- /dev/null +++ b/native/tests/TilesWallSourceTest.cpp @@ -0,0 +1,95 @@ +#include "core/TilesWallSource.h" + +#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 +// 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); +} + +// 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 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) {