From 0fed6786fe304195df66776787c5b5b82d321bcf Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Thu, 17 Sep 2026 16:18:30 +0200 Subject: fix(node): measure where a seek lands instead of predicting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix read ffprobe's key frames and took the last one at or before the request. It was wrong twice, and a viewer felt the difference: subtitles went from 5 s early to 2–3 s late. Matroska's Cues index only some keyframes, so an index seek backs off to an indexed one that the frame list does not single out. And the landing point moves with **which streams are mapped**, because the container is positioned where every mapped stream has data — on the reported title, a seek to 4913.7 s landed at 4909.863 with video alone and at 4907.236 with the second audio track mapped beside it. The frame scan gave the first number; the stream delivered the second; the gap was 2.65 s, and the measured audio displacement in the served stream was 2.65 s. So the node asks ffmpeg instead: the same seek, the same mapping, one copied frame under `-copyts`, and the answer read back off the result. 0.06–0.07 s, cheaper than the scan it replaces. The `-ss` argument stays at the request, so the bytes served are exactly the ones served before — only the number naming them changes. The probe runs after the audio track is resolved, because it cannot be right before that is known. An answer after the request, or further before it than any real keyframe gap, is discarded in favour of the old label: a number wrong by seconds beats a fabricated one. Found by decoding the served stream and locating its first frame in the source, which put it at 4907.213 s against an announced 4909.863 s. The test does the same thing rather than comparing the announced number against a second reading of the same probe. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UGY17EPph5LsLzePPXhUVc --- docs/MESHBAY_DESIGN.md | 21 +-- docs/MESHBAY_NODE_PROTOCOL.md | 2 +- .../src/meshbay_node/transport/webrtc_server.py | 160 +++++++++++---------- .../tests/test_stream_seek_reports_the_keyframe.py | 53 ++++--- 4 files changed, 138 insertions(+), 98 deletions(-) diff --git a/docs/MESHBAY_DESIGN.md b/docs/MESHBAY_DESIGN.md index 7ce1168..8f91a87 100644 --- a/docs/MESHBAY_DESIGN.md +++ b/docs/MESHBAY_DESIGN.md @@ -2052,14 +2052,19 @@ MSE string) fed to a source buffer, with the node holding one slot per viewer. from the end and echoed back; the client supplies the timestamp offset, because copying timestamps does not preserve position. - **`stream_init.start` names where the picture begins, never where the viewer - dragged.** Copied video can only begin on a keyframe, so on that path the node - resolves the request to the keyframe at or before it, seeks to that, and reports - that. The client builds `SourceBuffer.timestampOffset` out of this number, and a - subtitle cue carries the source's own absolute time: one GOP of disagreement - between the two puts every line on screen before it is spoken. The look-up is a - bounded read of the thirty seconds before the request, measured at 0.12–0.51 s - on a real title, and only the copy path needs it — re-encoded video begins - exactly where it is asked to. + dragged**, and on the copy path that position is **measured, never predicted**. + The client builds `SourceBuffer.timestampOffset` out of this number and a + subtitle cue carries the source's own absolute time, so every second of + disagreement puts a line on screen a second away from the voice saying it. + Reading the key frames and taking the last one at or before the request answers + a different question twice over: Matroska's Cues index only some keyframes, so + an index seek backs off to an indexed one that can be much earlier, and the + landing point moves with **which streams are mapped**, because the container is + positioned where every mapped stream has data — one real seek answered 4909.863 s + from the frame list and delivered 4907.236 s. So the node runs the same seek with + the same mapping, copies one frame under `-copyts`, and reads the answer back: + 0.06–0.07 s, cheaper than the scan it replaced. Only the copy path needs it — + re-encoded video begins exactly where it is asked to. - **A seek trims what it can, and what it can differs per stream — which is a desync, not an inconvenience.** Accurate seeking cannot trim copied video, which must begin on a keyframe, but it does trim re-encoded audio to the exact request. diff --git a/docs/MESHBAY_NODE_PROTOCOL.md b/docs/MESHBAY_NODE_PROTOCOL.md index 2aede26..a53d494 100644 --- a/docs/MESHBAY_NODE_PROTOCOL.md +++ b/docs/MESHBAY_NODE_PROTOCOL.md @@ -1671,7 +1671,7 @@ array while MediaSource consumes it a segment at a time. | Concurrent transcodes | 8 node-wide, semaphore on the transport context | | Seeking | a new `stream_req` with `start`; the previous stream is retired first, ffmpeg respawned with `-ss` | | Accurate seek | **off when the video is copied, on when it is re-encoded.** Copied video has to begin on a keyframe and cannot be trimmed to the request; re-encoded audio can, and is. Leaving both at the default put a whole GOP of silence at the head of every seek and left sound and picture a GOP apart — with correct timestamps throughout, so nothing downstream could detect it | -| `start` in `stream_init` | the value actually used, and on the copy path that is the **keyframe at or before the request**, resolved by a bounded look-up before ffmpeg is spawned (0.12–0.51 s, measured). The client adds it back as `SourceBuffer.timestampOffset`; a subtitle cue carries the source's absolute time, so reporting the request instead would put every line on screen one GOP before it is spoken | +| `start` in `stream_init` | the value actually used. On the copy path it is **measured** before the stream is served — the same seek with the same stream mapping, one frame copied under `-copyts`, 0.06–0.07 s — because an index seek lands on an indexed keyframe that the frame list cannot predict and that moves with which audio track is mapped. The client adds it back as `SourceBuffer.timestampOffset`; a subtitle cue carries the source's absolute time, so reporting anything else puts every line on screen away from the voice | | `audio_tracks` in `stream_init` | every audio track: `i` (the **audio ordinal**, what `-map 0:a:` takes, never the container stream index), `lang`, `title`, `codec`, `ch`. Empty for a file with no audio | | `audio_track` in `stream_req` | which ordinal to map. Absent, out of range or malformed is the first track | | `audio_track` in `stream_init` | the ordinal actually used, for the same reason `start` is reported: a list drawn before the file was replaced on disk can name a track that is no longer there, and the client must show what is playing rather than what it asked for. `null` when the file has no audio | diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index 180c891..47cffcc 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -292,12 +292,13 @@ AUDIO_TRANSCODE_TIMEOUT_SECS = 120 # pathological container rather than against the work itself, and it is short # next to the audio one because nothing here decodes a media stream. SUBTITLE_EXTRACT_TIMEOUT_SECS = 60 -# How far back to look for the keyframe a copied seek will land on. Measured -# on a real H264 title: the answer came back in 0.12–0.51 s, and the largest -# gap between keyframes was under 10 s. Thirty seconds is three times that and -# still one short read rather than a scan of the file. -KEYFRAME_LOOKBACK_SECS = 30 -KEYFRAME_LOOKUP_TIMEOUT_SECS = 10 +# Probing where a copied seek lands costs 0.06–0.07 s on a real title, so this +# bounds a pathological container rather than the work itself. +SEEK_PROBE_TIMEOUT_SECS = 10 +# An index seek backing off further than this did not find a keyframe gap; it +# measured something other than the stream about to be served, and the old +# label beats a fabricated one. The largest real gap seen was under 10 s. +SEEK_PROBE_MAX_BACKOFF_SECS = 60 # A subtitle file is text; a film's is ~96 KB. Anything past this is not a # subtitle track, it is an ffmpeg that found something else to write, and it # would sit in the media cache for ever. @@ -6187,33 +6188,19 @@ class WebRTCPeerSession: # that is re-encoded *can* start exactly at `start`, so accurate # seeking is right there and stays on. # - # **`start` below is rewritten to the keyframe on the copy path**, so - # from here down it is where the picture actually begins and not where - # the viewer dragged to. That distinction was invisible while only the - # scrubber read the number; it stopped being invisible when subtitles - # did, since their cues carry the source's absolute times and a GOP's - # worth of disagreement puts a line on screen before it is spoken. + # **`start` is rewritten on the copy path to where the seek actually + # lands**, which is measured below rather than predicted — see + # `_seek_lands_at`. From the rewrite on, it is where the picture really + # begins and not where the viewer dragged to. The distinction was + # invisible while only the scrubber read the number; it stopped being + # invisible when subtitles did, since their cues carry the source's + # absolute times and every second of disagreement puts a line on + # screen a second away from the voice saying it. + requested = start seek_args: list[str] = [] - if start > 0: - if transcode_video: - seek_args = ["-ss", f"{start:.3f}"] - else: - # Copied video begins on a keyframe whatever is asked for, so - # ask for the keyframe itself and report *that* as `start`. - # The bytes are the ones ffmpeg would have delivered anyway — - # it lands on the same frame either way — but the client now - # sets `timestampOffset` to where the picture really begins - # rather than to where it was asked to begin, which is what - # puts a subtitle cue over the line that is being spoken. - # - # Six decimals because ffprobe reports six: rounding the - # keyframe's own timestamp *down* would put it before the - # frame it names and select the previous keyframe instead, - # which is this same fault again, smaller. - keyframe = await _keyframe_at_or_before(file_path, start) - if keyframe is not None: - start = keyframe - seek_args = ["-noaccurate_seek", "-ss", f"{start:.6f}"] + if requested > 0: + seek_args = ["-ss", f"{requested:.3f}"] if transcode_video else [ + "-noaccurate_seek", "-ss", f"{requested:.3f}"] map_args = ["-map", "0:v:0"] if transcode_video: log.info("stream: re-encoding %s (%s) to H264", entry.name, @@ -6256,6 +6243,19 @@ class WebRTCPeerSession: # the failure doesn't surface until playback, as a SourceBuffer # forced out of its MediaSource with no further explanation. codec_args += ["-c:a", "aac", "-ac", "2", "-b:a", "192k"] + # Where that seek lands, measured with the mapping this stream will + # use. It has to be here rather than beside `seek_args` above: the + # landing point depends on which audio track is mapped, because the + # container is seeked to a position that serves *every* mapped stream + # — on a real title, video alone landed at 4909.863 s and the same + # seek with the second audio track landed at 4907.236 s. The `-ss` + # argument is deliberately left at the request, so the bytes served + # are exactly the ones served before; only the number naming them + # changes. + if requested > 0 and not transcode_video: + landed = await _seek_lands_at(file_path, requested, map_args) + if landed is not None: + start = landed proc = await asyncio.create_subprocess_exec( platform.ffmpeg_cmd(), "-hide_banner", "-loglevel", "error", *seek_args, @@ -6531,56 +6531,70 @@ async def _transcode_audio_to_aac(file_path: Path) -> bytes: tmp_path.unlink(missing_ok=True) -async def _keyframe_at_or_before(file_path: Path, t: float) -> float | None: - """Where a copied stream seeking to `t` will actually begin. - - Copied video has to start on a keyframe, so `-ss t` on a `-c:v copy` - stream delivers the keyframe at or before `t` — up to a whole GOP earlier. - The node used to report `t` anyway, and the client sets its - `SourceBuffer.timestampOffset` from that number: everything downstream - therefore believed the picture was a few seconds further along than it - was. Harmless while only the scrubber read it; not harmless once subtitles - do, because their cues carry the source's own absolute times and appeared - a GOP early — measured on a real H264 title at 0.8 s, 1.6 s and 4.6 s - depending on where the viewer dragged. - - Returns None when the lookup finds nothing, and the caller then keeps the - old behaviour: a label that is wrong by a few seconds is worth a great - deal less than a stream that does not start. +async def _seek_lands_at(file_path: Path, t: float, map_args: list[str]) -> float | None: + """Where an index seek to `t` actually puts this stream, in source time. + + Measured, not predicted. Copied video can only begin on a keyframe, and + the obvious way to find that keyframe — scan ffprobe's key frames and take + the last one at or before `t` — is wrong twice over. Matroska's Cues index + only some keyframes, so the seek backs off to an indexed one that can be + much earlier; and the landing point depends on **which streams are + mapped**, because the container is positioned where every mapped stream + has data. Measured on a real title: a seek to 4913.7 s landed at 4909.863 + with video alone and at 4907.236 with the second audio track mapped + alongside it. A prediction from the frame list gave the first number and + the stream delivered the second, which is 2.65 s of subtitles standing + away from the voice. + + So ffmpeg is asked instead: the same seek, the same mapping, one copied + frame, `-copyts` to keep the source's own timestamps, and the answer read + back off the result. Measured at 0.06–0.07 s, which is cheaper than the + frame scan it replaces. + + Returns None if anything about the probe fails, and the caller then keeps + the old label: a number that is wrong by a few seconds is worth much less + than a stream that does not start. """ - if t <= 0: - return 0.0 - window_start = max(0.0, t - KEYFRAME_LOOKBACK_SECS) + fd, tmp_name = tempfile.mkstemp(suffix=".mp4") + os.close(fd) + tmp_path = Path(tmp_name) try: proc = await asyncio.create_subprocess_exec( + platform.ffmpeg_cmd(), "-hide_banner", "-loglevel", "error", "-y", + "-copyts", "-noaccurate_seek", "-ss", f"{t:.3f}", + "-i", str(file_path), + *map_args, "-c", "copy", "-frames:v", "1", + "-f", "mp4", str(tmp_path), + stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.DEVNULL, + ) + await asyncio.wait_for(proc.wait(), timeout=SEEK_PROBE_TIMEOUT_SECS) + if proc.returncode != 0: + return None + probe = await asyncio.create_subprocess_exec( platform.ffprobe_cmd(), "-v", "error", - "-select_streams", "v:0", "-skip_frame", "nokey", - "-show_entries", "frame=pts_time", - # The window ends past `t`, and the `pts <= t` filter below is what - # actually bounds the answer: ffprobe's interval end is exclusive - # enough that a keyframe sitting exactly on `t` is never emitted, - # and the lookup then names the previous one — a whole GOP earlier - # than where the viewer asked to be, for the one position they are - # most likely to ask for twice (a resume lands on it). - "-read_intervals", f"{window_start:.3f}%{t + 1.0:.3f}", - "-of", "csv=p=0", str(file_path), + "-select_streams", "v:0", "-show_entries", "stream=start_time", + "-of", "csv=p=0", str(tmp_path), stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.DEVNULL, ) stdout, _ = await asyncio.wait_for( - proc.communicate(), timeout=KEYFRAME_LOOKUP_TIMEOUT_SECS) + probe.communicate(), timeout=SEEK_PROBE_TIMEOUT_SECS) except (asyncio.TimeoutError, OSError) as e: - log.warning("stream: keyframe lookup failed at %.1fs: %r", t, e) + log.warning("stream: seek probe failed at %.1fs: %r", t, e) return None - best: float | None = None - for line in stdout.decode(errors="replace").splitlines(): - try: - pts = float(line.strip().rstrip(",")) - except ValueError: - continue - # `<= t` and the largest such: the frame the seek will land on. - if pts <= t and (best is None or pts > best): - best = pts - return best + finally: + tmp_path.unlink(missing_ok=True) + text = stdout.decode(errors="replace").strip().rstrip(",") + try: + landed = float(text) + except ValueError: + return None + # A seek never lands after what was asked for, and a landing point wildly + # before it is a probe that measured something else — a chapter track, an + # attachment. Either way the old label beats a fabricated one. + if not 0 <= landed <= t + 0.5 or t - landed > SEEK_PROBE_MAX_BACKOFF_SECS: + log.warning("stream: seek probe at %.1fs answered %.3f — ignored", t, landed) + return None + return landed async def _extract_subtitle_to_webvtt(file_path: Path, ordinal: int) -> bytes: diff --git a/packages/meshbay-node/tests/test_stream_seek_reports_the_keyframe.py b/packages/meshbay-node/tests/test_stream_seek_reports_the_keyframe.py index acd2ebb..9aea2b4 100644 --- a/packages/meshbay-node/tests/test_stream_seek_reports_the_keyframe.py +++ b/packages/meshbay-node/tests/test_stream_seek_reports_the_keyframe.py @@ -9,16 +9,24 @@ further along than it did. That was a wrong label while only the scrubber read it. It became a wrong *answer* when subtitles arrived: their cues carry the source's own absolute -timestamps, so the mismatch put every line on screen a GOP before it was -spoken. Measured on a real H264 title, seeking to 600 s, 2650 s and 5000 s -landed on keyframes 0.82 s, 1.56 s and 4.64 s earlier. - -**The assertion is on the decoded picture, not on the number.** A test that -only compared `stream_init["start"]` against an expected keyframe would agree -with the implementation by construction — both would be reading the same -ffprobe. The first frame delivered is decoded and matched against the source -frame at the position the node claims, which is a statement about what was -served rather than about what was computed. +timestamps, so the mismatch put every line on screen before it was spoken. + +**Where the seek lands is measured, not predicted, and that distinction is +this module's subject.** The first attempt scanned ffprobe's key frames and +took the last one at or before the request. It was wrong twice: Matroska's +Cues index only some keyframes, so the seek backs off to an indexed one that +can be much earlier, and the landing point moves with **which streams are +mapped**, because the container is positioned where every mapped stream has +data. On a real title, one seek answered 4909.863 s from the frame list and +delivered 4907.236 s — 2.65 s of subtitles standing away from the voice, which +is what a viewer reported after the first fix. + +**The assertion is therefore on the decoded picture, not on the number.** A +test comparing `stream_init["start"]` against an expected keyframe would agree +with the implementation by construction, both reading the same probe. The +first frame delivered is decoded and matched against the source frame at the +position the node claims, which is a statement about what was served rather +than about what was computed. """ import shutil @@ -33,7 +41,7 @@ from meshbay_common.webcrypto import chunk_key_aes, decrypt_chunk_aes from meshbay_node.indexer.group_index import GroupIndex from meshbay_node.transport.webrtc_server import ( WebRTCPeerSession, - _keyframe_at_or_before, + _seek_lands_at, ) from conftest import needs_subprocess, one_root @@ -121,17 +129,30 @@ def _frame_md5(path: Path, at: float | None = None) -> str: return hashlib.md5(proc.stdout).hexdigest() +_MAPS = ["-map", "0:v:0", "-map", "0:a:0"] + + @pytest.mark.asyncio -async def test_the_lookup_finds_the_keyframe_the_seek_will_land_on(tmp_path): +async def test_the_probe_measures_where_the_seek_lands(tmp_path): + """Measured with the mapping the stream will use, never predicted. + + A scan of ffprobe's key frames answers a different question: Matroska + indexes only some keyframes, and the landing point also moves with which + audio track is mapped, because the container is positioned where every + mapped stream has data. On a real title those two answers were 4909.863 + and 4907.236 — 2.65 s of subtitles standing away from the voice. + """ clip = tmp_path / "clip.mp4" _make_h264_clip(clip) - assert await _keyframe_at_or_before(clip, _SEEK_TO) == pytest.approx( + assert await _seek_lands_at(clip, _SEEK_TO, _MAPS) == pytest.approx( _EXPECTED_KEYFRAME, abs=0.05) # A position that *is* a keyframe answers itself, not the one before. - assert await _keyframe_at_or_before(clip, 20.0) == pytest.approx(20.0, abs=0.05) - # Before the first one there is nothing earlier to find. - assert await _keyframe_at_or_before(clip, 0) == 0.0 + assert await _seek_lands_at(clip, 20.0, _MAPS) == pytest.approx(20.0, abs=0.05) + # A seek never lands after what was asked for. + for t in (7.0, 15.0, 23.0): + landed = await _seek_lands_at(clip, t, _MAPS) + assert landed is not None and landed <= t + 0.05, (t, landed) @pytest.mark.asyncio -- cgit v1.2.3