fix(plex): don't tonemap 10-bit SDR; identity-guard the HW→SW fallback
Deep-review round 1 on feature/plex-hdr-hw-tonemap: - F1 (correctness): _select_encode gated tonemap on bit-depth alone, so a 10-bit *SDR* (bt709) source got the HDR hable tone curve applied, visibly altering an SDR picture where only a 10->8-bit downconvert was needed. Split the ffprobe gate into a 3-way classifier (_probe_video_kind: hwsafe / hdr / sdr10); only true-HDR (PQ/HLG) takes the OpenCL tonemap graph, 10-bit SDR drops to software libx264 (an exact downconvert). Applies to both auto and on. - F2 (race): the runtime HW->SW fallback popped+killed _sessions[key] with no identity check. Because wait_for blocks OUTSIDE _lock, a concurrent same-key _spawn (rapid seek-restart) or _enforce_cap cull could kill this session; the fallback then respawned software over the NEWER session and breached the transcode cap. Guard the respawn: only fall back if _sessions[key] is still this session, else yield the currently-registered one. - tests: 3-way classifier (incl. HLG + empty-probe), real _opencl_present glob, real wait_for proc fast-fail branch, tonemap-variant fallback, the F1 regressions (auto + on), the supersede/no-respawn race, and the on/opencl matrix cell. Backend gate green (ruff + full pytest).
This commit is contained in:
@@ -304,14 +304,23 @@ class TestWantHw:
|
||||
def _policy(monkeypatch, mode):
|
||||
monkeypatch.setattr(stream.sysconfig, "get_str", lambda db, k: mode)
|
||||
|
||||
@staticmethod
|
||||
def _kind(monkeypatch, kind):
|
||||
# Pin the source classifier so the tests don't shell out to ffprobe. `kind` is one of
|
||||
# stream._VK_HWSAFE / _VK_HDR / _VK_SDR10.
|
||||
monkeypatch.setattr(stream, "_probe_video_kind", lambda src: kind)
|
||||
|
||||
@staticmethod
|
||||
def _hwsafe(monkeypatch, safe):
|
||||
# Default the bit-depth probe so the auto-decodable tests don't shell out to ffprobe.
|
||||
monkeypatch.setattr(stream, "_probe_video_hwsafe", lambda src: safe)
|
||||
# Back-compat convenience for the hw-gate tests that only care about 8-bit-SDR vs not: True →
|
||||
# _VK_HWSAFE, False → _VK_SDR10 (a non-HDR non-hwsafe class, so the hw half still resolves).
|
||||
monkeypatch.setattr(
|
||||
stream, "_probe_video_kind", lambda src: stream._VK_HWSAFE if safe else stream._VK_SDR10
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _opencl(monkeypatch, present):
|
||||
# Pin the OpenCL-runtime probe so the 10-bit routing doesn't depend on the CI box's /etc/OpenCL.
|
||||
# Pin the OpenCL-runtime probe so the routing doesn't depend on the CI box's /etc/OpenCL.
|
||||
monkeypatch.setattr(stream, "_opencl_present", lambda: present)
|
||||
|
||||
def test_no_hw_when_not_transcoding(self, monkeypatch):
|
||||
@@ -353,33 +362,45 @@ class TestWantHw:
|
||||
is False
|
||||
)
|
||||
|
||||
def test_auto_10bit_source_falls_back_to_software_without_opencl(self, monkeypatch):
|
||||
# A decodable HEVC that is 10-bit/HDR can't use scale_vaapi=format=nv12 (Gen 9.5), and with no
|
||||
# OpenCL runtime to tonemap on the GPU it drops to software libx264 (BUG 1's behaviour).
|
||||
def test_auto_hdr_source_falls_back_to_software_without_opencl(self, monkeypatch):
|
||||
# A decodable HEVC that is true-HDR (PQ/HLG) can't use scale_vaapi=format=nv12 (Gen 9.5), and with
|
||||
# no OpenCL runtime to tonemap on the GPU it drops to software libx264 (BUG 1's behaviour).
|
||||
self._policy(monkeypatch, "auto")
|
||||
self._hwsafe(monkeypatch, False)
|
||||
self._kind(monkeypatch, stream._VK_HDR)
|
||||
self._opencl(monkeypatch, False)
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert stream._select_encode(
|
||||
None, _FakeItem(codec_video="hevc"), transcode=True, src=self.SRC
|
||||
) == (False, False)
|
||||
|
||||
def test_auto_10bit_source_uses_hw_tonemap_with_opencl(self, monkeypatch):
|
||||
# With the OpenCL runtime present, the same 10-bit/HDR HEVC tone-maps on the GPU (VAAPI decode →
|
||||
# OpenCL tonemap → VAAPI encode) instead of dropping to CPU — the HW HDR path.
|
||||
def test_auto_hdr_source_uses_hw_tonemap_with_opencl(self, monkeypatch):
|
||||
# With the OpenCL runtime present, a true-HDR HEVC tone-maps on the GPU (VAAPI decode → OpenCL
|
||||
# tonemap → VAAPI encode) instead of dropping to CPU — the HW HDR path.
|
||||
self._policy(monkeypatch, "auto")
|
||||
self._hwsafe(monkeypatch, False)
|
||||
self._kind(monkeypatch, stream._VK_HDR)
|
||||
self._opencl(monkeypatch, True)
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert stream._select_encode(
|
||||
None, _FakeItem(codec_video="hevc"), transcode=True, src=self.SRC
|
||||
) == (True, True)
|
||||
|
||||
def test_auto_10bit_SDR_source_uses_software_not_tonemap(self, monkeypatch):
|
||||
# F1 regression: a 10-bit *SDR* (bt709) source is NOT HDR, so it must NOT get the hable HDR tone
|
||||
# curve even when the OpenCL runtime is present — that would visibly alter an SDR picture. It takes
|
||||
# software libx264 (an exact 10→8-bit downconvert), never (True, True).
|
||||
self._policy(monkeypatch, "auto")
|
||||
self._kind(monkeypatch, stream._VK_SDR10)
|
||||
self._opencl(monkeypatch, True)
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert stream._select_encode(
|
||||
None, _FakeItem(codec_video="hevc"), transcode=True, src=self.SRC
|
||||
) == (False, False)
|
||||
|
||||
def test_auto_8bit_source_uses_plain_hw_not_tonemap(self, monkeypatch):
|
||||
# An 8-bit SDR source takes the plain full-GPU scale path — never the tonemap graph — even where
|
||||
# OpenCL is available.
|
||||
self._policy(monkeypatch, "auto")
|
||||
self._hwsafe(monkeypatch, True)
|
||||
self._kind(monkeypatch, stream._VK_HWSAFE)
|
||||
self._opencl(monkeypatch, True)
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert stream._select_encode(
|
||||
@@ -399,6 +420,9 @@ class TestWantHw:
|
||||
# HW→SW fallback still backstops it). Render node still required — HW is impossible without it.
|
||||
self._policy(monkeypatch, "on")
|
||||
self._hwsafe(monkeypatch, False) # would veto in auto; "on" ignores it
|
||||
self._opencl(
|
||||
monkeypatch, False
|
||||
) # pin: tonemap-half is irrelevant here, keep it deterministic
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert (
|
||||
stream._want_hw(None, _FakeItem(codec_video="av1"), transcode=True, src=self.SRC)
|
||||
@@ -410,17 +434,40 @@ class TestWantHw:
|
||||
is False
|
||||
)
|
||||
|
||||
def test_on_still_picks_the_tonemap_graph_for_a_10bit_source(self, monkeypatch):
|
||||
# "on" trusts the GPU but must still route a 10-bit/HDR source through the tonemap graph (plain
|
||||
def test_on_still_picks_the_tonemap_graph_for_an_hdr_source(self, monkeypatch):
|
||||
# "on" trusts the GPU but must still route a true-HDR source through the tonemap graph (plain
|
||||
# scale_vaapi would hit the VPP wall) when OpenCL is present.
|
||||
self._policy(monkeypatch, "on")
|
||||
self._hwsafe(monkeypatch, False)
|
||||
self._kind(monkeypatch, stream._VK_HDR)
|
||||
self._opencl(monkeypatch, True)
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert stream._select_encode(
|
||||
None, _FakeItem(codec_video="hevc"), transcode=True, src=self.SRC
|
||||
) == (True, True)
|
||||
|
||||
def test_on_10bit_SDR_uses_plain_hw_not_tonemap(self, monkeypatch):
|
||||
# F1 regression under "on": a 10-bit SDR source is NOT HDR, so even with OpenCL present "on" must
|
||||
# pick plain HW (True, False) — the HDR tone curve must never touch an SDR picture.
|
||||
self._policy(monkeypatch, "on")
|
||||
self._kind(monkeypatch, stream._VK_SDR10)
|
||||
self._opencl(monkeypatch, True)
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert stream._select_encode(
|
||||
None, _FakeItem(codec_video="hevc"), transcode=True, src=self.SRC
|
||||
) == (True, False)
|
||||
|
||||
def test_on_without_opencl_is_plain_hw(self, monkeypatch):
|
||||
# "on" with no OpenCL runtime → plain HW, tonemap=False, regardless of source class (the runtime
|
||||
# HW→SW fallback backstops a source the plain path can't actually handle). Pins the tonemap half
|
||||
# of the "on" tuple, which test_on_forces_hw_and_skips_both_source_gates only checks via _want_hw.
|
||||
self._policy(monkeypatch, "on")
|
||||
self._kind(monkeypatch, stream._VK_HDR) # even HDR: no runtime → can't tonemap on the GPU
|
||||
self._opencl(monkeypatch, False)
|
||||
monkeypatch.setattr(stream, "_render_node_present", lambda: True)
|
||||
assert stream._select_encode(
|
||||
None, _FakeItem(codec_video="hevc"), transcode=True, src=self.SRC
|
||||
) == (True, False)
|
||||
|
||||
|
||||
class TestColdStartWarmup:
|
||||
"""The cold-start buffer warm-up: before reporting a session ready, start_session waits for the
|
||||
@@ -594,6 +641,69 @@ class TestTranscodeStartSession:
|
||||
if os.name == "posix":
|
||||
assert sw_kwargs.get("preexec_fn") is stream._sw_child_preexec # the SW child is niced
|
||||
|
||||
def test_a_hw_TONEMAP_session_that_produces_nothing_falls_back_to_software(
|
||||
self, capturing_env, monkeypatch
|
||||
):
|
||||
# The fallback gate is `not ready and s.hw`, shared by plain-HW and tonemap sessions — but only
|
||||
# the plain-HW variant was covered. Pin the tonemap variant too: a (True, True) session that
|
||||
# emits nothing (an OpenCL/tonemap init refusal) must respawn on libx264 and end hw=False,
|
||||
# tonemap=False, so an HDR item still plays on CPU.
|
||||
monkeypatch.setattr(stream, "_select_encode", lambda db, item, transcode, src: (True, True))
|
||||
seen = {"n": 0}
|
||||
|
||||
def flaky_wait(path, timeout=20.0, proc=None):
|
||||
seen["n"] += 1
|
||||
return (
|
||||
seen["n"] > 1
|
||||
) # the first seg-0 wait (HW tonemap) fails; every wait after succeeds
|
||||
|
||||
monkeypatch.setattr(stream, "wait_for", flaky_wait)
|
||||
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 False and s.tonemap is False
|
||||
assert len(_CapturingProc.calls) == 2 # the HW tonemap attempt, then the SW respawn
|
||||
(hw_cmd, *_), _ = _CapturingProc.calls[0]
|
||||
(sw_cmd, *_), _ = _CapturingProc.calls[1]
|
||||
assert (
|
||||
"tonemap_opencl=" in hw_cmd[hw_cmd.index("-vf") + 1]
|
||||
) # first attempt built the OpenCL graph
|
||||
assert _venc(sw_cmd) == "libx264" # respawn is plain software, no tonemap
|
||||
|
||||
def test_a_superseded_hw_session_does_not_respawn_over_the_newer_one(
|
||||
self, capturing_env, monkeypatch
|
||||
):
|
||||
# F2/race: start_session waits on seg_0 OUTSIDE _lock. If a concurrent same-key start (a rapid
|
||||
# seek-restart) or _enforce_cap replaces _sessions[key] while we wait, our proc dying must NOT
|
||||
# trigger the SW fallback — that would tear down the NEWER session and breach the transcode cap.
|
||||
# The identity guard must detect it's been superseded and yield the currently-registered session.
|
||||
monkeypatch.setattr(
|
||||
stream, "_select_encode", lambda db, item, transcode, src: (True, False)
|
||||
)
|
||||
|
||||
class _Newer: # stands in for the session a concurrent _spawn registered
|
||||
hw = True
|
||||
tonemap = False
|
||||
transcode = True
|
||||
last_access = 0.0
|
||||
|
||||
newer = _Newer()
|
||||
|
||||
def wait_then_supersede(path, timeout=20.0, proc=None):
|
||||
# Simulate the concurrent replacement happening during our out-of-lock wait, then report the
|
||||
# (now-killed) HW proc produced nothing.
|
||||
stream._sessions["12345"] = newer
|
||||
return False
|
||||
|
||||
monkeypatch.setattr(stream, "wait_for", wait_then_supersede)
|
||||
s = stream.start_session(
|
||||
db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True
|
||||
)
|
||||
# No SW respawn — only the initial HW _spawn ran; the newer session was left intact.
|
||||
assert len(_CapturingProc.calls) == 1
|
||||
assert s is newer # yielded the currently-registered session, did not clobber it
|
||||
assert stream._sessions["12345"] is newer
|
||||
|
||||
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`).
|
||||
@@ -607,9 +717,10 @@ class TestTranscodeStartSession:
|
||||
assert kwargs.get("preexec_fn") is None
|
||||
|
||||
|
||||
class TestProbeVideoHwsafe:
|
||||
"""The ffprobe-based 8-bit-SDR gate that keeps 10-bit/HDR sources off the VAAPI pipeline (BUG 1):
|
||||
scale_vaapi=format=nv12 can't convert them on Gen 9.5, so they must take software libx264."""
|
||||
class TestProbeVideoKind:
|
||||
"""The ffprobe-based source classifier that routes the transcode pipeline: 8-bit SDR → plain VAAPI;
|
||||
true-HDR (PQ/HLG) → the OpenCL tonemap graph; 10-bit *SDR* → software (an exact downconvert, NEVER
|
||||
the HDR tone curve — the F1 distinction). Exercises the REAL function body (not a monkeypatch)."""
|
||||
|
||||
@staticmethod
|
||||
def _probe(monkeypatch, stdout):
|
||||
@@ -619,31 +730,94 @@ class TestProbeVideoHwsafe:
|
||||
|
||||
monkeypatch.setattr(stream.subprocess, "run", lambda *a, **k: _R())
|
||||
|
||||
def test_8bit_sdr_is_hw_safe(self, monkeypatch):
|
||||
def test_8bit_sdr_is_hwsafe(self, monkeypatch):
|
||||
self._probe(monkeypatch, "pix_fmt=yuv420p\nbits_per_raw_sample=8\ncolor_transfer=bt709\n")
|
||||
assert stream._probe_video_hwsafe(Path("/x")) is True
|
||||
assert stream._probe_video_kind(Path("/x")) == stream._VK_HWSAFE
|
||||
|
||||
def test_10bit_pix_fmt_is_not_hw_safe(self, monkeypatch):
|
||||
# p010/yuv420p10le breaks the VPP format pass even when bits_per_raw_sample is absent.
|
||||
def test_10bit_sdr_pix_fmt_is_sdr10(self, monkeypatch):
|
||||
# yuv420p10le with a non-HDR transfer (bt2020-10 is NOT PQ/HLG) = 10-bit SDR → software, not
|
||||
# tonemap. Signalled by the pix_fmt suffix even when bits_per_raw_sample is absent.
|
||||
self._probe(monkeypatch, "pix_fmt=yuv420p10le\ncolor_transfer=bt2020-10\n")
|
||||
assert stream._probe_video_hwsafe(Path("/x")) is False
|
||||
assert stream._probe_video_kind(Path("/x")) == stream._VK_SDR10
|
||||
|
||||
def test_bits_per_raw_sample_over_8_is_not_hw_safe(self, monkeypatch):
|
||||
self._probe(monkeypatch, "pix_fmt=yuv420p\nbits_per_raw_sample=10\n")
|
||||
assert stream._probe_video_hwsafe(Path("/x")) is False
|
||||
def test_bits_per_raw_sample_over_8_sdr_is_sdr10(self, monkeypatch):
|
||||
self._probe(monkeypatch, "pix_fmt=yuv420p\nbits_per_raw_sample=10\ncolor_transfer=bt709\n")
|
||||
assert stream._probe_video_kind(Path("/x")) == stream._VK_SDR10
|
||||
|
||||
def test_hdr_transfer_is_not_hw_safe(self, monkeypatch):
|
||||
# Even an 8-bit-tagged PQ/HLG source can't be tonemapped to nv12 as configured.
|
||||
def test_pq_transfer_is_hdr(self, monkeypatch):
|
||||
# PQ (smpte2084) is true HDR — needs a real tonemap, even if only 8-bit-tagged.
|
||||
self._probe(
|
||||
monkeypatch, "pix_fmt=yuv420p\nbits_per_raw_sample=8\ncolor_transfer=smpte2084\n"
|
||||
monkeypatch, "pix_fmt=yuv420p10le\nbits_per_raw_sample=10\ncolor_transfer=smpte2084\n"
|
||||
)
|
||||
assert stream._probe_video_hwsafe(Path("/x")) is False
|
||||
assert stream._probe_video_kind(Path("/x")) == stream._VK_HDR
|
||||
|
||||
def test_probe_failure_assumes_hw_safe(self, monkeypatch):
|
||||
# ffprobe missing/erroring → assume safe; the runtime HW→SW fallback is the real backstop, so a
|
||||
def test_hlg_transfer_is_hdr(self, monkeypatch):
|
||||
# HLG (arib-std-b67) is the other HDR transfer — previously untested; it must classify as HDR.
|
||||
self._probe(
|
||||
monkeypatch,
|
||||
"pix_fmt=yuv420p10le\nbits_per_raw_sample=10\ncolor_transfer=arib-std-b67\n",
|
||||
)
|
||||
assert stream._probe_video_kind(Path("/x")) == stream._VK_HDR
|
||||
|
||||
def test_empty_probe_output_is_hwsafe(self, monkeypatch):
|
||||
# A probe that returns nothing (no streams / stripped metadata) → assume hwsafe; the runtime
|
||||
# HW→SW fallback backstops a wrong guess.
|
||||
self._probe(monkeypatch, "")
|
||||
assert stream._probe_video_kind(Path("/x")) == stream._VK_HWSAFE
|
||||
|
||||
def test_probe_failure_assumes_hwsafe(self, monkeypatch):
|
||||
# ffprobe missing/erroring → assume hwsafe; the runtime HW→SW fallback is the real backstop, so a
|
||||
# probe miss costs at most one wasted HW attempt, never a stranded item.
|
||||
def boom(*a, **k):
|
||||
raise OSError("no ffprobe")
|
||||
|
||||
monkeypatch.setattr(stream.subprocess, "run", boom)
|
||||
assert stream._probe_video_hwsafe(Path("/x")) is True
|
||||
assert stream._probe_video_kind(Path("/x")) == stream._VK_HWSAFE
|
||||
|
||||
|
||||
class TestOpenclPresent:
|
||||
"""The OpenCL-runtime probe (_opencl_present) — the precondition for the HW HDR tonemap graph. Drives
|
||||
the REAL glob against a temp ICD dir instead of monkeypatching the function, so a regression to the
|
||||
ICD path / glob / OSError guard would be caught (this routes every HDR source on prod)."""
|
||||
|
||||
def test_true_when_an_icd_file_is_registered(self, tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(stream, "_OPENCL_ICD_DIR", tmp_path)
|
||||
(tmp_path / "intel.icd").write_text("libintelocl.so\n")
|
||||
assert stream._opencl_present() is True
|
||||
|
||||
def test_false_when_the_dir_is_empty(self, tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(stream, "_OPENCL_ICD_DIR", tmp_path)
|
||||
assert stream._opencl_present() is False
|
||||
|
||||
def test_false_and_no_raise_when_the_dir_is_missing(self, tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(stream, "_OPENCL_ICD_DIR", tmp_path / "nope")
|
||||
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
|
||||
producing nothing) instead of blocking the whole timeout — that fast-fail is what keeps the HW→SW
|
||||
fallback from turning into a 25s stall. Exercises the REAL wait_for (every other test monkeypatches
|
||||
it wholesale, so this branch would otherwise never run)."""
|
||||
|
||||
class _DeadProc:
|
||||
def poll(self):
|
||||
return 1 # already exited non-zero
|
||||
|
||||
def test_returns_true_when_the_segment_exists_despite_a_dead_proc(self, tmp_path):
|
||||
seg = tmp_path / "seg_0.ts"
|
||||
seg.write_bytes(b"x")
|
||||
# The proc exited but DID write a first segment → ready. (The final existence re-check.)
|
||||
assert stream.wait_for(seg, timeout=5.0, proc=self._DeadProc()) is True
|
||||
|
||||
def test_returns_false_fast_when_a_dead_proc_wrote_nothing(self, tmp_path):
|
||||
# Missing segment + dead proc → give up immediately, not after `timeout`. Assert it's fast by
|
||||
# using a long timeout the test would visibly hang on if the fast-fail branch regressed.
|
||||
seg = tmp_path / "seg_0.ts"
|
||||
assert stream.wait_for(seg, timeout=30.0, proc=self._DeadProc()) is False
|
||||
|
||||
def test_empty_segment_from_a_dead_proc_is_not_ready(self, tmp_path):
|
||||
# A zero-byte segment is not a real segment (size > 0 required).
|
||||
seg = tmp_path / "seg_0.ts"
|
||||
seg.write_bytes(b"")
|
||||
assert stream.wait_for(seg, timeout=30.0, proc=self._DeadProc()) is False
|
||||
|
||||
Reference in New Issue
Block a user