Skip to content

fix(core): a preview video short of data holds the film instead of looping snippets - #6974

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/preview-stall-no-seek-loop
Nov 2, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
fix/preview-stall-no-seek-loop

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What changes for people

A preview whose video can't get data fast enough used to play about 0.2 s, freeze for half a second, and repeat for as long as it stayed short of data. It now holds the film until it has real data ahead, and it reports that hold as choppy, so an app can switch to a lighter copy.

Four causes, one place (packages/core/src/runtime/init.ts, plus the sync pass in media.ts):

  • Holding and resuming sought the starved video onto the film even when it was only about 70 ms late. A seek flushes the decoder and its buffer, which caused the next starve. A video under 0.5 s late now keeps its lag and rate steering closes it; a bigger lag still lands on the film.
  • A hold ended on HAVE_FUTURE_DATA, which can mean one frame. It now needs HAVE_ENOUGH_DATA or 1 s buffered ahead.
  • The buffered-range reading added for WebKit also answered for Chromium, so the clock ran seconds past a video that was already waiting. It now applies only to WebKit.
  • A hold counted as a pause, which had two effects: the playback health window was cleared, so it never read as choppy, and a later clip armed for the next cut was released and reloaded on each hold.

fails without the fix: init.buffering.test.ts, "a video a little behind the film is not sought when it starves or when it resumes", "waits for real data ahead", "the film waits for a playing video stuck waiting, even with the bytes buffered", "a hold for data does not reload a later clip", "a film held for data reads as choppy, so the app can lower it".

What I measured

A real Mac (M-series, 24 GB) running a released desktop build with only the runtime swapped, repo-free fixture: two 4K HEVC clips at 80 Mbps, 120 s film, network throttled to 3 MB/s, one run each, 230 s window.

Before After
Video seeks 235 4
Film played 77 s of 120 120 s, done at 138 s
Film speed, from 20 s on 0.28x 0.98x
Steps down to the lighter copy never at 20 s

Before never stepped down because the health window kept clearing. 3 MB/s can only carry about 0.3x of this file, so the speedup is the lighter copy, reached because holds now read as choppy.

The speed is sampled every 10 s. The after run has one dip to 0.74x at 50 s, where the app climbed back to Full, starved and came down. That climb is the app's decision, not this runtime's.

All 2,700+ tests in src/runtime pass, and the new test file passes 3 runs in a row.

What I did NOT exercise

  • Safari or WebKit playback. The WebKit branch is covered only by a unit test with a stubbed user agent.
  • A slow source that is not network throttled.
  • Captures: no Before and After clip attached. This is a timing change, measured in the table above.
  • A film starting with a long first load also steps down now, after about 4 s of holding. That follows from the change, and I did not measure it separately.
  • On the devbox, 6 tests outside the runtime fail in src/ (index.test.ts, deterministicFonts-aliasSupplement, pageBuild.golden). Load errors in the same run name workspace packages that weren't built there. I did not confirm that for each test; CI is the check.

Size

3 files, about 180 lines added, most of them tests.

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE · 0 blocker · 1 important · 0 nits · head 509d4836

The fails without the fix: line holds. I reverted init.ts and media.ts to the base and all 5 named tests in init.buffering.test.ts failed; with the fix, 12/12 pass. All of src/runtime passes (2709 tests) with the fix, in 2 of 2 runs.

  • important — red Comments check: the comment ratchet fails because comment lines were added: 5 in init.ts (14.2%, 903→908 lines) and 1 in media.ts, at the keepLateMedia doc comment. It is not a required check, but the PR body doesn't mention it. Cutting the added comments or folding them into names turns it green. #6973 hits the same ratchet.
Scope

I read the full diff, plus the transport tick, haltPlayback/play/pause handling of bufferingFor, and the strict/hard sync gates in syncRuntimeMedia. Strict sync can't seek a held video again during the hold: while the clock is paused, sync runs only for hasRunningTimedMedia(), and a halted video is paused. pause clears bufferingFor, so the "playing" health reading and arming stop when the user pauses. I did not run Safari/WebKit, and neither did the author. Required CI was still running at review time.

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 114 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (2)

  • seqplainkeys-none-px-r0-nested-z100: tracking 0.07, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.07, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.07, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true
  • seqplainkeys-none-pct-r0-root-z100: tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE · 0 blocker · 0 important · 0 nits · 509d4836 → 9071ff3b

The new commit only deletes 4 comment lines in init.ts and media.ts. The code is identical to 509d4836, which I approved. The Comments check is now green, which resolves the one finding from that review. Required CI was still running on this head at review time.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit 8519f28 Nov 2, 2026
98 checks passed
@miguel-heygen
miguel-heygen deleted the fix/preview-stall-no-seek-loop branch November 2, 2026 04:30
@heygengenesis heygengenesis Bot mentioned this pull request Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant