From 00539a4d976f702ae1bfe620df25c21e8a273bca Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 30 Aug 2026 02:04:17 +0200 Subject: [PATCH] fix(plex): deep-review follow-ups on E6 transcode + player polish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight verified findings from the sprint deep-review: - Transcode cap: _enforce_cap now counts only transcode sessions, so a live remux (stream-copy) stream can no longer be LRU-evicted mid-play — matching the config comment + the HU/EN Config hint that promise remux is uncapped. - Player Back: the browser/mouse Back interceptor (BackClose) now also mounts for the stats overlay and up-next card, so Back closes them first instead of ejecting the whole player (keyboard/Esc already did). - Honest cold-start: loadSession clears sessionIsTranscode up front, so a transcode->direct/remux switch no longer flashes a stale "Preparing stream..." during the session request. - Stats overlay a11y: the close (X) buttons in the stats, help and info overlays now read as "Close"/"Bezaras" (new plex.player.close key), not "Back"; dropped aria-live from the per-second stat grid so a screen reader is not flooded with telemetry. Tests: route-level transcode routing (transcode item -> HLS re-encode session, remux stays copy, direct opens none); start_session threading the preset/crf/max_height knobs + hw/preexec into the ffmpeg argv; UpNextCard countdown auto-advance, Play-now, Cancel and the S?-E? heading fallback. Also brings UpNextCard.tsx / plexStats.ts / plexStats.test.ts to prettier. --- backend/app/plex/stream.py | 12 +- backend/tests/test_plex_playback_session.py | 107 ++++++++++++++++++ backend/tests/test_plex_stream.py | 87 ++++++++++++++ frontend/src/components/PlexInfo.tsx | 2 +- frontend/src/components/PlexPlayer.tsx | 16 ++- frontend/src/components/PlexStatsOverlay.tsx | 7 +- .../src/components/UpNextCard.dom.test.tsx | 93 +++++++++++++++ frontend/src/components/UpNextCard.tsx | 9 +- frontend/src/i18n/locales/en/plex.json | 1 + frontend/src/i18n/locales/hu/plex.json | 1 + frontend/src/lib/plexStats.test.ts | 4 +- frontend/src/lib/plexStats.ts | 12 +- 12 files changed, 335 insertions(+), 16 deletions(-) create mode 100644 backend/tests/test_plex_playback_session.py create mode 100644 frontend/src/components/UpNextCard.dom.test.tsx diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index ab41436d..1f75d071 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -393,12 +393,14 @@ def _kill(s: HlsSession) -> None: def _enforce_cap(cap: int) -> None: - # Caller holds _lock. Drop the least-recently-accessed sessions over the cap. - if len(_sessions) <= cap: + # Caller holds _lock. Only TRANSCODE sessions count toward plex_max_transcodes — remux + # (stream-copy) and direct-serve are cheap and deliberately uncapped (see config.py: + # plex_max_transcodes, and the user-facing config hint). Drop the least-recently-accessed + # transcode sessions over the cap, leaving any concurrent remux streams untouched. + tx = [(key, s) for key, s in _sessions.items() if s.transcode] + if len(tx) <= cap: return - for key, s in sorted(_sessions.items(), key=lambda kv: kv[1].last_access)[ - : len(_sessions) - cap - ]: + for key, s in sorted(tx, key=lambda kv: kv[1].last_access)[: len(tx) - cap]: _kill(s) _sessions.pop(key, None) diff --git a/backend/tests/test_plex_playback_session.py b/backend/tests/test_plex_playback_session.py new file mode 100644 index 00000000..4be08777 --- /dev/null +++ b/backend/tests/test_plex_playback_session.py @@ -0,0 +1,107 @@ +"""The playback-session route's mode routing (E6 S1b). + +`POST /stream/{rk}/session` is the single switch that decides how an item plays: a browser-native +file is served raw (`mode:"direct"`), everything else (re)starts an HLS session — and a +`playable="transcode"` item (non-h264: HEVC/VP9/AV1/…) must take that HLS path with the video +RE-ENCODED, not stream-copied. That routing replaced a former `HTTP 501`, and the only thing that +tells `start_session` to re-encode is the `transcode=` flag this route derives from `it.playable`. +The command-builder itself is unit-tested in `test_plex_stream.py`; here we pin the ROUTE decision +(no ffmpeg spawned — `start_session` is stubbed to echo the flag it was handed). + +DB lane (`db` fixture) — skipped when no Postgres is reachable. +""" + +from types import SimpleNamespace + +from fastapi.testclient import TestClient + +import app.state as state +from app.auth import current_user +from app.db import get_db +from app.main import app +from app.models import PlexItem, PlexLibrary, User +from app.plex import paths as plex_paths +from app.plex import stream as plex_stream + + +def _item(db, *, playable, rating_key="m1"): + lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + db.add(lib) + db.flush() + it = PlexItem( + rating_key=rating_key, + library_id=lib.id, + kind="movie", + title="A Movie", + file_path="/media/a.mkv", + playable=playable, + ) + db.add(it) + db.commit() + return it + + +def _user(db, email="play@example.com"): + u = User(email=email) + db.add(u) + db.flush() + return u + + +def _post(db, user, monkeypatch, rating_key, *, captured): + """Drive the endpoint with ffmpeg stubbed out. `start_session` records the `transcode` flag it + was called with into `captured` and returns a stand-in session echoing it back.""" + monkeypatch.setattr(state, "_configured_cache", True) # pass the request gate + monkeypatch.setattr(plex_stream, "reap_idle", lambda: None) + monkeypatch.setattr(plex_paths, "local_media_path", lambda db_, fp: "/local/a.mkv") + + def fake_start(db_, it, start, audio, aoff, multi, transcode=False): + captured["transcode"] = transcode + return SimpleNamespace( + media_start_s=start, entry="index.m3u8", transcode=transcode, hw=False + ) + + monkeypatch.setattr(plex_stream, "start_session", fake_start) + app.dependency_overrides[get_db] = lambda: db + app.dependency_overrides[current_user] = lambda: user + try: + return TestClient(app).post(f"/api/plex/stream/{rating_key}/session") + finally: + app.dependency_overrides.clear() + + +def test_transcode_item_routes_to_an_hls_reencode_session(db, monkeypatch): + # A non-h264 item (formerly HTTP 501) now plays: mode=hls, transcode=True threaded to start_session. + it = _item(db, playable="transcode") + user = _user(db) + captured: dict = {} + resp = _post(db, user, monkeypatch, it.rating_key, captured=captured) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["mode"] == "hls" + assert body["transcode"] is True + assert captured["transcode"] is True # start_session was told to re-encode + + +def test_remux_item_stays_a_stream_copy_session(db, monkeypatch): + # A remux (h264) item takes the SAME HLS path but must NOT re-encode. + it = _item(db, playable="remux", rating_key="m2") + user = _user(db, "remux@example.com") + captured: dict = {} + resp = _post(db, user, monkeypatch, it.rating_key, captured=captured) + assert resp.status_code == 200, resp.text + body = resp.json() + assert body["mode"] == "hls" + assert body["transcode"] is False + assert captured["transcode"] is False + + +def test_direct_item_never_opens_a_session(db, monkeypatch): + # A browser-native file is served raw — start_session is never reached, so no transcode decision. + it = _item(db, playable="direct", rating_key="m3") + user = _user(db, "direct@example.com") + captured: dict = {} + resp = _post(db, user, monkeypatch, it.rating_key, captured=captured) + assert resp.status_code == 200, resp.text + assert resp.json()["mode"] == "direct" + assert "transcode" not in captured # start_session not called diff --git a/backend/tests/test_plex_stream.py b/backend/tests/test_plex_stream.py index 592bad9b..00c55ece 100644 --- a/backend/tests/test_plex_stream.py +++ b/backend/tests/test_plex_stream.py @@ -172,6 +172,18 @@ class _FakeItem: self.codec_video = codec_video +class _CapturingProc: + """Records the argv + kwargs each Popen was called with, then behaves like the ffmpeg proc.""" + + calls: list[tuple] = [] + + def __init__(self, *args, **kwargs): + _CapturingProc.calls.append((args, kwargs)) + + def poll(self): + return None + + def _venc(cmd): """The video codec ffmpeg was told to use (the value after the first `-c:v`).""" return cmd[cmd.index("-c:v") + 1] @@ -356,3 +368,78 @@ class TestColdStartWarmup: s = stream.start_session(db=None, item=_FakeItem(), start_s=0.0) assert s is not None assert waited == ["seg_0.ts"] + + +class TestTranscodeStartSession: + """The seam TestTranscodeCommand can't reach: start_session reading the three admin knobs + (plex_transcode_preset/crf/max_height) and threading them + transcode/hw into the ffmpeg argv and + the HlsSession. A swapped knob here is a SILENT mis-encode (still plays), so pin the whole wiring + end-to-end with a real (captured) Popen argv — not just _ffmpeg_cmd called with literals.""" + + @pytest.fixture + def capturing_env(self, tmp_path, monkeypatch): + monkeypatch.setattr(stream, "_HLS_ROOT", tmp_path) + monkeypatch.setattr(stream, "_sessions", {}) + src = tmp_path / "movie.mkv" + src.write_bytes(b"x") + monkeypatch.setattr(stream.paths, "local_media_path", lambda db, fp: src) + # Distinct sentinels per knob so a crf<->max_height swap (both ints, both via get_int) fails. + monkeypatch.setattr( + stream.sysconfig, + "get_int", + lambda db, k: { + "plex_transcode_crf": 18, + "plex_transcode_max_height": 480, + "plex_max_transcodes": 4, + }[k], + ) + monkeypatch.setattr( + stream.sysconfig, + "get_str", + lambda db, k: {"plex_transcode_preset": "slow"}.get(k, "auto"), + ) + monkeypatch.setattr(stream, "wait_for", lambda path, timeout=20.0: True) + _CapturingProc.calls = [] + monkeypatch.setattr(stream.subprocess, "Popen", _CapturingProc) + return tmp_path + + def test_software_transcode_threads_the_knobs_into_the_command( + self, capturing_env, monkeypatch + ): + monkeypatch.setattr(stream, "_want_hw", lambda db, item, transcode: False) + s = stream.start_session( + db=None, item=_FakeItem(codec_video="mpeg4"), start_s=0.0, transcode=True + ) + assert s is not None and s.transcode is True and s.hw is False + (cmd, *_), kwargs = _CapturingProc.calls[0] + assert _venc(cmd) == "libx264" + assert cmd[cmd.index("-preset") + 1] == "slow" # plex_transcode_preset + assert cmd[cmd.index("-crf") + 1] == "18" # plex_transcode_crf, NOT max_height + assert cmd[cmd.index("-vf") + 1] == "scale=-2:min(ih\\,480)" # plex_transcode_max_height + # The CPU-bound child is niced via preexec_fn on POSIX (the API process itself must not be). + if os.name == "posix": + assert kwargs.get("preexec_fn") is stream._sw_child_preexec + + def test_hardware_transcode_uses_the_gpu_and_never_nices(self, capturing_env, monkeypatch): + monkeypatch.setattr(stream, "_want_hw", lambda db, item, transcode: True) + s = stream.start_session( + db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True + ) + assert s is not None and s.transcode is True and s.hw is True + (cmd, *_), kwargs = _CapturingProc.calls[0] + assert _venc(cmd) == "h264_vaapi" + assert "libx264" not in cmd + assert ( + "-vf" not in cmd or cmd[cmd.index("-vf") + 1] == "scale_vaapi=format=nv12" + ) # no SW cap + assert kwargs.get("preexec_fn") is None # GPU encode is cheap — never niced + + def test_remux_reads_no_transcode_knobs_and_stream_copies(self, capturing_env, monkeypatch): + # A remux start must stream-copy the video and spawn no niced child (the transcode knobs and + # the preexec nice are gated behind `transcode=True`). + monkeypatch.setattr(stream, "_want_hw", lambda db, item, transcode: False) + s = stream.start_session(db=None, item=_FakeItem(), start_s=0.0) # transcode defaults False + assert s is not None and s.transcode is False + (cmd, *_), kwargs = _CapturingProc.calls[0] + assert _venc(cmd) == "copy" + assert kwargs.get("preexec_fn") is None diff --git a/frontend/src/components/PlexInfo.tsx b/frontend/src/components/PlexInfo.tsx index 61cc1608..30712d86 100644 --- a/frontend/src/components/PlexInfo.tsx +++ b/frontend/src/components/PlexInfo.tsx @@ -425,7 +425,7 @@ export default function PlexInfo({ diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index 67fa33a4..f781926d 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -206,6 +206,10 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { setReady(false); setBuffering(false); setLoadError(null); + // Clear the prior item's transcode flag up front so the cold-start overlay can't show a stale + // "Preparing stream…" for a not-yet-known session while the plexSession request is in flight — + // it's re-set honestly from the response below. + setSessionIsTranscode(false); let sess; try { // Multi-audio → request the multi-rendition session (no single `audio` ordinal); the track is @@ -832,9 +836,13 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { {/* While a panel/menu is open or an auto-skip countdown is running, take over the browser/mouse Back button (history popstate) so it closes/cancels that instead of dropping to the grid. */} - {(menuOpen || tracksOpen || helpOpen || infoOpen || skipProgress != null) && ( - - )} + {(menuOpen || + tracksOpen || + helpOpen || + infoOpen || + statsOpen || + upNextOpen || + skipProgress != null) && } {/* Top bar */}
setHelpOpen(false)} className="p-1 rounded-lg hover:bg-white/10" - aria-label={t("plex.player.back")} + aria-label={t("plex.player.close")} > diff --git a/frontend/src/components/PlexStatsOverlay.tsx b/frontend/src/components/PlexStatsOverlay.tsx index ccfc5fd1..7f1e9c8f 100644 --- a/frontend/src/components/PlexStatsOverlay.tsx +++ b/frontend/src/components/PlexStatsOverlay.tsx @@ -109,13 +109,16 @@ export function PlexStatsOverlay({
-
+ {/* No aria-live here: the rows re-sample every second, so a live region would make a screen + reader announce the changing values continuously and drown out real player announcements. + The overlay is a role="region" (labelled above) a user reads on demand, not a status feed. */} +
{rows.map((r) => (
{t(r.labelKey)}
diff --git a/frontend/src/components/UpNextCard.dom.test.tsx b/frontend/src/components/UpNextCard.dom.test.tsx new file mode 100644 index 00000000..f7f6bab5 --- /dev/null +++ b/frontend/src/components/UpNextCard.dom.test.tsx @@ -0,0 +1,93 @@ +// @vitest-environment jsdom +// +// The up-next end-card's core behaviour (E6 S4): the countdown that auto-advances at ~10s (the whole +// point — it REPLACED the old instant jump, so a regression that fires early or never is exactly the +// bug to guard), the "Play now"/Cancel controls, and the episode heading's null season/episode +// fallback. i18n is key-passthrough; `../lib/api` is mocked so no network runs; the countdown uses +// performance.now, which vitest's fake timers advance alongside setInterval. +import { describe, it, expect, afterEach, vi } from "vitest"; +import { render, screen, fireEvent, cleanup, act } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; +}); + +vi.mock("../lib/api", () => ({ + api: { plexItem: vi.fn(), plexImageUrl: () => "/img" }, +})); + +import { UpNextCard } from "./UpNextCard"; +import { api } from "../lib/api"; + +afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.clearAllMocks(); +}); + +function renderCard(opts: { onAdvance?: () => void; onCancel?: () => void } = {}) { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + , + ); +} + +describe("UpNextCard", () => { + it("auto-advances after the ~10s countdown, and NOT before", () => { + vi.useFakeTimers(); + // A never-resolving detail query: the heading is irrelevant here and this avoids async state + // updates racing the fake clock. + (api.plexItem as ReturnType).mockReturnValue(new Promise(() => {})); + const onAdvance = vi.fn(); + renderCard({ onAdvance }); + + act(() => { + vi.advanceTimersByTime(9000); + }); + expect(onAdvance).not.toHaveBeenCalled(); // still counting down + + act(() => { + vi.advanceTimersByTime(1200); + }); + expect(onAdvance).toHaveBeenCalledTimes(1); // crossed zero → advanced exactly once + }); + + it("'Play now' advances immediately, without waiting for the countdown", () => { + (api.plexItem as ReturnType).mockReturnValue(new Promise(() => {})); + const onAdvance = vi.fn(); + renderCard({ onAdvance }); + fireEvent.click(screen.getByRole("button", { name: /playNow/ })); + expect(onAdvance).toHaveBeenCalledTimes(1); + }); + + it("Cancel dismisses via onCancel and does not advance", () => { + (api.plexItem as ReturnType).mockReturnValue(new Promise(() => {})); + const onAdvance = vi.fn(); + const onCancel = vi.fn(); + renderCard({ onAdvance, onCancel }); + fireEvent.click(screen.getByText("plex.player.upNext.cancel")); + expect(onCancel).toHaveBeenCalledTimes(1); + expect(onAdvance).not.toHaveBeenCalled(); + }); + + it("an episode heading falls back to S?·E? when season/episode are null", async () => { + (api.plexItem as ReturnType).mockResolvedValue({ + id: "e7", + kind: "episode", + show_title: "Westworld", + title: "The Adversary", + season_number: null, + episode_number: null, + }); + renderCard(); + expect(await screen.findByText("Westworld · S?·E?")).toBeTruthy(); + }); +}); diff --git a/frontend/src/components/UpNextCard.tsx b/frontend/src/components/UpNextCard.tsx index 67c7d36d..bab222f5 100644 --- a/frontend/src/components/UpNextCard.tsx +++ b/frontend/src/components/UpNextCard.tsx @@ -90,7 +90,14 @@ export function UpNextCard({ > - + { }); it("shows an empty classification honestly as a dash", () => { - expect(buildStatRows({ ...base, playable: "" }).find((r) => r.key === "mode")?.value).toBe(DASH); + expect(buildStatRows({ ...base, playable: "" }).find((r) => r.key === "mode")?.value).toBe( + DASH, + ); }); }); diff --git a/frontend/src/lib/plexStats.ts b/frontend/src/lib/plexStats.ts index b5ebbfd8..fc41393c 100644 --- a/frontend/src/lib/plexStats.ts +++ b/frontend/src/lib/plexStats.ts @@ -109,7 +109,11 @@ export function buildStatRows(s: PlexStatsInput): StatRow[] { const rows: StatRow[] = [ { key: "mode", labelKey: "plex.player.stats.mode", value: fmtMode(s.playable, s.hwEncoder) }, { key: "transport", labelKey: "plex.player.stats.transport", value: s.transport }, - { key: "res", labelKey: "plex.player.stats.resolution", value: fmtRes(s.videoWidth, s.videoHeight) }, + { + key: "res", + labelKey: "plex.player.stats.resolution", + value: fmtRes(s.videoWidth, s.videoHeight), + }, ]; if (s.transport === "hls") { rows.push( @@ -118,7 +122,11 @@ export function buildStatRows(s: PlexStatsInput): StatRow[] { labelKey: "plex.player.stats.codec", value: fmtCodec(s.hlsLevel?.videoCodec, s.hlsLevel?.audioCodec), }, - { key: "bitrate", labelKey: "plex.player.stats.bitrate", value: fmtBitrate(s.hlsLevel?.bitrate) }, + { + key: "bitrate", + labelKey: "plex.player.stats.bitrate", + value: fmtBitrate(s.hlsLevel?.bitrate), + }, { key: "level", labelKey: "plex.player.stats.level", value: fmtLevel(s.hlsLevel) }, ); }