Skip to content

Commit dc6cc8b

Browse files
committed
fix(media): floor the probe timeout and label audio-only webm correctly
Two findings from review: - Node reads execFile's `timeout: 0` as 'no timeout', so once the shared budget was spent the 15s probe cap disappeared entirely — the opposite of what an exhausted budget should do. Reachable between the deadline passing and the abort timer firing, where assertOperationLive still sees a live signal. Floor the computed cap at 1ms. - extract_audio accepts webm, but mimeFromExt resolves that container to video/webm, so an audio-only extract was stored with a video content type. Resolve audio-only outputs through a small override map.
1 parent 6c7a25e commit dc6cc8b

3 files changed

Lines changed: 40 additions & 4 deletions

File tree

apps/sim/lib/media/ffmpeg-probe-resolution.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,17 @@ describe('probing without a discoverable ffmpeg binary', () => {
5252
expect(execFileMock.mock.calls[0][0]).toContain('ffprobe')
5353
})
5454

55+
it('always hands ffprobe a positive timeout', async () => {
56+
// Node reads `timeout: 0` as "no timeout", so the computed cap is floored.
57+
// Asserted on a healthy budget rather than an expired one: forcing the
58+
// expired window means racing the abort timer, which makes the test flaky.
59+
await runFfmpegOperation('probe', [{ buffer: Buffer.from('media'), mimeType: 'video/mp4' }])
60+
61+
const opts = execFileMock.mock.calls[0][2] as { timeout: number }
62+
expect(opts.timeout).toBeGreaterThan(0)
63+
expect(opts.timeout).toBeLessThanOrEqual(15_000)
64+
})
65+
5566
it('still refuses to transcode, which genuinely needs ffmpeg', async () => {
5667
await expect(
5768
runFfmpegOperation('convert', [{ buffer: Buffer.from('m'), mimeType: 'video/mp4' }], {

apps/sim/lib/media/ffmpeg.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,15 @@ describe('runFfmpegOperation per-operation validation', () => {
104104
)
105105
})
106106

107+
it('allows webm for extract_audio but not weba', async () => {
108+
const error = await runFfmpegOperation('extract_audio', [mediaFile()], {
109+
format: 'weba',
110+
}).catch((e: Error) => e)
111+
112+
expect(error.message).toContain('Unsupported output format')
113+
expect(error.message).toContain('webm')
114+
})
115+
107116
it('restricts extract_audio to audio containers', async () => {
108117
await expect(
109118
runFfmpegOperation('extract_audio', [mediaFile()], { format: 'png' })

apps/sim/lib/media/ffmpeg.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,11 @@ const OUTPUT_EXTS = new Set([
246246
*/
247247
const AUDIO_EXTS = new Set(['mp3', 'm4a', 'wav', 'ogg', 'flac', 'aac', 'opus', 'webm'])
248248

249+
/** Containers shared with video, whose content type differs for an audio-only output. */
250+
const AUDIO_ONLY_MIME: Record<string, string> = {
251+
webm: 'audio/webm',
252+
}
253+
249254
/**
250255
* Temp-file names are built as `${prefix}.${ext}` and joined against the temp
251256
* dir, so an extension carrying `/` or `..` escapes that dir once `path.join`
@@ -493,12 +498,17 @@ function describeProbeFailure(err: Error & { killed?: boolean; code?: unknown },
493498
function probeFile(filePath: string, limit: TimeoutAbortController): Promise<MediaProbe> {
494499
assertOperationLive(limit)
495500
const remaining = getRemainingExecutionMs(limit.signal) ?? PROBE_TIMEOUT_MS
501+
// Floored at 1ms: Node reads `timeout: 0` as "no timeout", so an expired
502+
// budget would otherwise remove the probe cap entirely — the opposite of what
503+
// an exhausted budget should do. Reachable between the deadline passing and
504+
// the abort timer firing, where assertOperationLive still sees a live signal.
505+
const timeout = Math.max(1, Math.min(PROBE_TIMEOUT_MS, remaining))
496506
return new Promise((resolve, reject) => {
497507
execFile(
498508
resolveFfprobePath(),
499509
['-v', 'error', '-print_format', 'json', '-show_format', '-show_streams', '-i', filePath],
500510
{
501-
timeout: Math.min(PROBE_TIMEOUT_MS, remaining),
511+
timeout,
502512
killSignal: 'SIGKILL',
503513
maxBuffer: PROBE_MAX_OUTPUT_BYTES,
504514
signal: limit.signal,
@@ -597,9 +607,13 @@ export async function runFfmpegOperation(
597607
}
598608
}
599609

600-
async function readOut(outputPath: string, ext: string): Promise<FfmpegResult> {
610+
async function readOut(
611+
outputPath: string,
612+
ext: string,
613+
contentType = mimeFromExt(ext)
614+
): Promise<FfmpegResult> {
601615
const buffer = await fs.readFile(outputPath)
602-
return { buffer, ext, contentType: mimeFromExt(ext) }
616+
return { buffer, ext, contentType }
603617
}
604618

605619
async function overlayAudio(
@@ -863,7 +877,9 @@ async function extractAudio(
863877
const outputPath = tempPath(dir, `out.${ext}`)
864878
const command = ffmpeg(inputPath).noVideo()
865879
await runCommand(command, outputPath, limit)
866-
return readOut(outputPath, ext)
880+
// A container shared with video (webm) resolves to a video content type by
881+
// default, but this output has had its video stream dropped.
882+
return readOut(outputPath, ext, AUDIO_ONLY_MIME[ext] ?? mimeFromExt(ext))
867883
}
868884

869885
async function convert(

0 commit comments

Comments
 (0)