fix(producer): give inlined media a document-unique render id

[miguel-heygen]

What

Media elements in a compiled render document now carry a document-unique data-hf-render-id, and the producer reads its media list off the inlined document instead of merging the per-file lists and deduplicating by element id.

Fixes #3340.

Why

Element ids are unique within one composition file. The render document is the inlined union of every file, so ids collide there. The producer merged the per-file media lists and deduplicated by id, which collapsed colliding clips into a single entry, and every id-keyed stage (extract, inject, visibility, bounds) then resolved through document.getElementById to whichever element came first in the document. The surviving clip's frames were injected onto the wrong element, usually one that is hidden during the survivor's window, so the visible scene rendered scene chrome with no footage.

Two shapes hit this, and neither is author error:

1. Two scenes that each declare <video id="clip">. Legal per file. Unavoidable when a scene is duplicated into a copy with its inner ids kept, and impossible to avoid when one file is mounted twice: it is a single file with a single id.
2. Two scenes that each declare a bare <video>. The timing compiler numbers auto-ids per file, so both arrive as hf-video-0. No authored id is involved at all, which makes this the more common of the two.

On main, the issue's reproduction compiles to videoCount: 1 and renders both halves as flat scene background.

How

assignMediaRenderIds (core) stamps every video[src] / audio[src] / img[src] with a document-unique key while the inliner still holds the merged document. That is the point where ids become ambiguous, and the only point that can tell repeated mounts of one file apart. The render id equals the element's own id whenever that id is already unique, so a document without a collision keeps byte-identical pipeline keys and log output; only later duplicates get a __hf2 suffix.

collectRenderMedia (producer) then reads the media list from the inlined document and recovers each clip's timeline window from the composition hosts it is nested inside. This replaces the per-file extraction plus id-dedupe merge, and retires parseSubCompositions's media extraction and offset bookkeeping along with it, so "what media exists in this render" has one owner.

Author id attributes are deliberately left untouched. Renaming the duplicate would need no engine changes at all, but 158 of the 161 registry blocks reference their own element ids from #id CSS or getElementById, so renaming would trade broken footage for broken styling in exactly the compositions being fixed. The engine instead resolves media through the render id, via one shared in-page bridge installed with evaluateOnNewDocument. Every call site keeps a getElementById fallback for documents the producer never compiled (snapshot, check, direct engine callers), where the authored id already is the identity.

Two details worth a reviewer's eye:

• The __render_frame_* sibling <img>s are derived from the render id now. They were derived from video.id, so duplicate videos produced duplicate sibling ids, moving the collision one element sideways. The four runtime readers of that sibling in packages/core/src/runtime were derived from the plain el.id too and now route through one shared owner, renderFrameSibling.ts. colorGrading's is the one that changed pixels: it returns the image the grading pass samples, so the second collider was graded from the first one's frame.
• applyDomLayerMask addresses a stamped element only by its render id, with no #id fallback alongside. An id is duplicated exactly when two compositions share it, so keeping #id as an extra selector would unhide the other scene's element, which is the collision this is meant to resolve.

recompileWithResolutions keeps the first-pass media list rather than re-collecting. Resolving a composition's duration stamps a data-end on its host, and re-collecting would newly clamp clips to it. That is a retiming, not an identity fix, and there is an existing test pinning the current behaviour.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

New tests

• packages/core/src/compiler/mediaRenderIds.test.ts (8 tests): unique ids pass through unchanged, authored and auto-id collisions disambiguate, repeated collisions keep counting, the pass is idempotent, and re-running over a partially stamped document does not hand out an id a later element already holds.
• packages/producer/src/services/htmlCompiler.test.ts (6 tests): all three collision shapes end-to-end through compileForRender (two scenes sharing an authored id, one scene file mounted twice, two unnamed videos), plus each id addressing exactly one element in the compiled HTML, author ids surviving intact, and the same collision for <audio>.

Reproduction, from the issue, rendered end to end:

	main	this branch
videoCount	1	2
t=0.5s / 1.5s / 2.5s	061907 (scene bg)	footage
t=3.5s / 4.5s / 5.5s	180a28 (scene bg)	footage

Both halves also show the correct slices: the rendered frame at t=0.5s matches the source at 5.5s and the frame at t=3.5s matches the source at 50.5s, which are the two scenes' distinct data-media-start values.

Suites: core 2421 passed, engine 1601 passed (3 skipped), producer unit lane 40 files with zero failures.

Not covered

