Repository navigation
fix: four regressions in main (headers, archives, fragmented MP4) and two subtitle bugs - #52
Merged
Merged
Conversation
declarativeNetRequest's urlFilter has no escape character: Firefox's CompiledUrlFilter splits it at `*` and matches every other character, a backslash included, literally. So escaping a URL's `*`, `^` and `|` (7d9053d) made the header rule require a backslash no URL has, and a stream whose URL contains a `*` - an Akamai `acl=/*` token, say - lost its Referer and Origin. Unescaped, those characters can only widen the match, never exclude the URL itself. header-rules.e2e.mjs checks, on the installed extension, that the server sees the Referer for a plain URL, one with a `*` and one with `^` and `|`; the unit test pins the filter to the URL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
httpGetLarge fetches the file in 250 MB chunks as parseFSA reads it, but the object URL was revoked (d4d533a) once the first two chunks had been requested. Every later chunk failed, so any .fsa archive over 500 MB could not be opened. FastStreamArchiveUtils.parseFSAFile now owns the URL and revokes it after parsing. An unreadable file now ends in "archive failed" too, instead of an uncaught error. The chunk size is FastStreamArchiveUtils.fileChunkSize, which archive-roundtrip.e2e.mjs lowers to 64 KB so a small archive spans many chunks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
To keep live streams moving, #49 removed every HLS playlist and DASH MPD from the download store right after loading it. But "dump buffer" writes the store into the .fsa archive, so archives had no manifest, and a player opened from one had to fetch it again - impossible offline or once the URL expired. The seek preview, which shares the store, also downloaded every playlist a second time. Now a player's first load of a manifest may come from the store and leaves it there; each player remembers which it has loaded (loadedManifests), and only a second load by the same player - hls.js or dash.js refreshing a live stream - drops the stored copy first. Unit tests cover both sides (another player gets the stored copy; the same player's reload downloads it again). archive-roundtrip.e2e.mjs plays HLS and DASH, writes the archive in memory, drops it on the player, and checks it plays again with no manifest from the network. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
checkEndOfStream ended the MediaSource once every sample mp4box knew of was appended. A fragmented file's samples become known one moof at a time, so that also held mid-file whenever a 1 MB range ended between two fragments (measured with mp4box 2.4.1: true after a cut at or a few bytes into any moof). The MediaSource then cut the duration to what was buffered and the video stopped early. For a fragmented file the range holding its last byte must now be in as well. mp4-fragmented-end.e2e.mjs builds that case - a `free` box moves the fixture's second moof to 8 bytes before the first range ends - and holds the second range back: the stream must stay open, the duration whole, and the video play to its end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c4d765d split cues at a line of spaces or tabs as if it were empty, so in a cue like "Top / (spaces) / Bottom" the text below that line became a block of its own with no timestamp and was dropped. ffmpeg and VLC keep it as part of the cue, as upstream did. Cues are now split only at empty lines. A line of spaces between two cues still ends the first: d3fec9b starts a cue at its timestamp line, and the trailing whitespace line is trimmed there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A SubRip cue with nothing under its timestamp becomes a cue with empty text, and vtt.js's convertCueToDOMTree returns null for it. renderSubtitles appended that null, and appendChild(null) throws. While such a cue was on screen, every timeUpdated() stopped there, so a subtitle track after it showed nothing and the skip segments were not updated. The cue is now skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MP4Player set the file's length from the first answer's Content-Range only after handing the data to mp4box, and parsing the moov runs onMetadataParsed, which then works the length out from the samples known so far. For a fragmented file those are only the fragments in the first range, so the ranges after it were never made. The end-of-stream check 066d962 added took the file to be complete there, and the stream was ended after the first range. The length is now read before the data is parsed. A server that gives no total (no Content-Range, or `bytes a-b/*`) still gets the estimate from the sample table, as before. The fragmented-end spec had asked for the whole duration while the last range was held. An empty_moov file, as ffmpeg writes it, has no mehd box, so its duration is what has been parsed so far and grows with each range. The spec now checks what the bug did: the stream is not ended early, and the video plays to its end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dropped on the player still showing the same video, the archive's source is taken for the one already in the sources list, which is not marked as loaded from an archive. Opening the archive later, as it is used, means a fresh page, so the spec now carries the archive over to an empty player page and drops it there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 160 s fixture cut into fragments looked 10 s long before the length came from Content-Range (the previous commit), and the rest was never loaded. The note it replaces said such a file stopped after about 3 s. The limit that is left - past its first 30 ranges, 30 MB, MP4Player cannot pick a range to load in a fragmented file - is measured and noted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test sought to 1 s and then paused, with both cues on 0-5 s. In the full verify the video had played on to 8.33 s before the pause took, so no cue was on and the second track showed nothing. Both cues now span the whole 10 s video, and the test only pauses: whatever the time is, the cues are on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nawid3333
added a commit
that referenced
this pull request
Sep 29, 2026
…s lost when saving (#53) * fix(mp4): play a fragmented MP4 past its first 30 MB A fragmented file's samples become known one moof at a time, as its ranges are parsed, and mp4box adds them to the track's sample list. MP4Player made its keyframe index, which picks the range to load, and the fragments' times only once, when the metadata was parsed. So it kept picking ranges from the start: it loaded the first maxFragmentsBuffered (30) ranges, 30 MB, and could not play or seek past them. Measured with a 480 s, 51 MB file. The index and the times are now made again whenever a range has added samples. A fragmented file is still read ahead by ranges, not by the 30 s maxBufferLength: where a time is in it is known only once the ranges before it are parsed, and that is how its duration and a far seek come to be known. That keeps a file of 30 MB or less loading whole, as before. With the times known, the ranges played through are let go, as for any other MP4. mp4-files.e2e.mjs plays the long fixture three times over (480 s, 51 MB), seeks past its first 30 ranges, and back to a part whose ranges were let go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(player): keep a replaced video's progress lookup off the next one loadProgressData() looks up the video's remembered time with two PBKDF2 hashes (600,000 iterations each), and did not check whether the video was still the same one when they were done. When another video was set meanwhile, the lookup wrote its record after the new video's reset. The new video's own lookup then saw a record and returned early, so it started at the other video's time and saved its progress into the other video's record. The lookup now keeps the player it started for, and writes nothing when the player has changed by the time either step is done. client-setup.e2e.mjs holds the first video's lookup until the second video's reset has run, and checks the second one starts at 0 with its own record. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(save): keep a frame that is shown after the last one decoded MP4Merger (the fMP4 HLS and DASH save) ended each track's edit list where its decode timeline ends. A stream cut in decode order, as a recording that stopped inside a group of pictures is, can show a frame after that point: the frames decoded after it are gone. The edit stopped short of that frame, so players left it out. The edit now lasts until the last shown frame ends, when that is later; for any other stream the two are the same. save-fmp4.e2e.mjs saves the video cut after 298 of its 300 frames in decode order (297 before). Its muxed fixture no longer uses -shortest: where that cut the video depended on the ffmpeg build (one from 2026-04 kept 298 frames, and the muxed test failed), and the tests count the saved frames against sample.mp4's. A fixture's marker now holds the ffmpeg arguments, so a fixture made with other ones is made again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(mp4): end the last timed range at the player's duration setFragmentTimes() gave the last range with known samples the metadata's duration as its end. A fragmented file without a mehd box has none there (0), so that range ended at 0, before its start. It now takes the duration the player counts (calculateDuration()), which for such a file is what has been parsed. The previous commit runs setFragmentTimes() after every range, so this is now the range at the parse edge each time, and once the file is parsed, its last range. mp4-files.e2e.mjs checks that no timed range of the 480 s file ends before it starts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(player): finish a video's setup when its remembered time cannot be read When the progress lookup threw (a record that does not decrypt, IndexedDB failing), loadProgressData() threw on. The rest of the video's setup waits on it, so the seek to the time in the URL and autoplay never came, and progress saving stayed off for the video. A record that cannot be read is now no remembered time: the setup goes on, and the record is written anew when there are hashes to write it under. client-setup.e2e.mjs makes the lookup throw and checks that the video is sought to its URL's time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(e2e): clear the remembered times after each client-setup case The progress cases save remembered times in the browser session's IndexedDB, and they outlive the page: a later case on the same video would start at one. Each case now clears them when it ends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(save): keep every frame of a transport stream's save, in sync HLS2MP4 (the save of HLS in MPEG-TS segments) lost frames in three ways: - a389dfd moved to hls.js 1.7.2, whose TSDemuxer.demux takes a chunkMeta before isSampleAes and flush. The call kept 1.6.9's order, so its flush flag landed on isSampleAes, and the data left at the end of the last segment was never parsed: every save lost the video's last two frames and the audio's last packet (14 AAC frames, a third of a second, on the test stream). - As MP4Merger did before a3befd1f, it ended each edit list where the decode timeline ends, so a frame shown after that was left out. - It placed a track that starts later with an empty edit counted in the track's own timescale, where the movie's is needed, and measured from the first decode time; then it shortened the track's edit by that delay. A 44100 Hz audio track lost its end and was out of step: the test stream's tone came out 44 ms late and one frame short, and audio that starts 0.48 s after the video came out 0.21 s early and 23 frames short. Tracks are now placed by their first shown frame, as MP4Merger and ffmpeg place them, and each edit lasts as long as its media. save-fmp4.e2e.mjs saves three transport streams: the video with its tone, the video cut in decode order, and the tone starting 0.5 s late. Each must keep every frame ffprobe counts in the stream, with the audio's start against the video's as in the stream. Each fix taken out fails at least one of them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(e2e): wait for the second video's lookup, not a fixed 1.5 s The race case read the remembered-time record 1.5 s after the picture came. The second video's lookup takes two PBKDF2 hashes: on a slow runner it could still be running then, and the case would fail blaming the race. It now waits until the lookup has kept its hashes. With the race back, the first video's hashes are there at once, and the case still fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(e2e): say where the concurrent-saves storage case stopped On the Windows runner "round-trips many concurrent saves" twice never settled, in one CI run of #52, and the failure could not say which call never returned. Like the next case in the file, it now records the step it reached and, through window.__diag, the backend and the storage calls still waiting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(e2e): keep the speed preset keys from misleading a test The player's mpv-style speed presets set 1x to 16x from a single letter (R, G, B, Q, W, A, Y, E, H), and the speed stays for the page's life. A test that pressed one, on purpose or not, left the video playing fast, and the times it or the next test on that page measured were off without anything saying so. speedWatch.mjs now runs around every test in every suite. Before a test it sets a leftover speed back to 1x and logs it. After a test it logs the preset keys pressed and the speed the test ended at, marked "SPEED" when the test failed. A page too busy to answer within 3 s is logged as not checked rather than passed over in silence. speed-watch.e2e.mjs: one case presses Q and ends at 3x; the next case on the same page must start at 1x with no presses logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four regressions in
main, found while checking which fixes could go to upstream (2026-09-29), and two subtitle bugs found while drafting those PRs. Each commit fixes one and comes with a test that fails without it. None changes what you see when things work; each only fixes a case that broke.1. Header rules skipped URLs with
*,^or|(7d9053d)The background adds a declarativeNetRequest rule to send a stream's Referer/Origin. 7d9053d "escaped" the URL's
*,^and|with a backslash, but urlFilter has no escape character: Firefox'sCompiledUrlFilter(ExtensionDNR.sys.mjs) splits it at*and matches every other character, the backslash included, literally. So the rule demanded a backslash no URL has, and a stream whose URL has a*(Akamaiacl=/*tokens, for one) got no Referer. The escaping is gone; unescaped, those characters can only widen the match, never exclude the URL.tests/e2e/ext-specs/header-rules.e2e.mjs: on the installed extension, the server must see the Referer for a plain URL (control), one with*, and one with^and|. With the old escaping, the last two get no Referer and the control still passes.tests/unit/NetRequestRuleManager.test.mjs: the rule's urlFilter is the URL minus its scheme, with no backslash.2. Dropped
.fsaarchives over 500 MB failed to open (d4d533a)httpGetLargereads the file in 250 MB chunks asparseFSAgoes, but the object URL was revoked once the first two chunks were requested, so every later chunk failed.FastStreamArchiveUtils.parseFSAFilenow keeps the URL until parsing is done. A file that cannot be read now ends in "archive failed" rather than an uncaught error.3. "Dump buffer" archives had no manifest (#49)
To keep live streams moving, #49 removed every HLS playlist and DASH MPD from the download store right after loading it. The archive is written from that store, so a player opened from an archive had to fetch the manifest again (impossible offline or once the URL expired), and the seek preview downloaded every playlist a second time. Now a player's first load of a manifest keeps it in the store (and may come from it); only a second load by the same player, i.e. a live refresh, drops the stored copy first. Live streams still refresh:
hls-live.e2e.mjsand the reload unit tests hold that.tests/unit/manifestReload.test.mjs: another player sharing the store gets the manifest without a download; the same player's reload downloads it again; the store keeps the newest copy. Mutation-checked both ways: with fix(player): whole-repo review part 2 - dead and live streams, saves, fragmented MP4 #49's loaders the three new tests fail; with the reload step disabled the reload tests fail.tests/e2e/specs/archive-roundtrip.e2e.mjs(bugs 2 and 3): plays HLS (master, video and audio playlists) and DASH, and writes the archive in memory as dumpBuffer does. It then drops the archive on a fresh, empty player page through the player's own drop handler, with the chunk size lowered to 64 KB. It checks the video plays again with no manifest fetched from the network. It fails with either fix taken out: without bug 3's fix the archive holds only segments, and without bug 2's the drop ends in "archive failed". (A fresh page, as when an archive is opened later. Dropped on the player still showing the same video, the archive's source is taken for the one already in the sources list, which isn't marked as coming from an archive. That behaviour predates the fork and isn't changed here.)4. A fragmented MP4 stopped early (5c88f96), and a long one after 10 s
Two commits:
checkEndOfStreamended the MediaSource once every sample mp4box knew of was appended. A fragmented file's samples become known onemoofat a time, so that also held mid-file whenever a 1 MB range ended between two fragments (measured with mp4box 2.4.1: true after a cut at, or a few bytes into, anymoof). The duration was then cut to what was buffered and the video stopped. For a fragmented file, the range holding the last byte must now be in as well.mp4-files.e2e.mjswas.Tests:
tests/e2e/specs/mp4-fragmented-end.e2e.mjsbuilds the mid-file case (afreebox moves the fixture's lastmoofto 8 bytes before the first range ends) and holds the second range back: the stream must stay open, and the video must play to its end. It fails with either commit taken out. (The duration while the range is held is what has been parsed: anempty_moovfile has nomehdbox to give it up front.)tests/e2e/specs/mp4-files.e2e.mjs: "plays a long fragmented MP4, far into it" (the 160 s fixture cut into fragments; seek to 120 s). Fails without the length fix (duration 10).mp4-files.e2e.mjs: MP4Player picks the range to load from the samples it knew when the metadata was parsed, so of a fragmented file bigger than its 30 buffered ranges (30 MB) it plays those and cannot seek past them. Measured with a 480 s, 51 MB file. Before, the limit was the first 1 MB. Fixing that means updating the sample index as ranges come in, which is a feature of its own.5. SubRip text below a line of spaces was lost (c4d765d)
c4d765d split cues at a line of spaces as if it were empty, so in a cue "Top / (spaces) / Bottom" the bottom line became a block with no timestamp and was dropped. ffmpeg and VLC keep it, as upstream did. Cues now split only at empty lines. A line of spaces between cues still ends the first one, because d3fec9b starts a cue at its timestamp. I ran 17 SubRip cases through srt2webvtt and vtt.js. Only this one gives different cues than before.
tests/unit/SubtitleUtils.test.mjs: a line of spaces and a line with a tab inside a cue are kept, with the text below them. Fails with the old split.6. A subtitle cue with no text stopped the time update (older than the fork)
A SubRip cue with nothing under its timestamp becomes a cue with empty text, and vtt.js gives it no DOM tree (
null).renderSubtitlesappended that null, which throws. While such a cue was on screen, everytimeUpdated()stopped there, so a subtitle track after it showed nothing and the skip segments were not updated. The cue is now skipped.tests/e2e/specs/subtitle-empty-cue.e2e.mjs: loads a track whose cue has no text, plus a second track, both spanning the whole video. It checks thattimeUpdated()runs through without an error, the skip segments are updated, and the second track's text is shown. Fails without the fix.Also
tests/e2e/specs/client-setup.e2e.mjs: 4dc5ab7 leftmain's lint red. PR #50, now inmain, made the same one-line change, so this branch merges withmainwithout a conflict.Checked
pnpm run verifyon Windows: exit 0, run at d1578a9, the commit before the last one. Results:subtitle-empty-cuepassed only on its retry: the video had played past the test's cues. The last commit makes the cues span the whole video.test-ext-github: 13 + 7 + 1 passed.verify:linuxon the final tree: running. I'll add the result here.header-rules: with the old escaping, the*and^/|URLs get no Referer.archive-roundtrip: fails with fix(player): whole-repo review part 2 - dead and live streams, saves, fragmented MP4 #49's loaders, and fails with the URL revoked early.mp4-fragmented-end: fails without the last-range check, and fails without the Content-Range order.mp4-files, long fragmented case: fails without the Content-Range order (duration 10).subtitle-empty-cue: fails without the null check. This was checked on the test before its last commit; I'll recheck the final version.SubtitleUtilsunit test: fails with the old split.main: it now includes feat(detection): find manifests by their Content-Type, and play the longest stream #50 and merges without conflicts, and the only file both change isclient-setup.e2e.mjs. The checks on this PR run on that merge. A local run of feat(detection): find manifests by their Content-Type, and play the longest stream #50'ssource-length.e2e.mjson the merged tree is still to do.save-fmp4's "muxed audio" case also fails onmain. Today's ffmpeg writes its generated fixture with 298 of 300 frames, so I used the fixture from the main checkout.No hand test: none of these is visible when things work, and each is held by a test that fails without the fix.
🤖 Generated with Claude Code