From 58304757aa89c27460ad7e6908bae0b1cb170f14 Mon Sep 17 00:00:00 2001 From: Kyle Date: Wed, 25 Mar 2026 16:25:57 -0700 Subject: [PATCH] Fix YouTube playback PR review issues --- .../services/overlay-window-bounds.test.ts | 5 ++ .../services/youtube/playback-resolve.test.ts | 9 +++ src/core/services/youtube/playback-resolve.ts | 65 ++++++++++++++++--- src/main.ts | 28 ++++++-- .../runtime/mpv-client-event-bindings.test.ts | 21 ++++++ .../runtime/youtube-playback-launch.test.ts | 26 ++++++++ src/main/runtime/youtube-playback-launch.ts | 27 +++++--- 7 files changed, 158 insertions(+), 23 deletions(-) diff --git a/src/core/services/overlay-window-bounds.test.ts b/src/core/services/overlay-window-bounds.test.ts index 255f832..9e55666 100644 --- a/src/core/services/overlay-window-bounds.test.ts +++ b/src/core/services/overlay-window-bounds.test.ts @@ -7,6 +7,11 @@ test('normalizeOverlayWindowBoundsForPlatform returns original geometry outside assert.deepEqual(normalizeOverlayWindowBoundsForPlatform(geometry, 'linux', null), geometry); }); +test('normalizeOverlayWindowBoundsForPlatform returns original geometry on Windows when screen is unavailable', () => { + const geometry = { x: 150, y: 90, width: 1200, height: 675 }; + assert.deepEqual(normalizeOverlayWindowBoundsForPlatform(geometry, 'win32', null), geometry); +}); + test('normalizeOverlayWindowBoundsForPlatform converts Windows physical pixels to DIP', () => { assert.deepEqual( normalizeOverlayWindowBoundsForPlatform( diff --git a/src/core/services/youtube/playback-resolve.test.ts b/src/core/services/youtube/playback-resolve.test.ts index ae1b1f8..c22616c 100644 --- a/src/core/services/youtube/playback-resolve.test.ts +++ b/src/core/services/youtube/playback-resolve.test.ts @@ -65,3 +65,12 @@ test('resolveYoutubePlaybackUrl rejects when yt-dlp returns no URL', async () => ); }); }); + +test('resolveYoutubePlaybackUrl rejects when yt-dlp output exceeds capture limit', async () => { + await withFakeYtDlp(`${'x'.repeat(1024 * 1024 + 1)}\n`, async () => { + await assert.rejects( + resolveYoutubePlaybackUrl('https://www.youtube.com/watch?v=abc123'), + /exceeded 1048576 bytes/, + ); + }); +}); diff --git a/src/core/services/youtube/playback-resolve.ts b/src/core/services/youtube/playback-resolve.ts index cf88b11..aac26f9 100644 --- a/src/core/services/youtube/playback-resolve.ts +++ b/src/core/services/youtube/playback-resolve.ts @@ -2,6 +2,18 @@ import { spawn } from 'node:child_process'; const YOUTUBE_PLAYBACK_RESOLVE_TIMEOUT_MS = 15_000; const DEFAULT_PLAYBACK_FORMAT = 'b'; +const MAX_CAPTURE_BYTES = 1024 * 1024; + +function terminateCaptureProcess(proc: ReturnType): void { + if (proc.killed) { + return; + } + try { + proc.kill('SIGKILL'); + } catch { + proc.kill(); + } +} function runCapture( command: string, @@ -12,29 +24,62 @@ function runCapture( const proc = spawn(command, args, { stdio: ['ignore', 'pipe', 'pipe'] }); let stdout = ''; let stderr = ''; + let settled = false; + const cleanup = (): void => { + clearTimeout(timer); + proc.stdout.removeAllListeners('data'); + proc.stderr.removeAllListeners('data'); + proc.removeAllListeners('error'); + proc.removeAllListeners('close'); + }; + const rejectOnce = (error: Error): void => { + if (settled) return; + settled = true; + cleanup(); + reject(error); + }; + const resolveOnce = (result: { stdout: string; stderr: string }): void => { + if (settled) return; + settled = true; + cleanup(); + resolve(result); + }; + const appendChunk = ( + current: string, + chunk: unknown, + streamName: 'stdout' | 'stderr', + ): string => { + const next = current + String(chunk); + if (Buffer.byteLength(next, 'utf8') > MAX_CAPTURE_BYTES) { + terminateCaptureProcess(proc); + rejectOnce(new Error(`yt-dlp ${streamName} exceeded ${MAX_CAPTURE_BYTES} bytes`)); + } + return next; + }; const timer = setTimeout(() => { - proc.kill(); - reject(new Error(`yt-dlp timed out after ${timeoutMs}ms`)); + terminateCaptureProcess(proc); + rejectOnce(new Error(`yt-dlp timed out after ${timeoutMs}ms`)); }, timeoutMs); proc.stdout.setEncoding('utf8'); proc.stderr.setEncoding('utf8'); proc.stdout.on('data', (chunk) => { - stdout += String(chunk); + stdout = appendChunk(stdout, chunk, 'stdout'); }); proc.stderr.on('data', (chunk) => { - stderr += String(chunk); + stderr = appendChunk(stderr, chunk, 'stderr'); }); proc.once('error', (error) => { - clearTimeout(timer); - reject(error); + rejectOnce(error); }); proc.once('close', (code) => { - clearTimeout(timer); - if (code === 0) { - resolve({ stdout, stderr }); + if (settled) { return; } - reject(new Error(stderr.trim() || `yt-dlp exited with status ${code ?? 'unknown'}`)); + if (code === 0) { + resolveOnce({ stdout, stderr }); + return; + } + rejectOnce(new Error(stderr.trim() || `yt-dlp exited with status ${code ?? 'unknown'}`)); }); }); } diff --git a/src/main.ts b/src/main.ts index 527ed2a..7626239 100644 --- a/src/main.ts +++ b/src/main.ts @@ -501,6 +501,7 @@ const anilistAttemptedUpdateKeys = new Set(); let anilistCachedAccessToken: string | null = null; let jellyfinPlayQuitOnDisconnectArmed = false; let youtubePlayQuitOnDisconnectArmed = false; +let youtubePlayQuitOnDisconnectArmTimer: ReturnType | null = null; const JELLYFIN_LANG_PREF = 'ja,jp,jpn,japanese,en,eng,english,enUS,en-US'; const JELLYFIN_TICKS_PER_SECOND = 10_000_000; const JELLYFIN_REMOTE_PROGRESS_INTERVAL_MS = 3000; @@ -972,13 +973,23 @@ const waitForYoutubeMpvConnected = createWaitForMpvConnectedHandler({ sleep: (delayMs) => new Promise((resolve) => setTimeout(resolve, delayMs)), }); +function clearYoutubePlayQuitOnDisconnectArmTimer(): void { + if (youtubePlayQuitOnDisconnectArmTimer) { + clearTimeout(youtubePlayQuitOnDisconnectArmTimer); + youtubePlayQuitOnDisconnectArmTimer = null; + } +} + async function runYoutubePlaybackFlowMain(request: { url: string; mode: NonNullable; source: CliCommandSource; }): Promise { youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(true); + let flowCompleted = false; try { + clearYoutubePlayQuitOnDisconnectArmTimer(); + youtubePlayQuitOnDisconnectArmed = false; let playbackUrl = request.url; let launchedWindowsMpv = false; if (process.platform === 'win32') { @@ -1032,20 +1043,27 @@ async function runYoutubePlaybackFlowMain(request: { : 'MPV not connected. Start mpv with the SubMiner profile or retry after mpv finishes starting.', ); } - youtubePlayQuitOnDisconnectArmed = false; - setTimeout(() => { - youtubePlayQuitOnDisconnectArmed = true; - }, 3000); + if (request.source === 'initial') { + youtubePlayQuitOnDisconnectArmTimer = setTimeout(() => { + youtubePlayQuitOnDisconnectArmed = true; + youtubePlayQuitOnDisconnectArmTimer = null; + }, 3000); + } const mediaReady = await prepareYoutubePlaybackInMpv({ url: playbackUrl }); if (!mediaReady) { - logger.warn('Timed out waiting for mpv to load requested YouTube URL; continuing anyway.'); + throw new Error('Timed out waiting for mpv to load the requested YouTube URL.'); } await youtubeFlowRuntime.runYoutubePlaybackFlow({ url: request.url, mode: request.mode, }); + flowCompleted = true; logger.info(`YouTube playback flow completed from ${request.source}.`); } finally { + if (!flowCompleted) { + clearYoutubePlayQuitOnDisconnectArmTimer(); + youtubePlayQuitOnDisconnectArmed = false; + } youtubePrimarySubtitleNotificationRuntime.setAppOwnedFlowInFlight(false); } } diff --git a/src/main/runtime/mpv-client-event-bindings.test.ts b/src/main/runtime/mpv-client-event-bindings.test.ts index 173365f..c695c76 100644 --- a/src/main/runtime/mpv-client-event-bindings.test.ts +++ b/src/main/runtime/mpv-client-event-bindings.test.ts @@ -76,6 +76,27 @@ test('mpv connection handler quits standalone youtube playback even after overla assert.deepEqual(calls, ['presence-refresh', 'report-stop', 'schedule', 'quit']); }); +test('mpv connection handler keeps overlay-initialized non-youtube sessions alive', () => { + const calls: string[] = []; + const handler = createHandleMpvConnectionChangeHandler({ + reportJellyfinRemoteStopped: () => calls.push('report-stop'), + refreshDiscordPresence: () => calls.push('presence-refresh'), + syncOverlayMpvSubtitleSuppression: () => calls.push('sync-overlay-mpv-sub'), + hasInitialPlaybackQuitOnDisconnectArg: () => true, + isOverlayRuntimeInitialized: () => true, + shouldQuitOnDisconnectWhenOverlayRuntimeInitialized: () => false, + isQuitOnDisconnectArmed: () => true, + scheduleQuitCheck: () => { + calls.push('schedule'); + }, + isMpvConnected: () => false, + quitApp: () => calls.push('quit'), + }); + + handler({ connected: false }); + assert.deepEqual(calls, ['presence-refresh', 'report-stop']); +}); + test('mpv subtitle timing handler ignores blank subtitle lines', () => { const calls: string[] = []; const handler = createHandleMpvSubtitleTimingHandler({ diff --git a/src/main/runtime/youtube-playback-launch.test.ts b/src/main/runtime/youtube-playback-launch.test.ts index de54263..b12a469 100644 --- a/src/main/runtime/youtube-playback-launch.test.ts +++ b/src/main/runtime/youtube-playback-launch.test.ts @@ -36,6 +36,32 @@ test('prepare youtube playback treats matching video IDs as already loaded', asy assert.deepEqual(commands, []); }); +test('prepare youtube playback does not mark matching target ready until tracks exist', async () => { + const commands: Array> = []; + let requestCount = 0; + const prepare = createPrepareYoutubePlaybackInMpvHandler({ + requestPath: async () => { + requestCount += 1; + return 'https://www.youtube.com/watch?v=abc123'; + }, + requestProperty: async (name) => { + if (name !== 'track-list') return null; + return requestCount >= 3 ? [{ type: 'video', id: 1 }] : []; + }, + sendMpvCommand: (command) => commands.push(command), + wait: createWaitStub(), + }); + + const ok = await prepare({ + url: 'https://www.youtube.com/watch?v=abc123', + timeoutMs: 1500, + pollIntervalMs: 1, + }); + + assert.equal(ok, true); + assert.deepEqual(commands, []); +}); + test('prepare youtube playback replaces media and waits for path switch', async () => { const commands: Array> = []; const observedPaths = [ diff --git a/src/main/runtime/youtube-playback-launch.ts b/src/main/runtime/youtube-playback-launch.ts index e390601..29514a8 100644 --- a/src/main/runtime/youtube-playback-launch.ts +++ b/src/main/runtime/youtube-playback-launch.ts @@ -95,16 +95,27 @@ export function createPrepareYoutubePlaybackInMpvHandler(deps: YoutubePlaybackLa // Ignore transient path request failures and continue with bootstrap commands. } - if (pathMatchesYoutubeTarget(previousPath, targetUrl)) { - return true; + const alreadyTarget = pathMatchesYoutubeTarget(previousPath, targetUrl); + if (alreadyTarget) { + if (!deps.requestProperty) { + return true; + } + try { + const trackList = await deps.requestProperty('track-list'); + if (hasPlayableMediaTracks(trackList)) { + return true; + } + } catch { + // Keep polling; mpv can report the target path before tracks are ready. + } + } else { + deps.sendMpvCommand(['set_property', 'pause', 'yes']); + deps.sendMpvCommand(['set_property', 'sub-auto', 'no']); + deps.sendMpvCommand(['set_property', 'sid', 'no']); + deps.sendMpvCommand(['set_property', 'secondary-sid', 'no']); + deps.sendMpvCommand(['loadfile', targetUrl, 'replace']); } - deps.sendMpvCommand(['set_property', 'pause', 'yes']); - deps.sendMpvCommand(['set_property', 'sub-auto', 'no']); - deps.sendMpvCommand(['set_property', 'sid', 'no']); - deps.sendMpvCommand(['set_property', 'secondary-sid', 'no']); - deps.sendMpvCommand(['loadfile', targetUrl, 'replace']); - const deadline = now() + timeoutMs; while (now() < deadline) { await deps.wait(pollIntervalMs);