Repository navigation
feat(core): bound imported scene timing to source intervals - #5132
Conversation
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Reviewed at 7503e04fa, focused on scene timing after edits.
- The trim/rate mapping and the terminal-frame clamp (
range.duration - 1/range.fps) read right. - The browser fixture's flat colours are far enough apart to catch a one-source-frame shift at the interior times it samples.
1. Blocker: the frame-source adapter and export disagree on visibility at a near-frame start (frameSources.ts:123-135).
- The adapter calls
isClipVisibleAt(time, start, ...)with the authored start. Export visibility snaps both ends first:timeline.ts:83usesexportClipWindow, which callssnapTimeToFrameBoundarywithFRAME_BOUNDARY_EPSILON = 1e-3frames. - Example at 30 fps with a scene starting at
1.00001(decimal noise is exactly what the snap exists for, and trim drags produce it): att = 1export shows the scene, but the adapter skips the seek because1 < 1.00001. That frame captures an unrendered or stale iframe. - Fix: gate dispatch with the same
exportClipWindowwhenexportRenderSeekis on, keep the authored start for thelocalTimearithmetic (clamped at 0 as now), and add a browser capture at that boundary.
2. Should-fix: the save/reopen test doesn't prove a reopened scene applies its range (session.film-scenes.test.ts:11-17,50-62).
sourceRangelives in an inertapplication/jsonscript, and the test checks that its text survives serialization.- Nothing registers a frame source after reopening, so the test passes even if the range is never applied.
- Fix: reopen an executable wrapper and assert the rendered source time after an edit.
Tests not run locally: no installed deps in my checkout.
— Jerrai
…-bridge # Conflicts: # packages/core/src/runtime/entry.ts
# Conflicts: # packages/core/src/runtime/frameSources.test.ts # packages/producer/src/services/coreRuntimeBrowser.test.ts
|
Both findings are valid and fixed in the latest commit; fixes from both parent PRs are included.
Validation: 4,252 core tests, 591 SDK tests, 51 real-browser tests, core build/typecheck, lint/format and commit hooks passed. Browser coverage uses first-party protocol fixtures. Actual partner HTML preview/export acceptance remains unverified under the previously reported browser policy block. |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at b29b84abb. Both findings are fixed.
- Dispatch boundary: source dispatch uses
exportClipWindow(start, end, canonicalFps)only under export seeks, so preview keeps authored bounds, andlocalTimestill subtracts the authored start, clamped at 0 (frameSources.ts:134-141). The unit and browser tests cover the near-frame start, the last active frame, and the snapped end. - Save/reopen: the browser test reopens the serialized composition, runs the preserved registration, and asserts source time 3.5 plus pixels. Dropping
sourceRange.startwould give 0.5, so the test discriminates.
— Jerrai
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at b29b84abb, focused on scene timing after edits.
- The boundary fix is right. Dispatch now gates on
exportClipWindowwith the canonical FPS whenever export seeking is on (frameSources.ts:136, wired atinit.ts:3988), the same flag and window the visibility sync uses (init.ts:2621,timeline.ts:83). A start of1.00001at 30 fps now draws att = 1in export.localTimestill uses the authored start, clamped at 0. - The clamp to the last source frame (
frameSources.ts:65) gives the hold-last-frame behaviour the guide promises. - The browser fixture's delayed acknowledgement makes the pixel checks meaningful for reverse, repeated, fresh-page and inpoint/rate seeks.
One note, not a finding: the editable-timing guide mounts one runner per scene, and each runner loads the full film. A 10-scene import means 10 full runners, each structured-cloning the whole asset payload and racing its own 20 s startup deadline on the render machine. A measured ceiling (scene count and payload size) in the importer would show where that stops holding.
CI at this head: 28 passing. regression and Player perf were cancelled at this sha, not failed; rerun them before merge.
Verdict: APPROVE
Reasoning: frame-source dispatch now matches export visibility at snapped boundaries, and the source-interval mapping and terminal hold are correct.
A composition that follows the frame-source contract registers no GSAP timeline, so lint reported missing_timeline_registry and every render waited 45 s for timeline registration (17.6 s -> 1 min 47.9 s for the 14 s imported film sample). Document the existing data-no-timeline opt-out for the host and a timeline-less root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Imported film wrappers have no GSAP timeline. Without data-no-timeline on the scene hosts and root, lint fails with missing_timeline_registry and each render waits 45 s for timeline registration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The user supplied an independent Claude Code end-to-end validation report for the actual harbor-ferry sample. It tested runtime b29b84a, carried by the previous documentation-only stack head 40e53e2. I have read the report; I did not independently execute the partner HTML in this session. Reported actual-sample results:
The report also found redundant transport draws after renderSeek. I reproduced that with a first-party async fixture and fixed it in 29a5c02, now carried in this PR. This final transport change is covered by our first-party regressions, but is not covered by the report's actual-sample runs. Final-stack validation passed: 53 real-browser tests and 316 targeted runtime tests (entry/init/transport/frame sources/film bridge), plus the core build. The owning PR also passed its full 4,234-test core suite and commit hooks. Remaining acceptance scope: Studio drag gestures/server-side undo and speed-control UI, MCP importer, audio/video sources and Linux rasterization were not exercised. The pre-existing 30 fps preview grid remains a separate issue. The report and local artifacts remain outside the repository. |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approved at c73af0609. The parent merge carries exactly the #5130 delta (init.ts +3, the browser test, and the frame-sources doc line), plus one doc line in imported-film-html.md requiring data-no-timeline on the film host. Nothing else changed since my last approval.
The regression, Perf and Preview parity failures here were cancelled runs ("Change detection did not succeed"), not test failures. I have re-run them.
— Jerrai
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-stamp at c73af0609. New since b29b84abb:
init.ts:renderSeeknow records its time as the transport's last seek, so the next transport tick does not seek the adapters again and start a second async draw after capture has already waited. This is the right owner for the fix: the transport's own "seek only when time or timeline changed" check now sees the explicit seek. Playback still drives sources, because the clock moves on.- The new browser test covers preview and export modes. It asserts exactly one draw per explicit seek after four transport frames and a second barrier wait, then that playback keeps drawing.
- The docs now require
data-no-timelineon hosts and on timeline-less roots, which avoids the 45 s timeline-registration wait.
CI: 27 passing, none failing.
Verdict: APPROVE
Reasoning: the delta closes a real post-capture redraw race at its source and is pinned in both seek modes.
The base branch was changed.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-stamp at 7a3d33587. Since my approval at c73af0609, the only changes are a merge of main and test-harness fixes; runtime code is unchanged.
1a134218bgives the film fixture an explicit page background, so the hidden-scene end frame no longer depends on a default colour.7a3d33587brings each page to the front before its screenshot. The diagnostic run at3b471b866showed the reverse/repeated test hanging insidepage.screenshot()after the 0.5 s frame had already been acknowledged, so this targets the right step.Producer: integration testsnow passes (190 of 190).
Optional: the phase tracking and onTestFailed logger in that test were for the diagnosis and can come out now.
CI at this head: 44 passing, 39 pending (edit accuracy, regression shards, Windows), none failing.
Verdict: APPROVE
Reasoning: the runtime change is the one already reviewed, and the film browser tests that first ran against main are now green.
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-approved at 7a3d33587. The scene-timing diff is identical to what I approved at c73af0609: 9 files, +420/-5 on both sides, and all 531 normalized diff lines match (the only difference is a line-number shift in one init.ts hunk). The three later commits (1a134218b, 3b471b866, 7a3d33587) touch only coreRuntimeBrowser.test.ts:
- the explicit fixture and reference background is the same on both sides;
- the new diagnostics only report which phase failed;
- the capture helper brings both the film page and its reference to the front before each screenshot.
Pixel comparison is still exact Buffer equality, and the 30 s deadline and the seek barrier are unchanged.
— Jerrai
Edit accuracy: accurate 2059 (base branch 2059), smooth 1595 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
What
Keep imported scenes within their original film source interval while allowing normal HyperFrames host timing edits. Extending a clip holds its last source frame instead of advancing into the next scene.
Why
A preserved runner accepts whole-film time. Scene-level trim, reorder, rate and overlap edits need an independent original source range; changing host duration alone cannot safely identify the scene's boundaries.
Related work
Depends on #5131 (which depends on #5130); merge parents first. Final PR of the runtime/OSS stack. Somansh's MCP importer supplies original scene metadata and separate runner instances.
How
Add optional
sourceRange: { start, duration, fps }to frame sources. Apply original offset after host inpoint/rate mapping and clamp to the interval's last source frame. Reuse existing half-open clip visibility semantics, including final-frame holding. Existing SDK editing/serialization already preserves these no-GSAP hosts, so this PR adds regression coverage rather than a new editor path. Disposal clears the runner document as well as rejecting pending requests, stopping scene work when the source is released.Test plan
Core build/typecheck and changed-file lint/format passed. All 4,244 core tests and 591 SDK tests passed (core uses
--maxWorkers=4 --testTimeout=30000). Existing engine seek-completion capture regression checked. Complexity report: highest new/modified frame-source function is CC 9. Scene-internal manual objects/keyframes and audio/video extraction remain outside this stack.Real-browser capture regression
All 48 tests in
packages/producer/src/services/coreRuntimeBrowser.test.tspass against the rebuilt runtime. The new first-party film-protocol fixture uses an opaque iframe, asynchronous draws, the public player and the engine's capture completion barrier. Screenshot pixels match static references for forward, reverse and repeated seeks, terminal source bounds, a fresh page, and edited playback inpoint/rate. It runs in the existing producer integration lane.Commands:
bun run --cwd packages/core build, thenbunx vitest run src/services/coreRuntimeBrowser.test.tsfrompackages/producer.This is synthetic browser-capture coverage. Shirley's downloaded HTML has not been executed or rendered: browser-tool local-file access was denied, including alternate execution routes. No partner source/assets are included in the test. Actual partner HTML visual acceptance and a complete video export remain unverified.