fix(core): clips that meet within float rounding count as meeting in edits and playback - #4535
miguel-heygen wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Blocking 1 — the new 12-significant-digit serializer moves frame-exact edit boundaries. parseTimeExpression("20f") at 30 fps produces 20 / 30 === 0.6666666666666666, but withoutFloatNoise writes 0.666666666667 for the duration of trim #a --end 20f and the start of trim #b --start 20f (a2Shared.ts:155,224–230). On interactive frame 20 the seek time is 20 / 30; runtime visibility uses authored seconds and the strict half-open test time >= start && time < end (core/src/runtime/timeline.ts:79–94, clipWindow.ts:1–3). Consequently A is still visible and B has not started at that frame. Before this PR, String(time.seconds) preserved the frame boundary. I verified the exact numeric/predicate result: at frame 20, original A=false/B=true, newly serialized A=true/B=false. Export-only frame snapping may mask it in renders, but it does not fix interactive preview/seek. Please preserve frame-derived boundary values exactly when serializing and test the saved bytes and loaded frame-20 visibility. The added two-trim e2e checks only command exit statuses.
Blocking 2 — the shared visual-overlap tolerance is not a valid CLI conflict rule. It both rejects empty intervals and accepts a real overlap (clipFacts.ts:39–40, a2Shared.ts:140–152,440). For the false conflict, spansOverlap(4.5, 5.5, 5, 5) returns true although [5,5) is empty and the pre-PR CLI comparison returned false. Pending media with unresolved duration is represented as duration 0 (describeProject.ts:220–235), so moving a real clip to [4.5,5.5) can now be refused unless --overwrite is used. Conversely, at 24 fps with an existing same-track clip [0, 20/24 + 5e-7) and a target moved from 1 second to 20f (duration 0.25), the new conflict check returns no overlap because the positive intersection is under 1e-6. It writes target start 0.833333333333; at exact frame 20 (20/24 === 0.8333333333333334), both clips satisfy the interactive half-open visibility predicate and Studio's strict timeRangesOverlap reports a collision (studio/src/player/components/timelineCollision.ts:193–200). The former CLI comparison also rejected this move. Please use actual half-open interval intersection for move/trim conflicts (empty spans never intersect), keep the separate point containment query for duplicate insertion (a2Shared.ts:426–436), and test both cases plus saved-frame visibility. I confirmed the numeric and source predicates, not a full CLI/Studio execution.
Related ripple regression — a clip wholly before insertion can move anyway. studio-server/src/helpers/duplicateElement.ts:63–65 changed its shift condition from start >= at to start >= at - 1e-6. With a same-track candidate [4.9999995,4.9999997) and insertion at 5 of a 1-second clip, the candidate is entirely before the insertion point, and the duplicate point check above does not flag it, but ripple now moves its start to 5.9999995. This silently relocates unrelated authored content; the old condition left it at 4.9999995. Please keep an exact boundary for deciding which clips shift, or preserve before/after classification explicitly, and add a near-boundary duplicate test. This is a source-and-numeric witness, not a full duplicate command run.
The shared Studio overlap comparison itself preserves Studio's former 1e-6 expression and its 13-line diff is behavior-neutral on that path. At review time the exact head is fb979efd07ca8dec6c7c1478f69642aa08c9c630; required CI was still running. These are source-and-numeric reproductions, not a live Studio preview run.
— Review by tai (pr-review)
834a183 to
f3c8a65
Compare
miguel-heygen
changed the title
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed f3c8a65af0892473cfd68fe4b8d1087d7dbb8c29. Changes requested. The earlier frame-boundary serialization defect is fixed: packages/cli/src/timeline/a2Shared.ts:214–216,267–295 saves 20f/30 fps as the exact JS value 0.6666666666666666, so the old clip is out and the next is in at interactive frame 20. Its new non-overshooting trim duration also keeps 4.74→25.74 on the requested side of the boundary. I checked saved HTML against the core runtime half-open visibility predicate, not a live Studio UI.
-
[Blocker — real overlaps still pass CLI conflict checks.]
packages/core/src/clipFacts.ts:39–40applies a 1µs tolerance tospansOverlap, whichpackages/cli/src/timeline/a2Shared.ts:140–152,439–445uses to decide whether move/trim may write without--overwrite. I ran the actual CLI on an existing clip[0, 20/24 + 5e-7)and moved another clip to20ffor 0.25 s: it exited successfully and saved the target at0.8333333333333334; both clips satisfy the runtime's visibility predicate at frame 20, while Studio's exact collision check sees the overlap. Duplicate's point-in-clip check also uses this helper (a2Shared.ts:426–436), so an insertion within a clip's last sub-microsecond can evade the conflict. Keep presentation tolerance separate from exact edit-safety interval and point tests; add these saved-file/runtime witnesses. -
[Blocker — ripple moves clips that were before insertion and erases real gaps.]
packages/studio-server/src/helpers/duplicateElement.ts:63–65treats any start within 1µs on either side as if it met the insertion point. Duplicating at 5 moved an unrelated clip wholly before insertion ([4.9999995, 4.9999997)) to start at 6 in an actual CLI-to-file run. A later clip that genuinely started at1.0000005was snapped to 2 when duplicating a one-second clip at 1, erasing its authored 0.5µs gap. Preserve the exact before/after classification and snap only a proven floating-point seam, rather than any nearby start. Neither new equality-boundary e2e test covers these cases.
Five focused suites passed, 86 tests total, after workspace prerequisites were built in the isolated worktree; no live Studio UI was run. CI reruns were still pending at review time, not the reason for the verdict.
Verdict: REQUEST CHANGES
Reasoning: Exact frame-20 serialization is repaired, but the remaining fuzzy conflict and ripple rules still save real overlaps and relocate authored clips.
— Review by tai (pr-review)
40de305 to
80ce01a
Compare
miguel-heygen
changed the title
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed 80ce01ac5ddf6d973a2d9efc13ccdc914aae64b1. Changes requested. The earlier frame-20 serializer, half-microsecond CLI-conflict, and duplicate-ripple witnesses are addressed at this head. One new runtime/preview mismatch remains:
The floating-point start is active but omitted by the live media index (packages/core/src/runtime/clipWindow.ts:3–7, packages/core/src/runtime/init.ts:2683–2699,2755–2764,2805–2811). At 30 fps, a video beginning at 19.8 + 0.1 has start = 19.900000000000002. After a settled paused seek to 19.8, seek to the real frame 19.9. sameInstant(19.9, start) now makes the clip visible and in-window, but the index's exact endpoint search asks for start <= 19.9, which is false. It was not in the previous in-window set either. The transport passes no clip to syncRuntimeMedia, then records 19.9 as its last synced time; a video with data-playback-start="3" remains visibly at source time 0 instead of seeking to its first frame at source time 3. This is the live indexed path, not the render-capture full-visit path. The new media.test.ts test passes [clip] directly and proves the local-time clamp, but bypasses the index that drops the clip. Align candidate visitation with the new seam predicate (or conservatively visit clips near crossed endpoints) and compare indexed versus full-visit state at this exact 30-fps seam.
A temporary integration regression in the existing init.mediaClipIndex.test.ts fixture reproduced this with the actual sandbox runtime and paused player seek: at 19.9 the indexed path visited 0 clips, left currentTime=0 while the video was visible, and the full-visit control visited 1 clip and set currentTime=3. The next frame converged; remaining paused at 19.9 does not. The targeted regression failed as expected and was removed after the witness; this is not a live Studio run. Existing CLI e2e tests passed 29/29 in the isolated worktree. Studio and player captures is red because the PR body lacks its required Before/After captures, not because this test failed; it needs a separate author-side evidence update.
— Review by tai (pr-review)
72c4bd6 to
e758bd2
Compare
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed #4535 at e758bd2ebe32411e534035d0711bf9aaae9f219f. Changes requested for the remaining zero-duration media collision regression. The previous media-index miss is fixed: packages/core/src/runtime/init.ts:2690–2700 widens both index searches by the shared instant tolerance, and the actual indexed-vs-full paused-seek test passes. The source-media terminal hold has tests as well. I withdraw the media-index blocker at this head.
packages/core/src/clipFacts.ts:45–49 makes spansOverlap report an overlap when one half-open interval is empty but sits inside another. packages/cli/src/timeline/a2Shared.ts:140–152,439–445 now uses it to reject timeline moves. Pending remote media is assigned duration zero by describeProject.ts:220–235. In an actual CLI temp-project witness, a pending video at [1,1) caused timeline move #a 0.5 for a one-second clip to fail with status 2 and #a would overlap #pending at 0.5-1.5; the project was unchanged. The PR merge-base used the strict Math.max(start, candidate.start) < Math.min(end, candidate.end) predicate and allowed this move, so the refusal is introduced by this change. It is not a consistent unknown-duration reservation: moves after the pending start are allowed. Please exclude empty spans from these conflict checks and add the pending-media move regression.
Focused core, CLI-timeline and Studio-overlap suites passed 163/163, 29/29 and 53/53 after building prerequisites. This was a CLI fixture, not a live Studio UI walk; CI is reference, not the basis of the finding.
— Review by tai (pr-review)
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Review at e758bd2e.
Verdict: REQUEST_CHANGES. tai's zero-duration finding reproduces at this head through the real CLI. It also reaches further than the pending-media move. Separately, this PR makes the preview and the exported MP4 disagree on a video clip whose start is a float sum. I checked that in a real render and in a Chrome preview. Everything else tai raised in rounds 1 to 3 is fixed, and each fix is pinned by a test that goes red when the fix is reverted.
What would flip this to an approve:
- Move and trim conflicts stop counting an empty span as an overlap, as the merge base did, and a CLI regression covers the pending-media move. Make the change at the
overlap()call site (packages/cli/src/timeline/a2Shared.ts:140-153) or with a separate point-in-span helper for duplicate, not insidespansOverlap. Duplicate's insertion check passes an empty span on purpose (a2Shared.ts:431). When I madespansOverlapignore empty spans, two duplicate tests went red: "refuses a duplicate that lands half a microsecond inside another clip" and "refuses duplicate insertion inside a spanning clip". - The engine's video start check uses the same rule as the runtime (Blocker 1 below), with a test at a float-sum start.
tai's findings, checked at this head
Round 1 (5325944144 at fb979efd)
- The 12-digit serializer moves frame-exact boundaries. Resolved. Trim writes
String(seconds)(a2Shared.ts:214-217). "trims to frame 20 at 30 fps so that frame shows the next clip and not this one" checks the saved0.6666666666666666and what shows at frame 20. Putting a 12-digit write back on the trim duration turns 3 e2e tests red. On the trim start it turns 1 red. - The shared tolerance counts an empty span as an overlap (
spansOverlap(4.5, 5.5, 5, 5)is true). Not resolved. This is the same defect as round 4 (packages/core/src/clipFacts.ts:45-49). - The 1 µs tolerance lets real overlaps through. Resolved. The tolerance is now
4 * Number.EPSILON * max(1, |t|)(clipFacts.ts:37). With a 1e-6 tolerance swapped back in, 4 tests go red: both "refuses a move/duplicate that lands half a microsecond inside another clip" tests and both ripple tests. - Ripple moves a clip that sits wholly before the insertion point. Resolved (
packages/studio-server/src/helpers/duplicateElement.ts:64-66). With the before-check removed, "duplicates without snapping a clip wholly before the insertion point to the copy" fails. With a 1 µs snap, that test fails and so does "...a clip half a microsecond after it to the copy".
Round 2 (5326088965 at f3c8a65a)
- Real overlaps pass the CLI conflict checks, and duplicate's point check misses them. Resolved. The evidence is the same as round 1 item 3.
- Ripple moves earlier clips and erases a real 0.5 µs gap. Resolved. The evidence is the same as round 1 item 4.
Round 3 (5326337942 at 80ce01ac)
- The media index skips a clip whose start is a float sum. Resolved (
packages/core/src/runtime/init.ts:2690-2693). With the margin set to zero, "visits a clip whose float-sum start is the same instant as the seek, as the full pass does" fails. Removing only the upper margin also fails it. - Missing Before/After captures. Resolved. The body now has them, and
Studio and player capturesis green at this head. That check is reference only, not a gate.
Round 4 (5326484951 at e758bd2e)
-
A zero-duration pending clip blocks a move. Confirmed. I ran
bun run src/cli.ts timeline ... --jsonon temp projects at this head. I then ran the same cases with only theoverlap()predicate ata2Shared.ts:151set back to the base'sMath.max(start, c.start) < Math.min(end, c.end):Case (all on track 0) Head Base predicate pending <video>(remotesrc, no duration) at[1,1),move #a 0.5on a 1 s clipexit 2, #a would overlap #pending at 0.5-1.5, file unchangedexit 0 same, move #a 1(starts on the pending clip)exit 0 exit 0 same, trim #a --end 2on a clip at[0,0.5)exit 2, #a would overlap #pending at 0-2exit 0 move #pending 4into a clip at[3,5)exit 2, #pending would overlap #a at 4-4exit 0 empty sub-composition (no children, no duration) at 1, move #a 0.5exit 2, #a would overlap #sc at 0.5-1.5exit 0 The trim case, the pending clip's own move and the empty sub-composition are new angles on the same defect. Row 2 shows the refusal does not act as a consistent reservation, as tai said.
New findings
Blocker 1. For a video clip whose start is a float sum, the preview now shows one frame the export does not. The engine still uses an exact start at packages/engine/src/services/videoFrameExtractor.ts:2345,2361,2372, and :2202 returns no frame when localTime < 0. The runtime now accepts the same instant (packages/core/src/runtime/clipWindow.ts:3,7).
Setup: video a at data-start="0.1" data-duration="0.2", and a-copy at data-start="0.30000000000000004". That second value is exactly what timeline duplicate #a writes. The source is green for 0.1 s, then red. The project runs at 30 fps, so frame 9 is t = 0.3.
- Head,
hyperframes renderthen ffmpeg reading the centre pixel per frame: frame 9 is RED and frames 10 and 11 are GREEN. The render holdsaat frame 9 and never paints the copy's first source frame. - Head, the built runtime in chrome-headless-shell,
__player.seek(0.3): GREEN, withahidden anda-copyvisible atcurrentTime=0. - Runtime built from the merge base
162de1a6, same seek: RED, withavisible anda-copyhidden. On the base, preview and export agreed.
A function-level probe at 19.8 + 0.1 shows the same one-frame gap: the engine paints nothing at frame 597 while isClipVisibleAt is true.
Divs are fine. The export DOM path already snaps boundaries to the frame (packages/core/src/runtime/timeline.ts:82-88), so for divs this PR brings preview and export into line. The video engine is the one place that did not follow.
Should-fix 1. The composition-end bound still compares exactly (a2Shared.ts:160, and the insertion-time bound at a2Shared.ts:98). Take the body's own layout: a 26.2 s composition and a 6.4 s clip.
timeline move #a 19.8exits 2 withmove would end at 26.200000000000003, beyond composition duration 26.2. Its hint sayschoose a start at or before the latest valid start 19.799999999999997.timeline duplicate #aon a clip at 19.8 lasting 6.4 exits 2 withtime 26.200000000000003 is outside the composition duration. A clip at 24.2 lasting 2 duplicates fine.- The runtime counts the first clip as reaching the end (
clipWindow.ts:22-24).
This predates the PR, but it is the same rule, in the function the PR edits.
Should-fix 2. A split at a clip's float-sum end writes a zero-length clip, and the preview shows that clip until the composition ends (packages/studio-server/src/helpers/sourceMutation.ts:364). The CLI uses this helper, and so do Studio's routes (routes/files.ts:2163,2861).
Setup: a at 19.8 lasting 6.4, b at 26.2, then timeline split #a 786f at 30 fps.
- The split exits 0. It leaves
aatdata-duration="6.4", because of the ms rounding atsourceMutation.ts:325-326. It addsa-2withdata-start="26.2" data-duration="0". - The runtime reads duration 0 as no end (
timeline.ts:79-80). In Chrome at head,a-2is visible at 27, 29 and 35 s of a 40 s composition, paintinga's red over empty time afterbends. split #a 26.1996also writesdata-duration="0", through the rounding alone.- At the same time value,
duplicate --at 26.2treats 26.2 asa's end. - That zero-length row is also exactly the input that trips the round-4 defect.
This predates the PR.
Should-fix 3. Nothing pins the Studio half of the PR. I tried three replacements for spansOverlap, at packages/studio/src/player/components/timelineZones.ts:29 and at timelineStackingSync.ts:103: the old 1e-6 formula, plain <, and an exact sameInstant. In every case the four Studio timeline suites still pass (timelineZones, timelineStackingSync, timelineClipDragCommit, timelineTrackPersistPipeline; 113 tests). That leaves both "Visible change" claims in the body untested: that a 5e-7 overlap now counts in lanes, and that a float-sum meet does not.
Should-fix 4. Studio drag and paste still treat a float-sum meet as an overlap. The sites are packages/studio/src/player/components/timelineCollision.ts:200, reached through isLaneFree (:207-220) and resolvePlacement (:248), and packages/studio/src/hooks/useClipboard.ts:118. Setup: A at 19.8 lasting 6.4 on lane 0.
- Dragging a 2 s clip B to 26.2 with lanes
[0,1]gives{"track":1}. - With a single lane, dropping B back where it already sits gives
{"track":0,"needsInsert":true}, so a no-op drop asks for a new track. - Pasting 2 s at 26.2 goes to track 1.
- The control at 26.3 stays on lane 0 with no insert.
Meanwhile spansOverlap says A and [26.2,28.2) do not overlap. The stacking comment calls the difference from timeRangesOverlap deliberate, but that note was about 1e-6 fuzz, and this PR's title covers edits. This predates the PR.
Nit 1. hyperframes lint rule duplicate_audio_track (packages/lint/src/project.ts:563) warns on two audio clips that only meet on a float sum: Multiple <audio> elements on track 0 overlap (a.mp3 at 19.8-26.2s, b.mp3 at 26.2-28.2s). The same layout with an exact meet (20 + 6.2) gets no warning. This predates the PR.
Nit 2. Nothing pins the saved bytes of move. A 12-digit write of the move start (a2Shared.ts:168) passes all 77 CLI timeline tests. This is the round-1 serializer defect on the one verb with no witness.
Nit 3. duplicate --at <source start> stacks the copy on top of the source, because ripple skips the source element (duplicateElement.ts:59).
- With an exact layout (
aat 20 lasting 6.2,bat 26.2),duplicate #b --at 26.2exits 0 and writesb-copyat 26.2 overb. The same happens on the base. - In the float layout, the base point check refused this by accident (
duplicate insertion at 26.2 falls inside #a). The head now allows it, so the same stacking becomes reachable there.
Nit 4. Very long chains of sums outgrow the tolerance. 0.3 added 49 times is 14.700000000000014, which is 1.42e-14 past 14.7, against a tolerance of 1.31e-14. After 48 × timeline duplicate #a on a 0.3 s clip, move #outro 14.7 exits 2 with #outro would overlap #a-copy at 14.7-16.7. This is unlikely in practice, and it is noted for the record.
Checked
- Test counts at this head match the body: core
src/runtimeplusclipFacts1380/1380 (1373 + 7), CLIsrc/timeline77/77,duplicateElement2,timelineZones23,timelineStackingSync30. - Full suites are green: CLI 3682, Studio 5635, studio-server 776, player 434.
tsc --noEmitis clean in core (both configs), cli, studio and studio-server.- All nine "single mutations each fail a test" claims in the body reproduce: 1 µs tolerance, exact comparison, the old runtime window, no before-check, 1 µs snap, no finite guard, no clamp, no index margin, exact terminal end.
- Surviving mutants, none of which I am asking to pin:
- an exact start in the terminal branch of
isClipVisibleAt(clipWindow.ts:22); - removing only the lower index margin (
init.ts:2692), which by my reading is never needed; - a 1x instead of 2x margin;
- taking the tolerance from the smaller magnitude.
- an exact start in the terminal branch of
- The studio-server suite catches none of the
duplicateElement.tsmutations. Only the CLI e2e tests do. - Tolerance, sums of 2 to 20 two-decimal terms, 200,000 sums at each length: none missed its decimal value, and the worst error was 0.59x the tolerance.
- Tolerance,
a/fps + b/fpsagainst(a+b)/fpsat 24, 25, 29.97, 30 and 60 fps: no misses. - Real gaps (5e-7, and 1.0000005 against 1) stay apart, and tests pin both. The rule is symmetric.
durationUntil(a2Shared.ts:284-288), over 198,198 start/end pairs on a 0.01 grid: it never overshoots. 2,142 pairs cannot land exactly, and none of those has a longer duration that lands exactly.git merge-tree --write-tree origin/main e758bd2eis clean. Main is 7 commits ahead, and none of those adds a clip-window comparison ininit.ts.hardSyncAllMedia(init.ts:4024-4025) does not get the new clamp. In a jsdom probe,play()from a paused seek onto a float-sum start wrote 3.000021 (withdata-playback-start="3"), because the clock had already moved. So it did not land before the first frame.- A fresh-agent adversarial pass raised the render parity, split ghost clip, single-lane drop, duplicate stacking and long-chain cases. I re-ran each one myself before folding it in above.
Not verified
- The Before/After Studio captures in the body. I did not drive Studio in a browser.
- Should-fix 4 in the Studio UI. I called the placement functions directly.
packages/player/src/parent-media.ts:162-194now writescurrentTime = time - start, about -1.7e-15, at a float-sum start. Browsers should clamp that to 0. Read only.- From the adversarial pass, read only:
hyperframes snapshot(snapshot.ts:100) likely uses the engine's exact-start, inclusive-end rule, so it would side with the export. The engine's inclusive end can also paint the earlier of two exactly meeting videos at the boundary frame when that video is on top. That was already true on the base.
— Rames
5b41445 to
b933614
Compare
Fallow audit reportFound 48 findings. Duplication (47)
Health (1)
Generated by fallow. |
59f9d7f to
d0d00ce
Compare
…edits and playback
d0d00ce to
5689ceb
Compare
There was a problem hiding this comment.
Re-reviewed #4535 at 5689ceb58dd839a1bd7f7f6bdc41ff57768489c0: changes requested; no approval. The prior zero-length CLI finding is fixed: edit conflicts require positive shared time while duplicate retains its point-inside-span check. Seven real-CLI fixtures covered pending/empty-clip variants and positive-overlap controls; timeline e2e passed 31/31. Rames's missing incoming video at frame 9 is repaired by the engine start guards and snapshot start check. The remaining issue is the outgoing side of that transition.
Blocking, narrowly scoped — this PR introduces preview/export disagreement at floating-point clip meets. Let video A start at 0.1 and last 0.2, so its end is 0.30000000000000004; B starts at that computed end; inspect frame 9 (t=0.3). At the PR base (162de1a6), the preview showed A and the export frame lookup activated only A. At this head, the tolerant runtime start/end predicates (packages/core/src/runtime/clipWindow.ts:3–8) make A inactive and B active, but FrameLookupTable (packages/engine/src/services/videoFrameExtractor.ts:2317, 2337–2348, 2363–2375) admits both: it adopted the tolerant B start while retaining inclusive A end (globalTime <= entry.end). The injector passes both IDs to syncVideoFrameVisibility (videoFrameInjector.ts:210–237), and both replacement images become visible (screenshotService.ts:879–895) even though the native A video is hidden. Snapshot likewise admits A through its end tolerance (packages/cli/src/commands/snapshot.ts:100–108). With a 50%-alpha B over A, my Chromium CSS-equivalent fixture rendered (128,0,128) rather than (0,0,128); with reversed DOM order, opaque A covered B. These are pixel witnesses, not full-project export runs. Rames separately reports reproducing the mismatch in real renders and snapshot at this head.
Scope correction to my initial review: the runtime's strict half-open end and the extractor's inclusive end both predate this PR. Exact clip meets already disagreed at the base; this PR did not originate that general end-policy defect. For the floating meet above, however, the base preview/export agreed on A, while this head shows B in preview and A+B in export. The new tolerant start behavior exposes the inherited end defect at a previously agreeing boundary, contrary to the PR's preview/export parity claim. My hold is for that newly broadened mismatch, not a claim that every end mismatch is new. The new engine test asserts B present but not A absent; the older shared-boundary test at videoFrameExtractor.test.ts:1138–1155 still asserts both active and says they match the runtime. Please make the outgoing replacement inactive at interior handoffs, or otherwise establish parity at this float boundary, while explicitly testing the intended lone terminal-frame hold (which a global end-policy change could affect).
The Fallow audit is red on a complexity gate; that is not the basis of this code verdict. I did not run a packaged player or full-project export myself. Rames's own approval is separate from this changes request.
— Review by tai (pr-review)
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 5689ceb5, following my REQUEST_CHANGES at e758bd2e.
Verdict: APPROVE. Both things I said would flip my review are fixed, and each fix is pinned by a test that goes red when the fix is reverted. This approval does not open the gate: tai's CHANGES_REQUESTED 5327211546 is live at this head, so the review decision stays CHANGES_REQUESTED until tai clears it. I checked tai's new blocker and reproduced it in a real render and a real snapshot. I read it as a real gap, but one this PR does not create in kind: an exact meet paints the same frame, at this head and on the base (New finding 1). My four should-fixes from last round are still open. All four predate the PR.
Earlier findings
Mine
- Blocker 1, the engine's video start check: resolved. The engine now uses the runtime's start rule at
packages/engine/src/services/videoFrameExtractor.ts:2202-2203(getFrameIndexAtTime),:2317(getFrame),:2346(fresh sweep),:2363(stepping cursor) and:2374(prune). Snapshot uses it too, atpackages/cli/src/commands/snapshot.ts:101-105.- I re-rendered last round's project with
hyperframes render: videoaat 0.1 lasting 0.2, thena-copyat0.30000000000000004. Frame 9 is now GREEN (it was RED ate758bd2e). Frames 10 and 11 are GREEN and frame 12 is RED. A Chrome preview seek to 0.3 is GREEN too. - The test is
videoFrameExtractor.test.ts:951. Making the start exact again at:2202,:2346,:2363or:2374turns it red, each one alone. Making snapshot's start exact atsnapshot.ts:101, or removing the clamp at:104, turns both cases ofsnapshot.test.ts:166red. - Two reverts survive.
:2317survives becauseFrameLookupTable.getFramehas no production caller, only tests. The clamp at:2203survives because the floor's1e-9already puts the tiny negative on frame 0. I am not asking to pin either one. The body says each of "the engine's four start checks" fails a test. That holds for three of the four checks inFrameLookupTable, plusgetFrameIndexAtTime.
- I re-rendered last round's project with
- Should-fix 1, the composition-end bound: not resolved (
packages/cli/src/timeline/a2Shared.ts:160and:98). The results match last round.move #a 19.8(a 6.4 s clip in a 26.2 s composition) exits 2 withmove would end at 26.200000000000003, beyond composition duration 26.2.duplicate #aon a clip at 19.8 lasting 6.4 exits 2 withtime 26.200000000000003 is outside the composition duration. - Should-fix 2, split writes a zero-length clip: not resolved (
packages/studio-server/src/helpers/sourceMutation.ts:364, with the ms rounding at:325-326).split #a 26.2andsplit #a 786fstill writea-2withdata-start="26.2" data-duration="0". This head's new trim rule (a2Shared.ts:269) refuses that shape for trim, so split is now the one CLI verb that still writes it. - Should-fix 3, nothing pins the Studio half: not resolved.
git diff e758bd2e 5689ceb5 -- packages/studiois empty. The newclipFacts.test.ts:85pinsspansOverlapfor a zero-length clip. Nothing pins the two Studio call sites,packages/studio/src/player/components/timelineZones.ts:29andtimelineStackingSync.ts:103. - Should-fix 4, Studio drag and paste: not resolved. These sites are unchanged:
timelineCollision.ts:200(reached throughisLaneFreeat:218andresolvePlacementat:248) andpackages/studio/src/hooks/useClipboard.ts:118. - Nits 1 to 4: unchanged. I re-ran two of them.
- Nit 2: a 12-digit write of the move start (
a2Shared.ts:168) still passes all 79 CLI timeline tests. The new pending-media move test expects"0.5", and a 12-digit write produces that too. - Nit 3:
duplicate #b --at 26.2still writesb-copyon top ofb.
- Nit 2: a 12-digit write of the move start (
tai's
- Round 4, a zero-length clip blocks edits (5326484951): resolved. Move and trim conflicts now need shared time (
a2Shared.ts:151, usingspansShareTimeatpackages/core/src/clipFacts.ts:52-59). Duplicate keeps its own point check (a2Shared.ts:431, usingisInsideSpanatclipFacts.ts:61-62).spansOverlapis unchanged, so the lane rule still gives a zero-length clip its own lane.- All five CLI rows from my last table now exit 0 and write the file, the same as the base predicate. The duplicate refusals still hold. tai also confirmed this with seven fixtures.
- Each revert turns a test red:
- the lane rule back at
:151turnstimeline.e2e.test.ts:404red; - a zero-length span as the duplicate point at
:431turns:420and the half-microsecond duplicate test red; - dropping the one-step trim check at
:269turns:390red.
- the lane rule back at
New findings
1. tai's boundary blocker (5327211546): I confirmed it. It comes from the export's existing end rule, not from a float rounding miss.
The code:
- The export keeps a video active through its end, inclusive:
videoFrameExtractor.ts:2347(started && globalTime <= entry.end),:2366, and the prune at:2374(globalTime > video.end). - The injector passes every active id (
videoFrameInjector.ts:210,:237), andscreenshotService.ts:883shows each one's replacement image. - The preview hides a clip at its end, float sums included:
packages/core/src/runtime/clipWindow.ts:7-8, reached throughpackages/core/src/runtime/timeline.ts:89. - Snapshot uses the same inclusive end (
snapshot.ts:101) and the same visibility call (snapshot.ts:578).
I reproduced it with hyperframes render and read the centre pixel at frame 9 (t = 0.3). The source is green for 0.1 s, then red. The table compares video A with the copy B. "Base export" means this head with videoFrameExtractor.ts restored from 162de1a6, because that file decides which video image shows. "Base preview" means the runtime built from 162de1a6.
| Frame 9 layout | Base preview | Base export | Head preview | Head export |
|---|---|---|---|---|
| Float meet, B after A | RED | RED | GREEN | GREEN |
| Float meet, B after A at 50% opacity | RED | RED | 0,64,1 | 124,64,0 (A under B) |
| Float meet, B before A in the DOM | RED | RED | GREEN | RED |
| Exact meet (A at 0 lasting 0.3), B at 50% | not run | not run | 0,64,1 | 124,64,0 |
| Exact meet, B before A in the DOM | GREEN | RED | GREEN | RED |
hyperframes snapshot --at 0.3 at this head matches the export: RED with B before A, 125,63,0 with B at 50%, and GREEN with B after A.
How I grade it:
- The end rule treats floats and exact values the same. At this head, a float meet paints the same frame as an exact meet, in both the preview and the export (compare rows 2 and 4). An inclusive end with the same-instant tolerance would give the same answers. So this is not a sibling of Blocker 1. Blocker 1 was a float sum being treated differently from its exact value.
- It predates the PR for exact meets. On the base, the exact meet with B first in the DOM already renders RED while the preview shows GREEN (row 5). The base test at
videoFrameExtractor.test.ts:1138-1155pins both clips as active at an exact shared boundary. Its comment says "The runtime shows both at the shared instant", but that was not true on the base either: the baseisInClipWindowistime >= start && time < end, and the base runtime hides A at 0.3 in row 5. The comment atvideoFrameExtractor.ts:2336-2340makes the same claim. - It is not limited to shared boundaries. At a lone clip end, the export draws one frame that the preview does not. B ends at 0.5 with nothing after it: export frame 15 is RED and the preview at 0.5 is BLACK, for float and exact values alike.
- What this PR adds is that float meets inherit the gap. On the base, float meets agreed in every layout: both paths drew A at 0.3, because the base preview also treated the float meet as a one-step overlap. At this head they agree only when B covers A.
Fixing this changes the export's frame at the end of every video that meets another one or just ends, whether the times are floats or exact. That is a separate decision from this PR, so I am not holding a block on it. That is my severity call; tai's block stands on its own. Two asks either way:
- Scope the body's "so the export and a snapshot show what the preview shows" to the start. The export still draws the outgoing video at a shared boundary.
- If the end rule does change in this PR, make the same decision for the lone end (frame 15). Rewrite the test at
:1138rather than keeping it, because its premise is false.
2. Fallow audit is red because of a complexity gate, not a defect. I ran fallow@2.75.0 audit --base origin/main locally and it fails. The one finding this PR introduces is finishTrim at a2Shared.ts:259: CRAP score 31.6 against a threshold of 30, cyclomatic 10. This head's || sameInstant(nextStart, nextStart + nextDuration) at :269 raised the cyclomatic count from 9 to 10. The 47 duplication rows are inherited: 21 clone groups, none introduced. Fallow was green at e758bd2e. The check itself is right. To clear the gate, move it into a small helper, or use the repo's // fallow-ignore-next-line complexity as at packages/producer/src/services/hdrCompositor.ts:169.
Checked
git diff e758bd2e 5689ceb5in full. The branch was squashed to one commit on the same base,162de1a6, ande758bd2eis still in the object store. I also readclipFacts.ts,clipWindow.ts,a2Shared.ts,FrameLookupTableand the snapshot resolver in full.- I grepped
packages/{engine,core,player,producer}/srcandpackages/cli/src/commandsfor<,<=,>and>=against clip starts and ends.- At this head, the only compares that still treat a float sum differently from its exact value are the CLI, Studio and lint sites filed above, plus
packages/core/src/runtime/init.ts:2439. - That line is
currentTime < endin the audio-group unmute check. At the exact end of a float-sum clip it reports the member as still to play, which costs one extra capture. I am not filing it. - These treat both kinds of time the same: HDR frames (
hdrCompositor.ts:164rounds), the player (packages/player/src/parent-media.ts:162usesisInClipWindow), distributed renders (planV2.ts:418andrenderChunk.tsbuild the sameFrameLookupTable), and snapshot's end tolerance.
- At this head, the only compares that still treat a float sum differently from its exact value are the CLI, Studio and lint sites filed above, plus
- Tests:
- Core: 3208/3208 (
src/runtime1373,clipFacts11; the body's 7 is stale). - Engine: 1954 passed and 8 failed, all from the environment. Two need a Chrome build that puppeteer does not have here. Four read
hdr-photo-pq.png, which is an LFS pointer here. Two break on the long temp path: the audioMixer command-line length and the fileServer Unix socket. The PR touches none of these files. videoFrameExtractor.test.ts161/161, CLIsrc/timeline79/79,snapshot.test.ts40/40.
- Core: 3208/3208 (
tsc --noEmitis clean in core (both configs), engine and cli.git merge-tree --write-tree origin/main 5689ceb5is clean at maine0e6442d. Main is 10 commits ahead. Those commits touchinit.tsandtimeline.tsbut add no clip-window compare.- CI at this head (reference only): 57 success, 7 skipped, 1 failure (Fallow).
- The After capture was recorded at
e758bd2e, and it still describes this head's preview. The only change toclipWindow.tssince then renameshasStartedtohasClipStartedand exports it.
Not verified
- The exact meet with B at 50% on the base. I rendered it only at this head; for exact values the code path is the same on the base.
- How many regression-shard baselines would move if the engine's end rule changed.
- The distributed and HDR render paths at a float-sum start. I only read them.
- Studio in a browser. Should-fixes 3 and 4 still rest on direct function calls from last round.
— Rames
This branch has not been deployed
3 participants
What this fixes
Two clips that meet should meet, not overlap, even when their times do not add up exactly in floating point. A clip at 19.8 lasting 6.4 ends at 26.200000000000003, so:
hyperframes timeline trimrefused to trim a clip starting at 26.2, as if the two overlapped, unless--overwritewas passed;Now there is one rule,
sameInstantin@hyperframes/core/clip-facts: two finite times a few floating-point rounding steps apart, scaled to their size (about 1e-14 s at 26 s), are the same instant. Nothing wider counts, so a real overlap of half a microsecond is still an overlap, and a clip with no known end (Infinity) is never mistaken for one ending now. The rule is used by:spansShareTime), so a zero-length clip such as pending remote media never blocks a move, as before this change;duplicate's insertion point must be strictly inside a clip to be refused (isInsideSpan); and a trim that would leave a clip one rounding step long is refused like a zero-length one;isInClipWindow, so a clip ending on a float sum hands that instant to the next clip, and the preview and the saved file never disagree; a media clip starting on a float sum is seeked to its first frame, never a hair before it, and the live media index visits a clip whose start or end is the same instant as the seek, so a paused seek onto that start reaches the clip instead of skipping it; a video ending on a float sum at the composition's end still holds its last frame there;duplicate's push-along step: clips at or after the insertion point move by the copy's length, and a clip that met the insertion point moves to exactly the copy's end; clips before it stay put;spansOverlap); a zero-length clip inside another still gets a lane of its own, as before;hyperframes snapshot: a video starting on a float sum is drawn from its first frame at that instant (hasClipStarted, the runtime's own start rule), so the export and a snapshot show what the preview shows.Times are written exactly as computed, as before (
20fat 30 fps stays0.6666666666666666). A trim with--endpicks the longest duration that, added to the start, does not end past the requested time: exact when a number lands exactly (26.2 to 29 writes2.8000000000000007, which adds to 29), otherwise the nearest number just before it (4.74 to 25.74).Visible change
In the preview, a clip that meets the next one on a float sum no longer shows for one instant together with it. Studio's lanes treat clips that overlap by more than a rounding step (previously more than 1 microsecond) as overlapping.
Tests
clipFacts.test.ts: 19.8 + 6.4 and 26.2, 0.1 + 1.1 and 1.2, and hour-scale sums are the same instant; 20/24 + 5e-7 and 20/24, 1.0000005 and 1 are not. Meeting clips do not overlap; clips sharing half a microsecond do.clipFacts.test.tsalso: a zero-length span never shares time with another but still overlaps one for a timeline lane; a time on either edge of a span, within rounding, is not inside it.clipWindow.test.ts: at 26.2 only the clip starting there is in its window; a clip ending 5e-7 later is still in its window; a clip with no known end is in its window.videoFrameExtractor.test.ts: a copy starting at 0.1 + 0.2 is drawn from its first frame at 0.3, on a fresh sweep, a stepping sweep and a direct lookup.snapshot.test.ts: the same start, and 19.8 + 6.4 at 26.2, sample frame time 0.media.test.ts: a paused clip starting at 19.8 + 0.1 is seeked to exactly 0 at 19.9.init.mediaClipIndex.test.ts: after a paused seek to 19.8, a seek to 19.9 leaves a video starting at 19.8 + 0.1 (playback start 3) at source time 3, the same state as a pass that visits every clip.media.test.ts: a paused video from 19.8 lasting 6.4, in a 26.2-second composition, holds its last frame (source time 6.4) at 26.2.timeline.e2e.test.ts, each reading the saved file and deciding visibility with the runtime's ownisClipVisibleAt:trim --end 20fand--start 20fshow only the second clip at frame 20;duplicate --at 1, each landing half a microsecond inside another clip, are refused and leave the file unchanged;duplicateon three meeting layouts places the copy at the first clip's end and the next clip at the copy's end, with exactly one clip showing at each boundary; a clip wholly before the insertion point stays put, and a clip half a microsecond after it moves by exactly the copy's length.Results: core
src/runtime1373 andclipFacts7, studio-serverduplicateElement2, StudiotimelineZones23 andtimelineStackingSync30, CLIsrc/timeline79 andsnapshot.test.ts40, enginevideoFrameExtractor.test.ts160 (one real-ffmpeg case also fails on the base commit on the test host);tscclean in core, engine, studio-server, studio and cli. Single mutations each fail a test: a 1 microsecond tolerance (the half-microsecond move and duplicate are saved), exact comparison (the brief's trim is refused), the old runtime window (both clips show at 26.2), no before-check in the push-along step (a clip before the insertion point moves), a 1 microsecond snap (a clip half a microsecond after the insertion point lands on the copy's end), no finite-time guard (a clip with no end is never in its window), no clamp on the clip's local time (the float-sum clip is not seeked), no margin in the media index's endpoint search (the paused video stays at source time 0), an exact end in the terminal-frame check (the video at the composition's end is left at source time 0), the lane rule used for edit conflicts (the move over pending media is refused), a zero-length span used as the duplicate point (a duplicate inside a clip is allowed), an exact trim length check (a one-step clip is written), each of the engine's four start checks and snapshot's start check made exact again (the float-sum copy is not drawn), and no clamp on snapshot's local time.Before
Studio's preview on the base commit, seeked to 26.2 (frame 786 at 30 fps). Clip A starts at 19.8 and lasts 6.4, clip B starts at 26.2: both paint.
After
The same project and seek on this branch (recorded at
e758bd2e; later commits change edit checks, the export engine and snapshot, not this preview): only B paints. At 26.1 and 26.3 both commits show only A, then only B.