diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-09-17 15:50:55 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-09-17 15:50:55 +0200 |
| commit | 3dcce17f9c616c8528a93bc625eb462d7fffb8f9 (patch) | |
| tree | 3fcd64d9e0b5bef9e917052f8269907325dbc350 /packages/meshbay-node | |
| parent | d472725167c1d335996059d24b7b74f3730768fb (diff) | |
| download | meshbay-3dcce17f9c616c8528a93bc625eb462d7fffb8f9.tar.gz | |
fix(node): a seek reports where the picture begins, not where it was asked
Copied video can only start on a keyframe, so `-ss t` with `-c:v copy`
delivers the keyframe at or before `t`. The node reported `t` anyway and the
client builds `SourceBuffer.timestampOffset` out of that number, so everything
downstream believed the picture stood a few seconds further along than it did.
That was a wrong label while only the scrubber read it — it is recorded as
such, low priority, in the design document. Subtitles made it a wrong answer:
their cues carry the source's own absolute timestamps, so the mismatch put
every line on screen before it was spoken. Reported from real use on an H264
title, where seeking to 600 s, 2650 s and 5000 s lands on keyframes 0.82 s,
1.56 s and 4.64 s earlier.
The copy path now resolves the request to that keyframe, seeks to it, and
reports it. The bytes delivered do not change — ffmpeg lands on the same frame
either way — only the number that names them. The look-up reads the thirty
seconds before the request and cost 0.12–0.51 s on that title, which is the
price already costed in §15.3 and never paid. Re-encoded video is untouched:
it can begin exactly where it is asked to, and does.
Two details worth their lines. The keyframe is passed back to ffmpeg at six
decimals, because rounding a keyframe's own timestamp down puts it before the
frame it names and selects the previous one — the same fault again, smaller.
And the ffprobe window ends past the request, since an interval whose end is
the request never emits a keyframe sitting exactly on it: the resume position
is the one place a viewer asks for the same instant twice, and it would have
been answered a whole GOP early.
The test decodes the first frame served and matches it against the source
frame at the position announced, rather than comparing `start` to an expected
number — both sides of that comparison would be reading the same ffprobe and
would agree by construction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UGY17EPph5LsLzePPXhUVc
Diffstat (limited to 'packages/meshbay-node')
| -rw-r--r-- | packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py | 86 | ||||
| -rw-r--r-- | packages/meshbay-node/tests/test_stream_seek_reports_the_keyframe.py | 181 |
2 files changed, 265 insertions, 2 deletions
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 2863679..8ffcbed 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -292,6 +292,12 @@ 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 # 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. @@ -6172,10 +6178,34 @@ class WebRTCPeerSession: # library's HEVC files looked like the only ones that worked: video # 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. seek_args: list[str] = [] if start > 0: - seek_args = ["-ss", f"{start:.3f}"] if transcode_video else [ - "-noaccurate_seek", "-ss", f"{start:.3f}"] + 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}"] map_args = ["-map", "0:v:0"] if transcode_video: log.info("stream: re-encoding %s (%s) to H264", entry.name, @@ -6493,6 +6523,58 @@ 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. + """ + if t <= 0: + return 0.0 + window_start = max(0.0, t - KEYFRAME_LOOKBACK_SECS) + try: + proc = 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), + stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.DEVNULL, + ) + stdout, _ = await asyncio.wait_for( + proc.communicate(), timeout=KEYFRAME_LOOKUP_TIMEOUT_SECS) + except (asyncio.TimeoutError, OSError) as e: + log.warning("stream: keyframe lookup 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 + + async def _extract_subtitle_to_webvtt(file_path: Path, ordinal: int) -> bytes: """ One subtitle track out of a container, whole, as WebVTT. 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 new file mode 100644 index 0000000..acd2ebb --- /dev/null +++ b/packages/meshbay-node/tests/test_stream_seek_reports_the_keyframe.py @@ -0,0 +1,181 @@ +""" +A seek on a copied stream reports where the picture actually begins. + +Copied video has to start on a keyframe, so `-ss t` with `-c:v copy` delivers +the keyframe at or before `t` — up to a whole GOP earlier. The node reported +`t` regardless, and the client sets `SourceBuffer.timestampOffset` from that +number, so everything downstream believed the picture stood a few seconds +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. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_common.crypto import generate_gek +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, +) + +from conftest import needs_subprocess, one_root + +_HAVE_FFMPEG = shutil.which("ffmpeg") and shutil.which("ffprobe") +pytestmark = [ + pytest.mark.skipif(not _HAVE_FFMPEG, reason="ffmpeg/ffprobe not installed"), + needs_subprocess, +] + +# 25 fps with a keyframe every 250 frames — one every 10 s, which is what a +# real WEB-DL looks like and what makes the gap large enough to see. +_GOP_SECONDS = 10 +_SEEK_TO = 15.0 # between the keyframes at 10 s and 20 s +_EXPECTED_KEYFRAME = 10.0 + + +def _make_h264_clip(path: Path) -> None: + """30 s of H264 the streaming path will copy rather than re-encode. + + The picture has to differ from one second to the next, or a frame from the + keyframe and a frame from the requested position would compare equal and + the test would pass against the bug it exists for. + """ + subprocess.run( + ["ffmpeg", "-hide_banner", "-loglevel", "error", "-y", + "-f", "lavfi", "-i", "testsrc=size=320x240:rate=25:duration=30", + "-f", "lavfi", "-i", "sine=duration=30", + "-c:v", "libx264", "-preset", "ultrafast", "-pix_fmt", "yuv420p", + "-g", str(_GOP_SECONDS * 25), "-keyint_min", str(_GOP_SECONDS * 25), + "-sc_threshold", "0", + "-c:a", "aac", "-shortest", str(path)], + check=True, capture_output=True) + + +def _session(video_path: Path, gek: bytes): + import blake3 + file_bytes = video_path.read_bytes() + file_id = blake3.blake3(file_bytes).hexdigest() + + sk_node = Ed25519PrivateKey.generate() + index = GroupIndex(group_id="g" * 32, sk_node=sk_node, gek=gek) + from meshbay_common.protocol import IndexEntry + index.add_entry(IndexEntry( + id=file_id, name=video_path.name, path=video_path.parent.name, + size=len(file_bytes), type="video", added_at=0)) + + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = { + "roots": one_root(video_path.parent), + "index": index, + "gek": gek, + "sk_node": sk_node, + "max_concurrent_streams": 4, + } + session._group_id = None + session._user_id = "tester" + session._stream_stopped = False + session._stream_keepalives = 0 + session.sent = [] + session._send = session.sent.append + session._audit = lambda *a, **k: None + return session, file_id + + +def _reassemble(sent: list[dict], gek: bytes, file_id: str) -> bytes: + file_hash = bytes.fromhex(file_id) + out = b"" + for m in sorted((m for m in sent if m.get("type") == "stream_data"), + key=lambda m: m["segment_index"]): + key = chunk_key_aes(gek, file_hash, m["segment_index"]) + out += decrypt_chunk_aes(key, m["nonce"], m["ct"]) + return out + + +def _frame_md5(path: Path, at: float | None = None) -> str: + """One decoded frame as raw pixels, hashed. `at` None means the first.""" + args = ["ffmpeg", "-hide_banner", "-loglevel", "error"] + if at is not None: + args += ["-ss", f"{at:.6f}"] + args += ["-i", str(path), "-frames:v", "1", "-f", "rawvideo", + "-pix_fmt", "rgb24", "-"] + proc = subprocess.run(args, check=True, capture_output=True) + import hashlib + return hashlib.md5(proc.stdout).hexdigest() + + +@pytest.mark.asyncio +async def test_the_lookup_finds_the_keyframe_the_seek_will_land_on(tmp_path): + clip = tmp_path / "clip.mp4" + _make_h264_clip(clip) + + assert await _keyframe_at_or_before(clip, _SEEK_TO) == 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 + + +@pytest.mark.asyncio +async def test_a_seek_reports_where_the_picture_begins_not_where_it_was_asked(tmp_path): + clip = tmp_path / "clip.mp4" + _make_h264_clip(clip) + gek = generate_gek() + session, file_id = _session(clip, gek) + + await session._stream_video_inner( + {"file_id": file_id, "start": _SEEK_TO, "credits": 0}) + + errors = [m for m in session.sent if m.get("type") == "error"] + assert not errors, errors + init = next(m for m in session.sent if m.get("type") == "stream_init") + + assert init["start"] == pytest.approx(_EXPECTED_KEYFRAME, abs=0.05), ( + f"the node announced {init['start']}, but a copied stream cannot " + f"begin anywhere but the keyframe at {_EXPECTED_KEYFRAME}") + assert init["start"] < _SEEK_TO, ( + "this fixture must have a keyframe gap to report, or the test cannot " + "tell the fix from the fault") + + +@pytest.mark.asyncio +async def test_the_announced_position_is_the_picture_that_was_served(tmp_path): + """Decoded and compared against the source — see this module's docstring.""" + clip = tmp_path / "clip.mp4" + _make_h264_clip(clip) + gek = generate_gek() + session, file_id = _session(clip, gek) + + await session._stream_video_inner( + {"file_id": file_id, "start": _SEEK_TO, "credits": 0}) + + init = next(m for m in session.sent if m.get("type") == "stream_init") + served = tmp_path / "served.mp4" + served.write_bytes(_reassemble(session.sent, gek, file_id)) + + assert _frame_md5(served) == _frame_md5(clip, at=init["start"]), ( + "the first frame delivered is not the source frame at the position " + "the node announced, so `start` still does not name what was served") + # And it is emphatically not the frame at the position asked for, or the + # comparison above would hold for the old behaviour too. + assert _frame_md5(served) != _frame_md5(clip, at=_SEEK_TO), ( + "the fixture's picture does not change across the keyframe gap, so " + "this comparison proves nothing") |