fix(subsync): resolve ffmpeg only when a track needs extraction

Stream subtitles arrive as URLs and are downloaded, never piped through ffmpeg, so demanding an ffmpeg install up front broke stream syncs (and CI, which has no ffmpeg). Resolution is now lazy, scoped to embedded tracks.
This commit is contained in:
2026-08-01 17:35:59 -07:00
parent 4b85ad3eb4
commit 558e00cb44
8 changed files with 134 additions and 59 deletions
@@ -1805,7 +1805,7 @@ describe('stats server API routes', () => {
Stale English subtitle Stale English subtitle
`; `;
fs.writeFileSync(sourcePath, 'fake media'); fs.writeFileSync(sourcePath, 'fake media');
fs.writeFileSync(alassPath, 'fake alass'); fs.writeFileSync(alassPath, 'fake alass', { mode: 0o755 });
fs.writeFileSync( fs.writeFileSync(
japanesePath, japanesePath,
`1 `1
@@ -1865,7 +1865,7 @@ Aligned English subtitle
const englishPath = path.join(dir, 'episode.en.srt'); const englishPath = path.join(dir, 'episode.en.srt');
const alassPath = path.join(dir, 'alass-cli'); const alassPath = path.join(dir, 'alass-cli');
fs.writeFileSync(sourcePath, 'fake media'); fs.writeFileSync(sourcePath, 'fake media');
fs.writeFileSync(alassPath, 'fake alass'); fs.writeFileSync(alassPath, 'fake alass', { mode: 0o755 });
fs.writeFileSync( fs.writeFileSync(
japanesePath, japanesePath,
`1 `1
+5
View File
@@ -7,16 +7,21 @@
*/ */
const BLOCKED_HTTP_HEADER_NAMES = new Set(['authorization', 'cookie', 'proxy-authorization']); const BLOCKED_HTTP_HEADER_NAMES = new Set(['authorization', 'cookie', 'proxy-authorization']);
// Least specific first: every list is merged in order and later entries
// overwrite earlier ones, so the file-local headers win.
const HTTP_HEADER_FIELD_PROPERTY_NAMES = [ const HTTP_HEADER_FIELD_PROPERTY_NAMES = [
'http-header-fields', 'http-header-fields',
'options/http-header-fields', 'options/http-header-fields',
'file-local-options/http-header-fields', 'file-local-options/http-header-fields',
] as const; ] as const;
// Most specific first: these resolve through requestFirstNonEmptyStringProperty,
// which stops at the first match, so the file-local value has to lead.
const USER_AGENT_PROPERTY_NAMES = [ const USER_AGENT_PROPERTY_NAMES = [
'file-local-options/user-agent', 'file-local-options/user-agent',
'options/user-agent', 'options/user-agent',
'user-agent', 'user-agent',
] as const; ] as const;
// Most specific first, for the same first-match reason as the user agent.
const REFERRER_PROPERTY_NAMES = [ const REFERRER_PROPERTY_NAMES = [
'file-local-options/referrer', 'file-local-options/referrer',
'options/referrer', 'options/referrer',
+27 -7
View File
@@ -46,9 +46,16 @@ test('extractSubtitleTrackToFile downloads an external track served over http',
const server = await startSubtitleServer(); const server = await startSubtitleServer();
try { try {
const result = await extractSubtitleTrackToFile({ const result = await extractSubtitleTrackToFile({
ffmpegPath: '/unused/ffmpeg', resolveFfmpegPath: () => {
throw new Error('external tracks must not resolve ffmpeg');
},
videoPath: server.url('/stream.mp4'), videoPath: server.url('/stream.mp4'),
track: { id: 2, type: 'sub', external: true, 'external-filename': server.url('/subs/ja.srt') }, track: {
id: 2,
type: 'sub',
external: true,
'external-filename': server.url('/subs/ja.srt'),
},
httpHeaders: null, httpHeaders: null,
}); });
@@ -72,9 +79,16 @@ test('extractSubtitleTrackToFile forwards mpv request headers to the subtitle ho
try { try {
const result = await extractSubtitleTrackToFile({ const result = await extractSubtitleTrackToFile({
ffmpegPath: '/unused/ffmpeg', resolveFfmpegPath: () => {
throw new Error('external tracks must not resolve ffmpeg');
},
videoPath: server.url('/stream.mp4'), videoPath: server.url('/stream.mp4'),
track: { id: 2, type: 'sub', external: true, 'external-filename': server.url('/subs/ja.srt') }, track: {
id: 2,
type: 'sub',
external: true,
'external-filename': server.url('/subs/ja.srt'),
},
httpHeaders, httpHeaders,
}); });
cleanupTemporaryFile(result); cleanupTemporaryFile(result);
@@ -92,7 +106,9 @@ test('extractSubtitleTrackToFile reports the HTTP status when the download fails
try { try {
await assert.rejects( await assert.rejects(
extractSubtitleTrackToFile({ extractSubtitleTrackToFile({
ffmpegPath: '/unused/ffmpeg', resolveFfmpegPath: () => {
throw new Error('external tracks must not resolve ffmpeg');
},
videoPath: server.url('/stream.mp4'), videoPath: server.url('/stream.mp4'),
track: { track: {
id: 2, id: 2,
@@ -116,7 +132,9 @@ test('extractSubtitleTrackToFile still uses a local external track in place', as
try { try {
const result = await extractSubtitleTrackToFile({ const result = await extractSubtitleTrackToFile({
ffmpegPath: '/unused/ffmpeg', resolveFfmpegPath: () => {
throw new Error('external tracks must not resolve ffmpeg');
},
videoPath: path.join(dir, 'video.mkv'), videoPath: path.join(dir, 'video.mkv'),
track: { id: 2, type: 'sub', external: true, 'external-filename': localPath }, track: { id: 2, type: 'sub', external: true, 'external-filename': localPath },
httpHeaders: null, httpHeaders: null,
@@ -134,7 +152,9 @@ test('extractSubtitleTrackToFile still uses a local external track in place', as
test('extractSubtitleTrackToFile rejects a missing local external track', async () => { test('extractSubtitleTrackToFile rejects a missing local external track', async () => {
await assert.rejects( await assert.rejects(
extractSubtitleTrackToFile({ extractSubtitleTrackToFile({
ffmpegPath: '/unused/ffmpeg', resolveFfmpegPath: () => {
throw new Error('external tracks must not resolve ffmpeg');
},
videoPath: '/tmp/video.mkv', videoPath: '/tmp/video.mkv',
track: { id: 2, type: 'sub', external: true, 'external-filename': '/tmp/does-not-exist.srt' }, track: { id: 2, type: 'sub', external: true, 'external-filename': '/tmp/does-not-exist.srt' },
httpHeaders: null, httpHeaders: null,
+32 -3
View File
@@ -24,7 +24,11 @@ export interface FileExtractionResult {
} }
export interface SubtitleExtractionInput { export interface SubtitleExtractionInput {
ffmpegPath: string; /**
* Resolved lazily: an external track never shells out to ffmpeg, so a stream
* whose subtitles arrive by URL must not fail just because ffmpeg is absent.
*/
resolveFfmpegPath: () => string;
videoPath: string; videoPath: string;
track: MpvTrack; track: MpvTrack;
/** mpv's request context, so stream-hosted tracks stay reachable. */ /** mpv's request context, so stream-hosted tracks stay reachable. */
@@ -41,6 +45,19 @@ function extensionForUrl(url: string, fallback: string): string {
} }
} }
/**
* Drop a scratch directory whose extraction never completed.
*
* Removal is recursive here — unlike `cleanupTemporaryFile`, nothing in a failed
* directory is worth keeping, and a half-written download would otherwise leave
* the directory behind for the life of the machine.
*/
function discardTemporaryDirectory(tempDir: string): void {
try {
fs.rmSync(tempDir, { recursive: true, force: true });
} catch {}
}
/** /**
* Pull a subtitle track mpv loaded from a URL down to disk. * Pull a subtitle track mpv loaded from a URL down to disk.
* *
@@ -53,11 +70,14 @@ async function downloadRemoteSubtitleTrack(
httpHeaders: ResolvedMpvHttpHeaders | null, httpHeaders: ResolvedMpvHttpHeaders | null,
): Promise<FileExtractionResult> { ): Promise<FileExtractionResult> {
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-subsync-')); const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-subsync-'));
try {
const outputPath = path.join(tempDir, `remote_track.${extensionForUrl(url, 'srt')}`); const outputPath = path.join(tempDir, `remote_track.${extensionForUrl(url, 'srt')}`);
const result = await downloadToFile(url, outputPath, toRequestHeaders(httpHeaders)); const result = await downloadToFile(url, outputPath, toRequestHeaders(httpHeaders));
if (!result.ok) { if (!result.ok) {
throw new Error(`Failed to download subtitle track: ${result.error?.error ?? 'unknown error'}`); throw new Error(
`Failed to download subtitle track: ${result.error?.error ?? 'unknown error'}`,
);
} }
if (!fileExists(outputPath)) { if (!fileExists(outputPath)) {
throw new Error(`Downloaded subtitle track is missing: ${url}`); throw new Error(`Downloaded subtitle track is missing: ${url}`);
@@ -65,6 +85,10 @@ async function downloadRemoteSubtitleTrack(
logger.info(`Downloaded remote subtitle track to ${outputPath}`); logger.info(`Downloaded remote subtitle track to ${outputPath}`);
return { path: outputPath, temporary: true }; return { path: outputPath, temporary: true };
} catch (error) {
discardTemporaryDirectory(tempDir);
throw error;
}
} }
async function extractInternalTrack(input: SubtitleExtractionInput): Promise<FileExtractionResult> { async function extractInternalTrack(input: SubtitleExtractionInput): Promise<FileExtractionResult> {
@@ -78,13 +102,14 @@ async function extractInternalTrack(input: SubtitleExtractionInput): Promise<Fil
} }
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-subsync-')); const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-subsync-'));
try {
const outputPath = path.join(tempDir, `track_${ffIndex}.${extension}`); const outputPath = path.join(tempDir, `track_${ffIndex}.${extension}`);
// Header args configure the HTTP demuxer, so they belong before `-i`. // Header args configure the HTTP demuxer, so they belong before `-i`.
const httpArgs = isRemoteMediaPath(input.videoPath) const httpArgs = isRemoteMediaPath(input.videoPath)
? toFfmpegInputHttpArgs(input.httpHeaders) ? toFfmpegInputHttpArgs(input.httpHeaders)
: []; : [];
const extraction = await runCommand(input.ffmpegPath, [ const extraction = await runCommand(input.resolveFfmpegPath(), [
'-hide_banner', '-hide_banner',
'-nostdin', '-nostdin',
'-y', '-y',
@@ -112,6 +137,10 @@ async function extractInternalTrack(input: SubtitleExtractionInput): Promise<Fil
} }
return { path: outputPath, temporary: true }; return { path: outputPath, temporary: true };
} catch (error) {
discardTemporaryDirectory(tempDir);
throw error;
}
} }
export async function extractSubtitleTrackToFile( export async function extractSubtitleTrackToFile(
+4 -1
View File
@@ -94,7 +94,10 @@ test('runSubsyncManual syncs stream subtitle tracks served over http', async (t)
getResolvedConfig: () => ({ getResolvedConfig: () => ({
alassPath, alassPath,
ffsubsyncPath: '', ffsubsyncPath: '',
ffmpegPath: '', // Points nowhere on purpose: both tracks are external, so resolving ffmpeg
// at all would throw. An empty path would just auto-discover the real
// ffmpeg on a dev machine and prove nothing.
ffmpegPath: path.join(tmpDir, 'no-such-ffmpeg'),
replace: true, replace: true,
}), }),
}; };
+2 -4
View File
@@ -212,9 +212,8 @@ async function subsyncToReference(
client: MpvClientLike, client: MpvClientLike,
httpHeaders: ResolvedMpvHttpHeaders | null, httpHeaders: ResolvedMpvHttpHeaders | null,
): Promise<SubsyncResult> { ): Promise<SubsyncResult> {
const ffmpegPath = resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg');
const primaryExtraction = await extractSubtitleTrackToFile({ const primaryExtraction = await extractSubtitleTrackToFile({
ffmpegPath, resolveFfmpegPath: () => resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg'),
videoPath: context.videoPath, videoPath: context.videoPath,
track: context.primaryTrack, track: context.primaryTrack,
httpHeaders, httpHeaders,
@@ -311,11 +310,10 @@ export async function runSubsyncManual(
return { ok: false, message: 'Select a subtitle source track for alass' }; return { ok: false, message: 'Select a subtitle source track for alass' };
} }
const ffmpegPath = resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg');
let sourceExtraction: FileExtractionResult | null = null; let sourceExtraction: FileExtractionResult | null = null;
try { try {
sourceExtraction = await extractSubtitleTrackToFile({ sourceExtraction = await extractSubtitleTrackToFile({
ffmpegPath, resolveFfmpegPath: () => resolveSubsyncExecutable(resolved.ffmpegPath, 'ffmpeg'),
videoPath: context.videoPath, videoPath: context.videoPath,
track: sourceTrack, track: sourceTrack,
httpHeaders, httpHeaders,
+9 -1
View File
@@ -28,9 +28,17 @@ function unique(values: string[]): string[] {
return values.filter((value, index) => value.length > 0 && values.indexOf(value) === index); return values.filter((value, index) => value.length > 0 && values.indexOf(value) === index);
} }
/**
* A same-named non-executable file earlier on PATH would otherwise shadow the
* real binary and surface as a spawn EACCES rather than "keep looking". Windows
* has no execute bit, so the regular-file check is all it can offer there.
*/
export function isExecutableFile(filePath: string): boolean { export function isExecutableFile(filePath: string): boolean {
try { try {
return fs.statSync(filePath).isFile(); if (!fs.statSync(filePath).isFile()) return false;
if (process.platform === 'win32') return true;
fs.accessSync(filePath, fs.constants.X_OK);
return true;
} catch { } catch {
return false; return false;
} }
+14 -2
View File
@@ -92,11 +92,23 @@ export function getSubsyncConfig(config: SubsyncConfig | undefined): SubsyncReso
}; };
} }
/**
* ffsubsync streams a progress bar to stderr, so an unbounded summary reaches
* the OSD (and the error log) as tens of kilobytes of carriage returns. The tail
* is the part that says what went wrong.
*/
const COMMAND_OUTPUT_SUMMARY_LIMIT = 2000;
function tailForSummary(value: string): string {
if (value.length <= COMMAND_OUTPUT_SUMMARY_LIMIT) return value;
return `${value.slice(-COMMAND_OUTPUT_SUMMARY_LIMIT)}`;
}
export function summarizeCommandFailure(command: string, result: CommandResult): string { export function summarizeCommandFailure(command: string, result: CommandResult): string {
const parts = [ const parts = [
`code=${result.code ?? 'n/a'}`, `code=${result.code ?? 'n/a'}`,
result.stderr ? `stderr: ${result.stderr}` : '', result.stderr ? `stderr: ${tailForSummary(result.stderr)}` : '',
result.stdout ? `stdout: ${result.stdout}` : '', result.stdout ? `stdout: ${tailForSummary(result.stdout)}` : '',
result.error ? `error: ${result.error}` : '', result.error ? `error: ${result.error}` : '',
] ]
.map((value) => value.trim()) .map((value) => value.trim())