fix(plex): make the HW->SW fallback supersede-check atomic; honest cull yield
Deep-review round 2 on the round-1 fix: - Race (medium): the round-1 identity guard did the supersede check and the respawn in TWO separate _lock sections. A newer same-key session installed in the gap was still torn down by _spawn's unconditional pop+kill (the round-1 comment even had the direction backwards). Fold the check into _spawn via a new replace_only= param: the identity test and the pop+replace now share ONE lock, so the respawn is a true no-op when superseded/culled. - Honest states (low): the superseded branch returned current_session(key), which is None when the key was cap-culled during our out-of-lock wait; the route turns None into a dishonest 'Media file not found on disk' 404 for a file that exists. Yield 'current_session(key) or s' so a culled session degrades to the route's retryable 'not available yet' path instead. - Hygiene: _want_hw docstring corrected (it is a test-only shim now; production decides via _select_encode) and the two stale _want_hw references refreshed (stream.py header, config.py plex_transcode_hw). - tests: the cap-cull no-respawn/no-None sub-case; the _opencl_present except OSError branch (glob raising, not just empty); tightened a tautological -vf assertion. Backend gate green (ruff + full pytest).
This commit is contained in:
@@ -265,7 +265,7 @@ class Settings(BaseSettings):
|
||||
# --- Transcode (playable="transcode") re-encode tuning (E6 S1) ---
|
||||
# Hardware-encoder policy: "auto" = VAAPI (h264_vaapi) when the iGPU render node is present AND the
|
||||
# source codec is hardware-decodable, else libx264 software; "on" = force VAAPI wherever the render
|
||||
# node exists (skip the decodability gate); "off" = always software. See plex/stream.py `_want_hw`.
|
||||
# node exists (skip the decodability gate); "off" = always software. See plex/stream.py `_select_encode`.
|
||||
plex_transcode_hw: str = "auto"
|
||||
# Software-fallback libx264 tuning (IGNORED on the VAAPI path). `preset` trades CPU for size; `crf`
|
||||
# trades quality for size (lower = better + bigger). `max_height` caps the SOFTWARE transcode's output
|
||||
|
||||
+26
-17
@@ -145,7 +145,7 @@ _HLS_TAIL = [
|
||||
|
||||
# --- Transcode (playable="transcode") video re-encode -----------------------------------------
|
||||
# The remux path stream-copies the video (`-c:v copy`); a non-h264 source can't be copied and must be
|
||||
# re-encoded to H.264. Two encoders, chosen per-session by `_want_hw`:
|
||||
# re-encoded to H.264. The encoder + tonemap are chosen per-session by `_select_encode`:
|
||||
# * VAAPI hardware (`h264_vaapi`) — the prod default once the iGPU is passed in. A full GPU pipeline
|
||||
# (decode + scale + encode all on-device) offloads the CPU almost entirely, so several concurrent
|
||||
# 1080p transcodes are fine and the API stays responsive.
|
||||
@@ -382,8 +382,8 @@ def _select_encode(db: DbSession, item: PlexItem, transcode: bool, src: Path) ->
|
||||
|
||||
|
||||
def _want_hw(db: DbSession, item: PlexItem, transcode: bool, src: Path) -> bool:
|
||||
"""The HW gate on its own (the `hw` half of `_select_encode`). Kept as a thin wrapper for the
|
||||
callers/tests that only care whether a session runs on the GPU at all."""
|
||||
"""The HW gate on its own (the `hw` half of `_select_encode`). A readability shim for the tests that
|
||||
assert only whether a session runs on the GPU (production decides via `_select_encode` directly)."""
|
||||
return _select_encode(db, item, transcode, src)[0]
|
||||
|
||||
|
||||
@@ -587,7 +587,15 @@ def start_session(
|
||||
# multi-audio: the video rendition is variant v0 → seg_{i}_0.ts; single-audio → seg_{i}.ts
|
||||
return directory / (f"seg_{i}_0.ts" if is_multi else f"seg_{i}.ts")
|
||||
|
||||
def _spawn(hw: bool, tonemap: bool = False) -> HlsSession:
|
||||
def _spawn(
|
||||
hw: bool, tonemap: bool = False, *, replace_only: "HlsSession | None" = None
|
||||
) -> "HlsSession | None":
|
||||
# `replace_only` makes the supersede identity-aware for the runtime fallback: the whole
|
||||
# check-and-replace happens under ONE _lock, so a newer same-key session (a concurrent
|
||||
# seek-restart) installed between the fallback's decision and here is never clobbered. When the
|
||||
# registered session is no longer `replace_only`, we abort and return None (the caller yields to
|
||||
# whoever is registered now). The INITIAL spawn passes replace_only=None and always supersedes —
|
||||
# that is the seek-restart's intended last-writer-wins.
|
||||
# Software-fallback encode tuning — read only on the SW transcode path (the VAAPI path and
|
||||
# remux ignore preset/crf/max_height, and remux must not touch the DB for them).
|
||||
preset, crf, max_height = "veryfast", 20, 0
|
||||
@@ -600,6 +608,8 @@ def start_session(
|
||||
# platform without fork (the API always runs in a Linux container in prod) it's simply omitted.
|
||||
preexec = _sw_child_preexec if (transcode and not hw and os.name == "posix") else None
|
||||
with _lock:
|
||||
if replace_only is not None and _sessions.get(key) is not replace_only:
|
||||
return None # superseded/culled while we waited out of lock — don't clobber the newer one
|
||||
old = _sessions.pop(key, None)
|
||||
if old is not None:
|
||||
_kill(old)
|
||||
@@ -647,22 +657,21 @@ def start_session(
|
||||
if not ready and s.hw:
|
||||
# We waited on _seg(0) OUTSIDE _lock, so `s.proc` dying is ambiguous: it may have failed on its
|
||||
# own (VAProfile/init/tonemap refusal), OR a concurrent same-key _spawn (a rapid seek-restart) or
|
||||
# _enforce_cap may have killed it to install/keep a newer session. Only fall back if THIS session
|
||||
# is still the registered one — otherwise respawning would tear down the newer session and (via
|
||||
# _spawn's supersede + _enforce_cap) breach the transcode cap. If superseded, yield to whoever is
|
||||
# registered now. (A tiny window remains between this check and _spawn's own lock; that just makes
|
||||
# the fallback lose to an even-newer seek, which is the correct seek-restart outcome anyway.)
|
||||
with _lock:
|
||||
superseded = _sessions.get(key) is not s
|
||||
if superseded:
|
||||
return current_session(key)
|
||||
# Runtime HW→SW fallback: respawn on libx264 (no HW, no tonemap) so a failed GPU session never
|
||||
# strands playback; the software path CPU-decodes and downconverts every source. s.hw is now
|
||||
# False, so the route honestly reports "CPU" and the player's Preparing-stream state stays correct.
|
||||
# _enforce_cap may have killed it to install/keep a newer session. Respawn on libx264 (no HW, no
|
||||
# tonemap) so a genuinely-failed GPU session never strands playback — but pass `replace_only=s` so
|
||||
# the respawn is a no-op if we've been superseded: the identity check and the pop+replace share
|
||||
# ONE lock in _spawn, so we can never tear down the newer session. s.hw is now False, so the route
|
||||
# honestly reports "CPU" and the player's Preparing-stream state stays correct.
|
||||
log.warning(
|
||||
"plex transcode: HW session produced no segments — falling back to software key=%s", key
|
||||
)
|
||||
s = _spawn(False, False)
|
||||
respawned = _spawn(False, False, replace_only=s)
|
||||
if respawned is None:
|
||||
# Superseded/culled mid-fallback — yield the session registered now (a newer seek-restart), or
|
||||
# our own now-dead session if the key was cap-culled (its segment requests degrade to the
|
||||
# route's retryable "not available yet", never a dishonest "file not found" from a None).
|
||||
return current_session(key) or s
|
||||
s = respawned
|
||||
ready = wait_for(_seg(0), timeout=25.0, proc=s.proc)
|
||||
if ready:
|
||||
# For a seek (start_s>0) learn the REAL start offset K from the first segment's absolute PTS,
|
||||
|
||||
@@ -590,9 +590,9 @@ class TestTranscodeStartSession:
|
||||
(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
|
||||
# Plain-HW path unconditionally emits the nv12 downconvert (a disjunction on "-vf not in cmd"
|
||||
# would silently pass if the filter were dropped — assert it is actually present).
|
||||
assert 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_hdr_tonemap_transcode_threads_the_opencl_graph_and_flags_the_session(
|
||||
@@ -704,6 +704,41 @@ class TestTranscodeStartSession:
|
||||
assert s is newer # yielded the currently-registered session, did not clobber it
|
||||
assert stream._sessions["12345"] is newer
|
||||
|
||||
def test_a_cap_culled_hw_session_does_not_respawn_and_is_not_reported_missing(
|
||||
self, capturing_env, monkeypatch
|
||||
):
|
||||
# F2/race, the cull sub-case: while we wait on seg_0 out of lock, cross-key cap pressure lets
|
||||
# _enforce_cap evict THIS session's key entirely (no replacement). The fallback must still not
|
||||
# respawn (that would re-breach the cap the cull just enforced), and must NOT return None — a None
|
||||
# is turned by the route into a dishonest "file not found" 404 for a file that exists. It yields
|
||||
# our own (now-dead) session instead, so the client's segment requests degrade to the retryable
|
||||
# "not available yet" path.
|
||||
monkeypatch.setattr(
|
||||
stream, "_select_encode", lambda db, item, transcode, src: (True, False)
|
||||
)
|
||||
captured = {}
|
||||
|
||||
def wait_then_cull(path, timeout=20.0, proc=None):
|
||||
captured["s"] = stream._sessions[
|
||||
"12345"
|
||||
] # the HW session the initial _spawn just registered
|
||||
del stream._sessions[
|
||||
"12345"
|
||||
] # simulate _enforce_cap dropping the LRU under cap pressure
|
||||
return False
|
||||
|
||||
monkeypatch.setattr(stream, "wait_for", wait_then_cull)
|
||||
s = stream.start_session(
|
||||
db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True
|
||||
)
|
||||
assert (
|
||||
len(_CapturingProc.calls) == 1
|
||||
) # no SW respawn — the cull is respected, cap not re-breached
|
||||
assert (
|
||||
s is captured["s"]
|
||||
) # our own session yielded, NOT None (which the route would 404 as missing)
|
||||
assert "12345" not in stream._sessions # the culled key stays gone
|
||||
|
||||
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`).
|
||||
@@ -790,9 +825,21 @@ class TestOpenclPresent:
|
||||
assert stream._opencl_present() is False
|
||||
|
||||
def test_false_and_no_raise_when_the_dir_is_missing(self, tmp_path, monkeypatch):
|
||||
# A missing dir: Path.glob yields nothing (no raise) → False.
|
||||
monkeypatch.setattr(stream, "_OPENCL_ICD_DIR", tmp_path / "nope")
|
||||
assert stream._opencl_present() is False
|
||||
|
||||
def test_false_when_glob_raises_oserror(self, monkeypatch):
|
||||
# The `except OSError` guard proper: an unreadable ICD dir (permission denied) makes glob RAISE,
|
||||
# not return empty. That must be swallowed to False, never propagated up through _select_encode
|
||||
# into start_session (which would crash HDR routing).
|
||||
class _BoomDir:
|
||||
def glob(self, pattern):
|
||||
raise OSError("permission denied")
|
||||
|
||||
monkeypatch.setattr(stream, "_OPENCL_ICD_DIR", _BoomDir())
|
||||
assert stream._opencl_present() is False
|
||||
|
||||
|
||||
class TestWaitForProcFastFail:
|
||||
"""wait_for(proc=…) must stop as soon as the ffmpeg proc exits (a VAAPI init refusal dies in ~1s
|
||||
|
||||
Reference in New Issue
Block a user