• The full producer render regression suite (Docker, native linux/amd64) is still running; result will be posted as a comment.
• distributed/png-sequence reported PSNR 0 on all 60 frames in that run. That was my harness setup, not the code: the worktree was created without git lfs pull, so every baseline frame was a 129-byte LFS pointer being compared against a real frame. The pointers record the baselines' sha256, and the rendered frames hash identically to them on every frame sampled, so this branch reproduces that baseline byte for byte.
• No lint rule was added for duplicate ids. Lint cannot fix the mount-one-file-twice case, and after this change a collision is no longer a defect worth failing on.
• The reporter's data-hf-render-key="<composition-chain>.<id>" naming was not adopted. A chain of authored composition ids is identical across two mounts of one file, so it does not disambiguate that case.

[jrusso1020]

Reviewed at 096f9624. The diagnosis is right and the fix is in the right place — stamping while the inliner still holds the merged document is the only point that can tell two mounts of one file apart. All four items you flagged check out at source, with one exception: item 3 closes on the creation side and leaves four in-page readers on the old derivation. No blockers.

Audited end-to-end: core/compiler/mediaRenderIds.ts, producer/services/renderMediaCollector.ts, engine/services/mediaRenderIdBridge.ts, the changed regions of screenshotService.ts / videoFrameInjector.ts / videoFrameExtractor.ts / audioMixer.ts / frameCapture.ts, plus the untouched readers the change implicates — core/src/runtime/colorGrading.ts, media.ts, mediaProxy.ts, adapters/video-texture-compat.ts, and main's retired parseSubCompositions for offset parity. Read for context: both new test files, compilationRunner.ts / compilationTester.ts. Not executed: I did not run the suites locally; everything below is a source read.

CI is incomplete, so nothing here rests on it. 7 of 8 required contexts green at this head; the required Test context has not reported — the CI run is still in_progress, with Producer: integration tests and Preview parity also running. Worth a re-check before this merges rather than reading the current rollup as green.

The four you flagged

