Repository navigation
fix(core): skip a video the browser can't decode instead of freezing the film - #7355
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 · 1 blocker · 1 important · 0 nits · head 6cca23b9
The 5 s header rule can't tell an undecodable clip from one the browser simply refused to start, and the codec-map branch can't fire with any map the server produces. Details inline.
Scope
Read all 4 files, plus the callers of isUnplayable (media.ts sync and primeMediaForLaterStarts, init.ts isPlayableTrack), maybeProxyProactively, and the codec map producer (studio-server/src/helpers/mediaCodecMap.ts, mediaProxyPreview.ts). CI was still running at review time. The fails without the fix: line is present.
| } | ||
| } | ||
|
|
||
| function stoppedAtMetadata(el: HTMLMediaElement): boolean { |
There was a problem hiding this comment.
blocker: a clip whose play() was rejected with NotAllowedError meets this test too. On iOS the element sits at HAVE_METADATA + NETWORK_IDLE until a play is allowed. The 50 ms retry never resets firstPlayAskedAt, and loadeddata never fires, so 5 s later the clip counts as unplayable. Nothing clears that until emptied. Where the player doesn't promote to parent proxies (shouldPromoteMediaAutoplayFallback is false for the slideshow player), this happens to an unmuted clip that wasn't primed. The user's next tap can't save it, because primeMediaForLaterStarts (L225) skips isUnplayable elements and the sync stops calling play(). On main, the next tap primes it and it plays.
Fix: forget the ask when the browser refuses it, e.g. firstPlayAskedAt.delete(el) in the play().catch when name === "NotAllowedError" (L865), or start the clock only on a play() that didn't reject. Please add a test: a rejected play at header, 5 s, and isUnplayable stays false.
There was a problem hiding this comment.
Fixed in 12cfb9b: a NotAllowedError from play() now forgets the ask (the rejection handler deletes it), so a clip waiting for a tap restarts its 5 s on every refused retry and keeps being primed. Added keeps waiting on a clip at its header whose play() the browser refused, which fails with that reset removed.
| swapToProxy(el, entry, "proactive"); | ||
| return; | ||
| } | ||
| if (entry?.representativeMime && canPlay === "") noteCodecRejected(el); |
There was a problem hiding this comment.
important: no real map reaches this line. scanProjectMedia only puts browserHostile facts in __HF_MEDIA_CODEC_MAP__ (mediaCodecMap.ts:438), and representativeMime is only non-null for hostile codecs (codecFactsFor). So any entry with a MIME the browser rejects takes the swapToProxy branch above and returns. The new test uses browserHostile: false with a VP9 MIME, which the server never emits. That makes the PR body's first case ("the codec map says so") and its fails-without-fix claim for that test hold only for a synthetic input. On a real iOS VP9 map entry, the clip is proxied, as it is on main. Either drop this branch and its test, or build the test fixture from codecFactsFor/scanProjectMedia output and show the input that reaches it.
There was a problem hiding this comment.
Agreed, and checked: scanProjectMedia keeps only browserHostile entries and codecFactsFor sets representativeMime only for hostile codecs, so that line could not run with a real map. Dropped the branch and its test in 12cfb9b; mediaProxy.ts and its test are back to main. The header rule alone is the fix.
6cca23b to
12cfb9b
Compare
There was a problem hiding this comment.
Verdict: APPROVE · 2 resolved, 0 open, 0 new · 6cca23b9 → 12cfb9b6. A NotAllowedError now clears the ask, so the 5 s clock restarts on the next retry. The new refused-play test goes red without that reset. The codec-map branch and its synthetic test are gone, and maybeProxyProactively matches main again.
What
A video the browser stopped loading after its header no longer freezes the whole film in preview. The runtime now skips such a clip, plays the rest, and logs one
console.warnnaming it.A clip counts when, 5 s after it was first asked to play, it is still at
HAVE_METADATAwithnetworkStateNETWORK_IDLEand has never had a frame of its current source. A clip that is still loading (NETWORK_LOADING) never skips, however long it takes. A clip whoseplay()the browser refused (NotAllowedError, waiting for a tap) never skips either: each refusal restarts its 5 s.Why
On iOS Safari (iPhone 17 Simulator, iOS 26.5), a film with VP9 WebM clips never played: the playhead stopped at about 0.04 s and stayed. That Safari answers
""forvideo/webm; codecs="vp9", yet with a plainsrcit reads each WebM's header (480 px wide, no error), setsnetworkStateto idle and never delivers a frame. The runtime only treated an element as unplayable on anerrororNETWORK_NO_SOURCE, so it waited on those clips forever and held the whole film.A typed
<source>the browser rejects is already dropped by the browser (NETWORK_NO_SOURCE), so it needed no change. The gap is a source with no type:canPlayType("video/webm")answers"maybe"there, so the file type alone can't tell.How
isUnplayableinpackages/core/src/runtime/media.ts, the check every playable-or-not decision routes through (the transport's buffering hold, the play() guard, the iOS tap priming), adds the header case and warns once per element.markPlayRequestedrecords when an element was first asked to play;loadeddatamarks it as having had a frame andemptied(a new source) resets it. ANotAllowedErrorfromplay()forgets the ask, so a clip waiting for a tap keeps being primed and retried as on main.Test plan
play() storm guard (unplayable elements), all on fake timers:skips a clip whose fetch stopped at its header 5 s after it was asked to play: not at 4999 ms, yes at 5000 ms, one warning naming the clip.keeps waiting on a clip still loading at its header 5 s after it was asked to play: 60 s, no skip, no warning.keeps waiting on a clip at its header whose play() the browser refused: aNotAllowedError, then 5 s, no skip, no warning.isUnplayablereduced to main's rule, the header test fails; with theNETWORK_IDLEcondition removed, the still-loading test fails; with the refusal reset removed, the refused-play test fails.media.test.tsandmediaProxy.test.tspass 3 runs in a row; the wholesrc/runtimesuite (120 files, 2861 tests) passes on Linux; coretsc --noEmitclean.heavy-alpha-webmfilm, 3 plays at the first head: before, the playhead held at 0.03 to 0.07 s in every play; after, it reaches 10.0 of 10.0 s each time, with one skip warning per WebM clip. The later change only adds the refusal reset (the Simulator run allows autoplay, so noplay()was refused) and removes a branch that never fired there; the Simulator run was not repeated.The film still waits 5 s at each start where such a clip falls due (here at 0 s and about 3 s) before skipping it.
Size
Small on purpose: one shared check gains one case, the play() rejection handler resets it on a refusal, and three tests.
Size: a lone fix. A video the browser cannot decode freezes the whole film, and no open PR in the player can carry it.