diff options
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") |