1. Byte-equivalence — holds, and it is correctly scoped. uniqueRenderId returns baseId untouched when it is unclaimed (mediaRenderIds.ts:51-52), and all three parsers prefer the stamp then fall back to the plain id (videoFrameExtractor.ts:547 and :621, audioMixer.ts:494). So on a collision-free document every pipeline key is the plain id exactly as before. Note the claim is about keys and log output, not the compiled HTML — that does gain an attribute on every media element — and the body says keys, so it is accurate as written.

   Because that looked like it should move the committed goldens, I checked: the 29 packages/producer/tests/*/output/compiled.html files that contain <video> do not break. compilationRunner.ts:54 compares through validateCompilation (compilationTester.ts:245), which extracts timed elements and matches them by author id — and author ids are untouched. Structural, not byte-for-byte.

2. applyDomLayerMask — safe, and the code is stricter than the body describes. screenshotService.ts:476-483 is an if/else, not an unconditional render-id-only path: a stamped element is addressed by [data-hf-render-id="…"] alone (right — #id would unhide the colliding scene's element), while an unstamped document still falls back to #${CSS.escape(id)} at :481-482. The element resolution a few lines up (:470) goes through the bridge, which itself falls back to getElementById (mediaRenderIdBridge.ts:39). And both callers are producer HDR-composite paths (hdrCompositor.ts:683, captureHdrFrameShared.ts:244), never snapshot or check — so the asymmetry cannot reach a document the producer did not compile. The audit comes out clean in both directions.

3. __render_frame_* — see below. It closes in the engine and not in core/src/runtime.

4. recompileWithResolutions — the test pins exactly what you said, and it is genuinely pre-existing. htmlCompiler.test.ts:1139-1166 is present on main at the same line. It resolves scene-host to duration 2 and asserts the clip stays {start: 2, end: 6}, i.e. the freshly stamped data-end does not newly clamp it. Claim verified rather than taken.

important — item 3 closes on the creation side; four in-page readers still key on video.id

The engine half is right: the sibling is created from the render id (screenshotService.ts:615, __hfMediaId(video) ?? video.id) and read back by render id (videoFrameInjector.ts:288, screenshotService.ts:484,510). But four readers of that same id live in packages/core/src/runtime and still build it from the plain el.id. None is in the diff:

• colorGrading.ts:2472-2476 — the sharp one. findRenderFrameImage is document.getElementById(\render_frame${video.id}_`)with no sibling or class check to save it. For the second colliding video (plainclip, render id clip__hf2) that resolves render_frame_clip— the *first* video's frame — sohasInjectedRenderFrame (:2478-2484`) answers using another element's image and the grading source is the wrong clip's pixels.
• adapters/video-texture-compat.ts:36-40 — mostly mitigated. The primary path is the class-checked immediate sibling (:27-35) and the engine inserts the <img> at video.nextSibling, so the id lookup is only the documented "in case a node was inserted between them" fallback.
• media.ts:413-414 (skipForInjectedVideo) and mediaProxy.ts:63-69 (isRenderMode) — both are really "are we mid-render" proxies, so finding any render-frame sibling gives the right answer. Correct today for a reason that has nothing to do with identity, which makes them latent rather than broken.

Grading this honestly, because it cuts against calling it a blocker: on a collision-free document the render id is the plain id, so all four behave exactly as before, and on a colliding document they were already resolving to the wrong element before this PR. Nothing that worked stops working — this is incomplete propagation of the new identity, not a regression, and the render is strictly better than main either way. It earns the flag because it sits in the one place the body says the sideways collision is closed, and colorGrading turns it back into wrong pixels for the second clip.

Cheap to close: the bridge is installed via evaluateOnNewDocument before any page script runs (frameCapture.ts:1300), so window.__hfMediaId?.(el) ?? el.id — the same one-liner the engine uses — is available in core runtime too, and mediaProxy.ts:64 already reads an optional global two lines above the site.

nit — a unique authored id can still be renamed by an unrelated collision

uniqueRenderId draws suffixes from the same taken set, so for plain ids clip, clip, clip__hf2 in document order the third element — whose authored id is unique — becomes clip__hf2__hf2, because the second claimed clip__hf2. Only pipeline keys move (author ids are untouched), so nothing user-visible breaks, and it is outside the body's "document without a collision" claim. Worth noting that the stamped form of this is tested ("does not claim an id that a later element already holds as its render id") while the unstamped form is not.

notes

• Selector agreement checked, since a miss here fails quietly rather than loudly: MEDIA_SELECTOR (video[src], audio[src], img[src]) covers every element the three parsers select (video[src], img[src], audio[id][src]), so no parsed element arrives unstamped. That matters because an unstamped element would silently take ROOT_WINDOW at renderMediaCollector.ts:126 — losing its host offset rather than erroring — and the -audio strip at :144 lines up with audioMixer.ts:507 building the track id off the render id.
• Host-offset parity against the retired walk: parseNumeric rejects a relative data-start and contributes 0, and main's retired code did the same (parseFloat(el.getAttribute("data-start") || "0")), so a composition host with a referenced start behaves identically on both sides. Publishing the negative result so the next reader does not re-derive it as a finding.
• Your "Not covered" note has distributed/png-sequence at PSNR 0 pending confirmation as pre-existing. The reasoning is consistent — that fixture has no media elements, so nothing in this change reaches it — but the same-image comparison against main is still owed before it can be treated as settled.

Verdict: COMMENT
Reasoning: No blockers — the identity fix is sound, correctly placed, and every load-bearing claim in the body holds at source. Not an approval because a required context has not reported yet and the item-3 gap is worth closing first; neither is a reason to hold the design.

— Rames Jusso

[miguel-heygen]

Thanks, that's a good catch and the grading one is real. Pushed c19ea92.

Item 3: all four readers now route through one owner

You were right that it's the half the diff doesn't show. Confirmed all four at source before touching them:

reader	what the plain-id lookup did
colorGrading.ts findRenderFrameImage	returns the image the grading pass samples, no class check to catch it — the second collider was graded from the first one's frame
media.ts skipForInjectedVideo	render-mode signal
mediaProxy.ts isRenderMode	render-mode signal
adapters/video-texture-compat.ts	id lookup, but only as a fallback behind a positional sibling check

The bottom three happen to agree on a colliding document, because during render both videos are injected so the boolean lands right either way. That is agreement by accident, so they got the same treatment rather than a "safe enough" pass.

New packages/core/src/runtime/renderFrameSibling.ts owns "which frame belongs to this element" (readMediaRenderId / renderFrameElementId / findInjectedRenderFrame), and all four call it. It reads the stamp and falls back to the author id, so a collision-free document resolves exactly as before and preview/snapshot/check are untouched.

I took your suggestion's intent but not the mechanism: rather than window.__hfMediaId?.(el) ?? el.id in core, the runtime reads MEDIA_RENDER_ID_ATTR directly. mediaRenderIds.ts has no imports, so core/runtime importing it pulls one string constant and no bridge-installation ordering assumption. The engine's in-page copy stays (page.evaluate can't import) but now names core as the definition, and renderFrameSibling.test.ts pins the __render_frame_<renderId>__ format both sides build so they can't drift silently. 7 new tests, including the two-colliding-videos case resolving to distinct frames.

png-sequence: my reasoning was wrong, and the result is better than I claimed

It is not a Chrome bump. I quoted the meta.json note because it matched the shape, but the actual cause is my own harness setup: I created the devbox worktree without git lfs pull, so every "baseline" frame was a 129-byte LFS pointer. That is what PSNR 0 on all 60 frames was — real frame vs. pointer text.

Checking it properly turned out to be stronger evidence than the same-image comparison you asked for. The pointer records the baseline's sha256, so the rendered frames can be compared against the committed baseline directly:

pointer oid: 2c8fe979b1f7614e3d06a8621676000d490385e24e4e1621220d7fa6d7e7c40c
actual sha256: 2c8fe979b1f7614e3d06a8621676000d490385e24e4e1621220d7fa6d7e7c40c

Byte-identical on all four frames I sampled (0.00s, 0.13s, 0.23s, 0.30s). So this branch reproduces that baseline byte for byte, which is the strictest gate in the suite passing, not failing. No main comparison needed and no baseline regeneration.

I've corrected the PR body rather than leaving the wrong explanation in it.

Still owed

Agreed on not reading the rollup as green — I won't. The full Docker regression suite is still running on this branch (it was mid-flight when I pushed, so it is testing 096f9624, not the runtime commit); I'll re-run and post the summary. Local suites after the new commit: core 2428, engine 1601 (3 skipped), producer unit lane 40 files zero failures, and the issue reproduction still renders footage in both halves with videoCount: 2.

Ready for re-review whenever you are.

[jrusso1020]

Re-review at c19ea923 (my earlier COMMENT was at 096f9624). Both things I named as holding back an approval are resolved, so this is an approval — see the CI note and the follow-up below.

The item-3 gap is closed, and closed better than I asked for

I suggested routing the untouched readers through the new identity; renderFrameSibling.ts makes it a single owner with the derivation documented, which is the version that survives the next person touching it. Verified each reader at source rather than trusting the commit message:

• colorGrading.ts:2473-2475 — findRenderFrameImage now resolves via findInjectedRenderFrame, keeping isDrawableSource as the filter. This was the one that changed pixels, so it is the one that mattered.
• adapters/video-texture-compat.ts:38-40 — id-lookup fallback replaced; the sibling fast path keeps its __render_frame__ class check.
• media.ts:414 — skipForInjectedVideo no longer needs the el.id && guard.
• mediaProxy.ts:66 — isRenderMode likewise.

Two details worth calling out because they are easy to get wrong and this got them right. readMediaRenderId uses ||, not ?? — getAttribute returns "" for a valueless attribute, and ?? would have kept that empty string instead of falling through to the author id. And it matches the in-page bridge exactly (mediaRenderIdBridge.ts:46, el.getAttribute(attr) || el.id), which is the whole point of having one definition. The author-id fallback keeps uncompiled documents — preview, snapshot, check — resolving as before, and renderFrameSibling.test.ts covers all three states including the collision case that resolves each video to its own frame.

important — the drift guard names both sides but pins one

mediaRenderIdBridge.ts:16-19 says renderFrameSibling.test.ts "pins the sibling-id format both sides build." It pins core's side: renderFrameElementId asserted against the literals __render_frame_hero__ and __render_frame_c__hf2__ (renderFrameSibling.test.ts:36-43). The engine builds that same id at six independent template sites — screenshotService.ts:484, :510, :615, :732, and videoFrameInjector.ts:290, :669 — and none of them is compared against renderFrameElementId by anything. Change the template at :615 and the test stays green while every core reader silently stops finding its frame, which is this PR's own failure mode one level up.

The mechanism to fix it is already in the file: installMediaRenderIdBridge imports from core and passes MEDIA_RENDER_ID_ATTR into evaluateOnNewDocument as an argument. Installing the sibling-id rule the same way — a window.__hfRenderFrameId built from renderFrameElementId's definition, with the six engine sites calling it — leaves one definition, and then the existing test really does span both sides.

nit

screenshotService.ts:615 is window.__hfMediaId?.(video) ?? video.id where core uses ||. __hfMediaId is declared returning string and evaluates el.getAttribute(attr) || el.id, so for an element with neither it yields "" rather than undefined, and "" ?? video.id keeps "" — creating an <img id="__render_frame___"> that no core reader looks for, since readMediaRenderId returns null there. Inert today: compileForRender assigns positional ids to id-less timed media (the video-hfid-no-id fixture exists for exactly that), and the behaviour is identical to pre-PR. || would just make the two sides agree in the one case they currently don't.

notes

• findInjectedRenderFrame does not check the __render_frame__ class even though the test fixtures set it. That matches what colorGrading did before — isDrawableSource is the real filter — and video-texture-compat still class-checks its sibling path, so nothing loosened here.
• The engine-side id space is internally consistent, which I checked rather than assumed: every site that creates a sibling and every site that looks one up uses the same string it resolved the media element with (screenshotService.ts:705-706 and :732 both on item.videoId; videoFrameInjector.ts:286-290 on the same id it passed to __hfMediaEl; :656 derives id from __hfMediaId before :669 uses it).

CI

My earlier note was that the required Test context had not been created at all. It has now reported at c19ea923, and all 8 required contexts are green — including Test, Tests on windows-latest, regression, and all nine regression-shards plus Preview parity. That resolves the caveat I attached to the previous review.

Verdict: APPROVE
Reasoning: The identity fix is correct and complete across the readers, the two conditions I named last time — the item-3 propagation gap and the missing required context — are both resolved, and what's left is a drift guard weaker than its comment claims plus one inert operator mismatch, neither of which affects the fix. This approval doesn't merge it; the merge is yours.

— Rames Jusso

[miguel-heygen]

Both follow-ups closed in fbc9512. You were right that the guard was weaker than its comment, and the comment was mine, so it needed fixing before this merged rather than after.

The drift guard now spans both sides

Rather than the window.__hfRenderFrameId global, I took the affixes out of core (RENDER_FRAME_ID_PREFIX / RENDER_FRAME_ID_SUFFIX / renderFrameIdForRenderId) and removed the literal from all six engine sites. There is no `__render_frame_${...}__` template left in the engine — the only remaining mentions are prose in comments.

Two shapes, picked per site rather than uniformly:

• Four sites resolve the id on the Node side, where the engine can import core directly: screenshotService.ts :493 / :519 via the args payload, :750 from injectVideoFramesBatch's updates.map(... renderFrameIdForRenderId), and videoFrameInjector.ts :294 from videoIds.map(...).
• Two iterate the DOM in-page, so the id isn't known outside: ensureRenderFrameSiblings and queryElementStacking take the affixes as evaluate arguments.

I used arguments rather than a global on purpose. ensureRenderFrameSiblings documents itself as reachable from callers that skip initializeSession, and a global installed there would be undefined for them; an evaluate argument is always present, so this adds no install-order assumption on top of the one __hfMediaEl already carries.

The nit, both instances

Fixed at screenshotService.ts:629 and also at videoFrameInjector.ts:668, which has the same ?? el.id. Your reasoning was exact: __hfMediaId returns "" for an element with neither id, ?? keeps it, and __render_frame___ is an id no reader looks for. Inert for the reason you gave, and now the two sides agree in that case too.

Note on the queryElementStacking complexity finding

Worth flagging since it would look like a metric regression: adding the two parameters pushed the arrow onto its own line, which displaced the existing // fallow-ignore-next-line complexity so it no longer sat above the function it suppresses. The fallow gate failed on a 290-line arrow that was already there and already suppressed. Moving the comment inside the evaluate( call restored it, matching how injectVideoFramesBatch in the same file places its own. No complexity was actually added, and I did not widen any suppression.

Verification

Core 2428, engine 1601 (3 skipped), producer unit lane 40 files zero failures. The issue reproduction still renders videoCount: 2 with footage in both halves, sampled identical to the previous head at all six timestamps.

CI is re-running on the new head, so this is not green yet and I'm not treating it as such — same caveat you attached before. Yours to re-review; I'll hold the merge until you've had a look and CI reports.

[miguel-heygen]

Correction on the regression run I said I'd report: it produced no usable signal, so treat it as absent rather than as evidence.

It finished 72/72 failed, and the reason is the same one behind the png-sequence noise, one layer deeper than I first thought. Missing git lfs pull left the input fixtures as LFS pointers too, not just the baselines, so ffprobe rejected them:

{"event":"test_error","suite":"webgl-video-texture-render-compat",
 "error":"[FFmpeg] ffprobe exit with code 1: moov atom not found\n[input]: Invalid data found when processing input"}

The container was also killed partway through (exited with code 143). Nothing about the branch can be concluded from that run in either direction — it is not a pass I am claiming and not a failure worth chasing.

I am not re-running it. CI already runs this suite properly via the regression and nine regression-shards contexts, which you confirmed green at c19ea923; a local re-run would duplicate that and add an hour without adding coverage. The per-frame hash check from my earlier comment stands on its own for png-sequence, since it compared the rendered frames against the committed baseline oids directly.

So the regression evidence for this PR is CI's, not mine. It is re-running on fbc9512 now (11 pass / 16 pending as I write this), and I'll hold the merge until it settles and you've re-reviewed — the new commit dismissed the previous approval, so the PR is currently REVIEW_REQUIRED.