Repository navigation
fix(core): Safari loads a later clip from its in-point, not from the file's front - #7091
miguel-heygen wants to merge 2 commits into
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blocker · 1 important · 1 nit · head 726baee2
- blocker: this is a fix PR, but the body has no
fails without the fix:line naming a test that goes red without the change. The new WebKit test only fails today becausepreloadFromInPointdoesn't exist yet. Name a test that checks the behavior, for examplepreloadis stillmetadatawhenload()runs and only turnsautoafter the seek. - blocker: the
Commentsworkflow fails its comment ratchet:preloadMedia.tscomment share goes from 3.2% to 4.0% (2 lines added, and the share may only go down). Cut the docblock or move it into a name.
Scope
Read the full diff, prepareUpcomingMedia, and the prefetch-slot path in init.ts (startPrefetch, onPrefetchSettled, preloadNearPlayhead). The device A/B on the phone is still unchecked in the test plan. CI on this head was still running when I reviewed, apart from Comments, which failed.
…e-armed or no longer due
| return; | ||
| } | ||
| if (media.preload === "none") media.preload = "metadata"; | ||
| if (media.readyState === 0 && media.networkState !== 2) media.load(); |
There was a problem hiding this comment.
blocker: in real WebKit this path never seeks and never reaches auto. A preload="none" clip with a src sits at networkState === 1 (IDLE). load() then queues an abort (WebCore prepareForLoad, and the HTML spec too), and that abort fires after prepareUpcomingMedia registers its listeners. The abort → cancel handler removes the loadedmetadata listener before the header lands.
I checked it in Playwright WebKit 2370 by running this head's preloadFromInPoint (esbuild bundle) on <audio preload=none data-media-start=120 src=…> at networkState=1. After 3 s: preload=metadata, currentTime=0. The event order was abort → loadstart → suspend → loadedmetadata, and the pending listener had already been cancelled. jsdom never fires abort, so the unit tests miss this.
Fix: let an abort that comes before this load's loadstart leave the pending listener in place, or attach the listeners after loadstart. Then add a test that dispatches abort before loadedmetadata.
|
Closing: two iPhone A/B runs of this change against main showed no effect on the long single-file film (hold at the second cut 21.6 s vs 20.1 s median, about 1.25 GB pulled either way, large ranges still starting at 0 to 3 MB). iPhone Safari does not honor the header-then-seek-then-read order this relies on, so the cause is not what this assumed. |
What
On Safari, a clip due later whose in-point is deep in its file now loads its header first, seeks to the in-point, and only then reads. Before, it was set to read at once, starting from the front of the file.
Why
A 70 s film cut from one 76 MB file pulled 1,097 MB through 283 requests on an iPhone. Every element released for loading opened an open-ended range near the start of the file (0.4 to 0.8 MB) and pulled about 31 MB before WebKit dropped it for the seek to its real in-point (up to 63 MB in). At 7.2 s of the film six of these started together, and at about 10 MB/s that is the 20 s hold at the second cut. The 3-take film cut from a 12 MB file never showed it.
Related work
Follows #7012 (one connection per file for the take on screen and the next); this changes what the next take does once it is released, not which clips are.
How
preloadFromInPointinpreloadMedia.ts, called fromstartPrefetchininit.ts. On WebKit, for a clip whose media start is above 0:preloadgoes tometadata, the existingprepareUpcomingMediaseeks to the in-point when the header lands, and thenpreloadgoes toauto. A clip at the front of its file, and everything off WebKit, takes the old path unchanged. I tried and dropped capping a playing element's read-ahead (no effect on the phone) and one prefetch slot at a time (breaks #7012's on-screen plus next take).Test plan
fails without the fix:
preloadMedia.test.ts> "loads its header only, seeks to the in-point, and only then reads" (red when the deep-clip branch is switched off, so the clip goesautobefore the seek) and "reads on once its header lands even if it is no longer due, or was re-armed meanwhile" (red when the bail-out path no longer raisespreloadtoauto).auto; and the old path for a clip at the file front and off WebKit (preloadMedia.test.ts). Wholesrc/runtimesuite: 117 files passed, exit 0.Size
A lone change of about 60 lines in the single path that starts a later clip's load; nothing else open can carry it.