fix(player): give YouTube's own fullscreen a working path + round-6 findings

The reported bug: after F, the embed's own fullscreen button is dead. Root cause
measured in Chrome — the browser's fullscreen lock sits on OUR wrapper, so
YouTube's button would need a nested request from a cross-origin frame. Handing
the lock to the iframe fixes it, but `iframe.requestFullscreen()` only resolves
from a CLICK (from a key press the promise stays pending forever, and awaiting
it spends the activation so a fallback is denied too).

So: F keeps fullscreening our wrapper (unchanged, our overlays stay visible) and
a new toolbar button hands the lock to the iframe — verified in real Chrome:
document.fullscreenElement is the IFRAME at 1920x1024, i.e. a plain YouTube
fullscreen where its own controls work.

Round-6 review findings, all fixed:
- one _rate_limiter + one _cancel_poll shared by the yt-dlp hook and run_ffmpeg
  (was three hand-rolled throttles; cancel latency differed 2s vs 1s)
- stepVolume takes {delta, fallback} — two adjacent number args were silently
  transposable
- volume gestures before the player is ready are honoured instead of dropped
- normalizeVolume's fallback is typed unknown (its guard was unreachable) and
  applyVolume takes a number again
- the throttle tests inject a clock instead of patching global time.monotonic
This commit is contained in:
2026-07-24 02:16:58 +02:00
parent c4942eb590
commit d8d82e47d0
7 changed files with 149 additions and 69 deletions
+50 -36
View File
@@ -273,15 +273,14 @@ def _fill_asset_meta_early(asset_id: int, info: dict) -> None:
def _make_progress_hook(job_id: int, asset_id: int):
state = {"last_db": 0.0, "last_check": 0.0, "meta_done": False}
state = {"meta_done": False}
cancelled = _cancel_poll(job_id) # same policy as the ffmpeg path
allow_write = _rate_limiter(_DB_WRITE_INTERVAL_S)
def hook(d: dict) -> None:
now = time.monotonic()
# Cooperative pause/cancel: if the job is no longer 'running', abort the download.
if now - state["last_check"] >= 2.0:
state["last_check"] = now
if _job_status(job_id) not in ("running", None):
raise _Aborted
if cancelled():
raise _Aborted
if not state["meta_done"] and d.get("info_dict"):
state["meta_done"] = True
try:
@@ -290,9 +289,8 @@ def _make_progress_hook(job_id: int, asset_id: int):
pass
if d.get("status") != "downloading":
return
if now - state["last_db"] < 1.0:
if not allow_write():
return
state["last_db"] = now
# yt-dlp downloads the video and audio streams SEPARATELY (each 0→100%), then merges.
# Label the phase so the user understands the bar resetting between streams instead of
# seeing a mysterious 95%→5% jump.
@@ -319,46 +317,62 @@ def _make_progress_hook(job_id: int, asset_id: int):
return hook
# ffmpeg's `-progress pipe:1` emits a block of ~12 lines every ~0.5s, and run_ffmpeg calls both of
# its callbacks once per LINE. Each of these is a database round-trip (`_job_status` opens a session
# and SELECTs; `_set_job` opens one and UPDATE+COMMITs), so an unthrottled pair costs ~10^5 queries
# on a long re-encode. Both are therefore rate-limited to ~1s — the same 1s DB throttle the
# download progress hook already applies (see `_make_progress_hook`).
_FFMPEG_CALLBACK_INTERVAL_S = 1.0
# Both progress sources call back FAR faster than the database should be touched: yt-dlp fires its
# hook per chunk, and ffmpeg's `-progress pipe:1` emits a block of ~12 lines every ~0.5s while
# run_ffmpeg calls its callbacks once per LINE. Every callback here is a round-trip (`_job_status`
# opens a session and SELECTs; `_set_job` opens one and UPDATE+COMMITs), so unthrottled they cost
# ~10^5 queries on a long job. ONE interval, ONE implementation, used by both paths — three
# hand-rolled copies is how the ffmpeg side ended up unthrottled while the download side was fine.
_DB_WRITE_INTERVAL_S = 1.0
_CANCEL_POLL_INTERVAL_S = 1.0
def _ffmpeg_progress_writer(job_id: int, phase: str):
"""`on_progress` for run_ffmpeg: persist at most one row write per second, and only when the
whole-percent value actually changed (the bar shows integers)."""
state = {"at": 0.0, "pct": -1}
def _rate_limiter(interval_s: float, *, now=time.monotonic):
"""`should_run()` → True at most once per `interval_s` (the first call always passes).
`now` is injectable so tests drive a clock instead of patching the global one."""
state = {"at": 0.0}
def write(pct: float) -> None:
whole = int(pct)
now = time.monotonic()
if whole == state["pct"] or now - state["at"] < _FFMPEG_CALLBACK_INTERVAL_S:
return
state["at"] = now
state["pct"] = whole
_set_job(job_id, progress=whole, phase=phase)
def should_run() -> bool:
t = now()
if state["at"] and t - state["at"] < interval_s:
return False
state["at"] = t
return True
return write
return should_run
def _ffmpeg_cancel_poll(job_id: int):
"""`should_cancel` for run_ffmpeg: ask the DB at most once a second and reuse the answer in
between. Cancellation is sticky, so once it says yes it never queries again."""
state = {"at": 0.0, "cancelled": False}
def _cancel_poll(job_id: int, interval_s: float = _CANCEL_POLL_INTERVAL_S, *, now=time.monotonic):
"""Cooperative "has this job been paused/cancelled?" poll, rate-limited and sticky: once it
says yes it never queries again. Shared by the yt-dlp hook and run_ffmpeg so cancel latency is
one policy, not one per call site."""
allow = _rate_limiter(interval_s, now=now)
state = {"cancelled": False}
def cancelled() -> bool:
now = time.monotonic()
if not state["cancelled"] and now - state["at"] >= _FFMPEG_CALLBACK_INTERVAL_S:
state["at"] = now
if not state["cancelled"] and allow():
state["cancelled"] = _job_status(job_id) not in ("running", None)
return state["cancelled"]
return cancelled
def _ffmpeg_progress_writer(job_id: int, phase: str, *, now=time.monotonic):
"""`on_progress` for run_ffmpeg: persist at most one row write per interval, and only when the
whole-percent value actually changed (the bar shows integers)."""
allow = _rate_limiter(_DB_WRITE_INTERVAL_S, now=now)
state = {"pct": -1}
def write(pct: float) -> None:
whole = int(pct)
if whole == state["pct"] or not allow():
return
state["pct"] = whole
_set_job(job_id, progress=whole, phase=phase)
return write
def _pp_phase(pp: str) -> str:
"""Map a yt-dlp postprocessor class name to a user-facing phase label. ffmpeg post-steps
aren't byte-progress operations (no %), so the UI shows the step name + an indeterminate
@@ -533,7 +547,7 @@ def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict
editmod.build_browser_safe_cmd(produced, dest, reenc_v, reenc_a),
total_s=total_s,
on_progress=_ffmpeg_progress_writer(job_id, "processing"),
should_cancel=_ffmpeg_cancel_poll(job_id),
should_cancel=_cancel_poll(job_id),
)
produced.unlink(missing_ok=True)
# Keep the stored asset metadata truthful about what's now on disk.
@@ -712,7 +726,7 @@ def _process_edit(job_id: int, asset_id: int) -> None:
cmd,
total_s,
on_progress=_ffmpeg_progress_writer(job_id, "editing"),
should_cancel=_ffmpeg_cancel_poll(job_id),
should_cancel=_cancel_poll(job_id),
)
if not dest_staging.exists():
raise RuntimeError("ffmpeg produced no output file")
+5 -9
View File
@@ -26,8 +26,7 @@ class TestProgressWriter:
writes = []
clock = {"t": 1000.0}
monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f))
monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"])
write = worker._ffmpeg_progress_writer(7, "processing")
write = worker._ffmpeg_progress_writer(7, "processing", now=lambda: clock["t"])
write(10)
write(11) # same second → dropped
clock["t"] += 1.5
@@ -38,8 +37,7 @@ class TestProgressWriter:
writes = []
clock = {"t": 1000.0}
monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f))
monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"])
write = worker._ffmpeg_progress_writer(7, "processing")
write = worker._ffmpeg_progress_writer(7, "processing", now=lambda: clock["t"])
write(10.1)
clock["t"] += 5
write(10.9) # still 10% — the bar shows integers
@@ -50,7 +48,7 @@ class TestCancelPoll:
def test_asks_once_then_reuses_the_answer(self, monkeypatch):
calls = []
monkeypatch.setattr(worker, "_job_status", lambda job_id: calls.append(job_id) or "running")
cancelled = worker._ffmpeg_cancel_poll(7)
cancelled = worker._cancel_poll(7)
for _ in range(40):
assert cancelled() is False
assert len(calls) == 1
@@ -64,8 +62,7 @@ class TestCancelPoll:
return "cancelled"
monkeypatch.setattr(worker, "_job_status", status)
monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"])
cancelled = worker._ffmpeg_cancel_poll(7)
cancelled = worker._cancel_poll(7, now=lambda: clock["t"])
assert cancelled() is True
clock["t"] += 60 # cancellation is sticky: no re-query, and no un-cancelling
assert cancelled() is True
@@ -75,8 +72,7 @@ class TestCancelPoll:
clock = {"t": 1000.0}
state = {"status": "running"}
monkeypatch.setattr(worker, "_job_status", lambda job_id: state["status"])
monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"])
cancelled = worker._ffmpeg_cancel_poll(7)
cancelled = worker._cancel_poll(7, now=lambda: clock["t"])
assert cancelled() is False
state["status"] = "paused"
clock["t"] += 1.0 # the throttle must not delay a cancel by more than its interval
+73 -11
View File
@@ -11,6 +11,7 @@ import {
ChevronLeft,
ChevronRight,
ExternalLink,
Maximize,
Repeat,
Repeat1,
Settings,
@@ -209,11 +210,49 @@ export default function PlayerModal({
p.pauseVideo?.(); // 1 === playing
else p.playVideo?.();
};
/** The element F (and our own button) fullscreens: the PLAYER IFRAME, not our stage wrapper.
*
* Fullscreening the wrapper looked equivalent — the video filled the screen either way — but it
* left the browser's fullscreen lock on an element YouTube knows nothing about, so YouTube's own
* fullscreen button had to make a NESTED request from inside a cross-origin frame while we held
* that lock. That is the user-reported bug: after F, the embed's own fullscreen control is dead.
* Targeting the iframe makes our shortcut and YouTube's button the same operation on the same
* element — i.e. exactly a plain YouTube embed, where the button demonstrably works.
*
* Trade-off, deliberate: our on-player volume flash and the "YouTube controls active" badge sit
* OUTSIDE the iframe, so they aren't painted in fullscreen — YouTube's own volume UI covers that
* case. The stage stays as the fallback for the window between mount and the player being ready.
*/
const playerIframe = (): HTMLElement | null => {
const p = playerRef.current;
const frame = p && typeof p.getIframe === "function" ? p.getIframe() : null;
return (frame as HTMLElement | null) ?? null;
};
/** F / our stage-fullscreen: puts OUR wrapper on screen, so the volume flash and the queue
* chrome stay visible. YouTube's own fullscreen button is inert while we hold that lock — use
* `enterNativeFullscreen` (the toolbar button) when you want YouTube's own controls. */
const toggleFullscreen = () => {
const el = stageRef.current;
if (!el) return;
if (document.fullscreenElement) document.exitFullscreen?.();
else el.requestFullscreen?.();
else stageRef.current?.requestFullscreen?.();
};
/** True YouTube fullscreen: hand the browser's fullscreen lock to the PLAYER IFRAME, so the
* embed behaves exactly like any YouTube embed — its own control bar, its own fullscreen
* button, its quality menu, all live.
*
* Why a button and not the F key: measured in Chrome (2026-07-24), `iframe.requestFullscreen()`
* resolves from a CLICK but stays pending forever when it comes from a key press — the request
* must be delegated to the out-of-process frame, and a keyboard gesture in the parent document
* doesn't carry that delegation. Awaiting it and falling back is not an option either: the await
* spends the transient activation, so the second request is denied and the key does nothing.
* A click is the gesture that provably works, so this is a click-only affordance. */
const enterNativeFullscreen = () => {
const frame = playerIframe();
if (!frame?.requestFullscreen) return;
if (document.fullscreenElement) document.exitFullscreen?.();
frame.requestFullscreen?.().catch(() => {
// Denied (an old browser, a policy) — keep the old behaviour rather than nothing at all.
stageRef.current?.requestFullscreen?.();
});
};
const flashVolume = (level: number) => {
setVolumeUi(level);
@@ -228,8 +267,9 @@ export default function PlayerModal({
};
/** Push a level (0100) to the player and remember it; 0 also mutes (a player left at zero
* volume but unmuted shows YouTube's speaker icon as ON, which reads as broken). */
const applyVolume = (level: unknown, flash = true) => {
// An unusable level (a player that answered NaN) falls back to the LAST GOOD one, never to the
const applyVolume = (level: number, flash = true) => {
// Callers hand over an already-normalised level (stepVolume / the stored value), but the
// player can still answer NaN, so re-normalise here against the LAST GOOD level — never the
// default: jumping a quiet session to full blast is the surprise this feature exists to avoid.
const next = normalizeVolume(level, volumeRef.current);
const p = playerRef.current;
@@ -246,8 +286,10 @@ export default function PlayerModal({
if (flash) flashVolume(next);
};
const nudgeVolume = (delta: number) => {
// No early return when the player isn't ready yet: the level lives in `volumeRef` and onReady
// applies it, so a wheel spin in the first second after opening (the natural reaction to a
// video that starts loud) now lands instead of being silently dropped.
const p = playerRef.current;
if (!p || typeof p.getVolume !== "function") return;
// Whose level do we step from? Our own ref, EXCEPT when the gesture starts fresh — then read
// the player, because the user may have moved YouTube's own slider in the meantime.
// `getVolume()` is answered across the iframe boundary and lags our `setVolume` by a beat, so
@@ -256,8 +298,10 @@ export default function PlayerModal({
const now = Date.now();
const continuing = now - lastNudgeAtRef.current < 1000;
lastNudgeAtRef.current = now;
const cur = continuing ? volumeRef.current : p.isMuted?.() ? 0 : p.getVolume?.();
applyVolume(stepVolume(cur, delta, volumeRef.current), true);
const live =
p && typeof p.getVolume === "function" ? (p.isMuted?.() ? 0 : p.getVolume()) : null;
const cur = continuing || live === null ? volumeRef.current : live;
applyVolume(stepVolume(cur, { delta, fallback: volumeRef.current }));
};
// The player can navigate to other videos (YouTube links in a description). The
@@ -530,10 +574,16 @@ export default function PlayerModal({
return () => el.removeEventListener("wheel", onWheel);
}, []);
// Track whether we're the fullscreen element (F, our button, or the browser's own Esc exit).
// Track whether the fullscreen element is ours — the player iframe (see fullscreenTarget) or,
// before the player exists, the stage. Covers F, our button, YouTube's own button and the
// browser's Esc exit alike, since all of them land on one of those two elements.
useEffect(() => {
const onChange = () => {
const ours = !!document.fullscreenElement && document.fullscreenElement === stageRef.current;
const fsEl = document.fullscreenElement;
// Refs only — this listener is bound once, so it must not close over render values.
const p = playerRef.current;
const frame = p && typeof p.getIframe === "function" ? p.getIframe() : null;
const ours = !!fsEl && (fsEl === stageRef.current || fsEl === frame);
isFullscreenRef.current = ours; // read by the once-bound keydown handler
setIsFullscreen(ours); // drives the overlay's pointer-events
};
@@ -1067,10 +1117,22 @@ export default function PlayerModal({
</div>
)}
{/* True YouTube fullscreen — see enterNativeFullscreen. Deliberately a CLICK-only
affordance (the iframe fullscreen request is only reliable from a click), and the
only way to reach YouTube's own control bar + fullscreen button; F fullscreens our
wrapper instead, which keeps our overlays but leaves YouTube's controls inert. */}
<button
onClick={enterNativeFullscreen}
title={t("player.nativeFullscreenHint")}
aria-label={t("player.nativeFullscreen")}
className="ml-auto shrink-0 inline-flex items-center justify-center w-9 h-9 rounded-lg text-muted hover:text-fg hover:bg-surface border border-border transition"
>
<Maximize className="w-4 h-4" />
</button>
{!navigated && (
<AddToPlaylist
videoId={active.id}
className="ml-auto shrink-0 inline-flex items-center justify-center w-9 h-9 rounded-lg text-muted hover:text-fg hover:bg-surface border border-border transition"
className="shrink-0 inline-flex items-center justify-center w-9 h-9 rounded-lg text-muted hover:text-fg hover:bg-surface border border-border transition"
/>
)}
{!navigated && (
+2
View File
@@ -36,5 +36,7 @@
"embedDisabledBody": "The owner disabled playback on other sites (common for auto-generated “Topic” music tracks). It still plays on YouTube.",
"openOnYoutube": "Open on YouTube",
"shortcutsHint": "Click: play/pause · Scroll or ↑ ↓: volume · F: fullscreen",
"nativeFullscreen": "YouTube fullscreen",
"nativeFullscreenHint": "YouTube fullscreen — the player's own controls (quality, subtitles, its fullscreen button). F fills the screen with our player instead.",
"nativeControls": "YouTube controls active"
}
+2
View File
@@ -36,5 +36,7 @@
"embedDisabledBody": "A tulajdonos letiltotta a lejátszást más oldalakon (gyakori az auto-generált „Topic” zenei sávoknál). A YouTube-on viszont lejátszható.",
"openOnYoutube": "Megnyitás YouTube-on",
"shortcutsHint": "Kattintás: lejátszás/szünet · Görgő vagy ↑ ↓: hangerő · F: teljes képernyő",
"nativeFullscreen": "YouTube teljes képernyő",
"nativeFullscreenHint": "YouTube teljes képernyő — a lejátszó saját vezérlőivel (minőség, felirat, saját teljes képernyő gomb). Az F a mi lejátszónkkal tölti ki a képernyőt.",
"nativeControls": "YouTube vezérlők aktívak"
}
+8 -7
View File
@@ -34,24 +34,25 @@ describe("normalizeVolume", () => {
it("uses the default when the fallback is unusable too", () => {
expect(normalizeVolume(NaN, NaN)).toBe(DEFAULT_VOLUME);
expect(normalizeVolume(null, undefined as unknown as number)).toBe(DEFAULT_VOLUME);
expect(normalizeVolume(null, undefined)).toBe(DEFAULT_VOLUME);
expect(normalizeVolume(null, "80")).toBe(DEFAULT_VOLUME);
});
});
describe("stepVolume", () => {
it("steps up and down from the current level", () => {
expect(stepVolume(60, 5, 100)).toBe(65);
expect(stepVolume(60, -5, 100)).toBe(55);
expect(stepVolume(60, { delta: 5, fallback: 100 })).toBe(65);
expect(stepVolume(60, { delta: -5, fallback: 100 })).toBe(55);
});
it("clamps at both ends instead of wrapping", () => {
expect(stepVolume(98, 5, 100)).toBe(100);
expect(stepVolume(3, -5, 100)).toBe(0);
expect(stepVolume(98, { delta: 5, fallback: 100 })).toBe(100);
expect(stepVolume(3, { delta: -5, fallback: 100 })).toBe(0);
});
it("steps from the fallback when the player answers garbage", () => {
// A spin must not lose notches to a stale/unusable reading — it continues from what we last set.
expect(stepVolume(undefined, -5, 90)).toBe(85);
expect(stepVolume(NaN, -5, 90)).toBe(85);
expect(stepVolume(undefined, { delta: -5, fallback: 90 })).toBe(85);
expect(stepVolume(NaN, { delta: -5, fallback: 90 })).toBe(85);
});
});
+9 -6
View File
@@ -16,15 +16,18 @@ const clamp = (n: number) => Math.max(0, Math.min(100, Math.round(n)));
* being `0` (silently muting the player) is exactly the coercion to avoid. `fallback` is the last
* level known to work; it is itself validated, so a corrupt value can never travel through it.
*/
export function normalizeVolume(raw: unknown, fallback: number = DEFAULT_VOLUME): number {
export function normalizeVolume(raw: unknown, fallback: unknown = DEFAULT_VOLUME): number {
if (typeof raw === "number" && Number.isFinite(raw)) return clamp(raw);
if (typeof fallback === "number" && Number.isFinite(fallback)) return clamp(fallback);
return DEFAULT_VOLUME;
}
/** One volume step (wheel notch / arrow key) from `current`, falling back to `fallback` when the
* player answered something unusable. */
export function stepVolume(current: unknown, delta: number, fallback: number): number {
const base = normalizeVolume(current, fallback);
return clamp(base + delta);
/** One volume step (wheel notch / arrow key) from `current`.
*
* `delta` and `fallback` ride in an options object on purpose: as two adjacent `number`
* parameters they were transposable without a type error, and swapping them silently steps the
* volume BY the current level. */
export function stepVolume(current: unknown, opts: { delta: number; fallback: unknown }): number {
const base = normalizeVolume(current, opts.fallback);
return clamp(base + opts.delta);
}