diff options
| -rw-r--r-- | docs/MESHBAY_DESIGN.md | 21 | ||||
| -rw-r--r-- | docs/MESHBAY_NODE_PROTOCOL.md | 2 | ||||
| -rw-r--r-- | packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py | 156 | ||||
| -rw-r--r-- | packages/meshbay-node/tests/test_stream_seek_reports_the_keyframe.py | 51 |
4 files changed, 135 insertions, 95 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:<n>` 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. +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. - 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. + 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 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. + 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. +timestamps, so the mismatch put every line on screen before it was spoken. -**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. +**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 |