Repository navigation
fix(core): a preview video short of data holds the film instead of looping snippets - #6974
Conversation
There was a problem hiding this comment.
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
Commentscheck: the comment ratchet fails because comment lines were added: 5 ininit.ts(14.2%, 903→908 lines) and 1 inmedia.ts, at thekeepLateMediadoc 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.
Edit accuracy: accurate 271 (base branch 271), smooth 114 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
There was a problem hiding this comment.
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.
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 inmedia.ts):HAVE_FUTURE_DATA, which can mean one frame. It now needsHAVE_ENOUGH_DATAor 1 s buffered ahead.waiting. It now applies only to WebKit.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 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/runtimepass, and the new test file passes 3 runs in a row.What I did NOT exercise
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.