Skip to content

Commit 79d78e5

Browse files
committed
fix(media): let a host with only ffprobe still probe
ensureFfmpeg() conflated 'resolve the binary' with 'require the binary', and resolveFfprobePath called it first — so on a host with FFPROBE_PATH set but no ffmpeg, the first probe succeeded and every later one threw, because the failed lookup is memoized. Split the non-throwing init from the ffmpeg-required assertion; transcoding still demands ffmpeg, probing no longer does. Covered by a test in its own file, since the binary lookup memoizes at module scope and a shared file would already have consumed that state.
1 parent acc3c6d commit 79d78e5

2 files changed

Lines changed: 87 additions & 18 deletions

File tree

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/**
2+
* Standalone file: the ffmpeg module memoizes its binary lookup at module
3+
* scope, so exercising the "no ffmpeg installed" branch needs a fresh module
4+
* state that a shared file would already have consumed.
5+
*
6+
* @vitest-environment node
7+
*/
8+
import { beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
const { execSyncMock, execFileMock } = vi.hoisted(() => ({
11+
execSyncMock: vi.fn(),
12+
execFileMock: vi.fn(),
13+
}))
14+
15+
vi.mock('node:child_process', () => ({
16+
execSync: execSyncMock,
17+
execFile: execFileMock,
18+
}))
19+
20+
import { runFfmpegOperation } from '@/lib/media/ffmpeg'
21+
22+
const PROBE_JSON = JSON.stringify({
23+
format: { duration: '3', format_name: 'mov,mp4' },
24+
streams: [{ codec_type: 'video', codec_name: 'h264', width: 640, height: 480 }],
25+
})
26+
27+
describe('probing without a discoverable ffmpeg binary', () => {
28+
beforeEach(() => {
29+
vi.clearAllMocks()
30+
// No ffmpeg on this host.
31+
execSyncMock.mockImplementation(() => {
32+
throw new Error('which: no ffmpeg in PATH')
33+
})
34+
execFileMock.mockImplementation((_bin, _args, _opts, cb) => {
35+
cb(null, PROBE_JSON, '')
36+
return {}
37+
})
38+
})
39+
40+
it('keeps probing across repeated calls', async () => {
41+
const file = { buffer: Buffer.from('media'), mimeType: 'video/mp4' }
42+
43+
// The second call is the regression: the binary lookup is memoized after
44+
// the first, and an ffmpeg-required check here would throw from then on
45+
// even though ffprobe is perfectly usable.
46+
for (const _ of [1, 2, 3]) {
47+
const result = await runFfmpegOperation('probe', [file])
48+
expect(result.probe).toMatchObject({ hasVideo: true, width: 640, height: 480 })
49+
}
50+
51+
expect(execFileMock).toHaveBeenCalledTimes(3)
52+
expect(execFileMock.mock.calls[0][0]).toContain('ffprobe')
53+
})
54+
55+
it('still refuses to transcode, which genuinely needs ffmpeg', async () => {
56+
await expect(
57+
runFfmpegOperation('convert', [{ buffer: Buffer.from('m'), mimeType: 'video/mp4' }], {
58+
format: 'mp3',
59+
})
60+
).rejects.toThrow('FFmpeg not found')
61+
})
62+
})

apps/sim/lib/media/ffmpeg.ts

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,9 @@ let ffmpegInitialized = false
1818
let ffmpegPath: string | null = null
1919
let ffprobePath: string | null = null
2020

21-
/** Lazy system FFmpeg binary resolution, mirroring lib/audio/extractor.ts. */
22-
function ensureFfmpeg(): void {
23-
if (ffmpegInitialized) {
24-
if (!ffmpegPath) {
25-
throw new Error(
26-
'FFmpeg not found. Install: brew install ffmpeg (macOS) / apk add ffmpeg (Alpine) / apt-get install ffmpeg (Ubuntu)'
27-
)
28-
}
29-
return
30-
}
21+
/** Lazy system FFmpeg binary resolution, mirroring lib/audio/extractor.ts. Never throws. */
22+
function initFfmpegPath(): void {
23+
if (ffmpegInitialized) return
3124
ffmpegInitialized = true
3225

3326
try {
@@ -39,25 +32,39 @@ function ensureFfmpeg(): void {
3932
}
4033
}
4134

35+
/**
36+
* Transcoding requires ffmpeg itself. Probing does not — kept separate from
37+
* {@link initFfmpegPath} so a host with only ffprobe can still probe.
38+
*/
39+
function ensureFfmpeg(): void {
40+
initFfmpegPath()
41+
if (!ffmpegPath) {
42+
throw new Error(
43+
'FFmpeg not found. Install: brew install ffmpeg (macOS) / apk add ffmpeg (Alpine) / apt-get install ffmpeg (Ubuntu)'
44+
)
45+
}
46+
}
47+
4248
/**
4349
* Mirrors fluent-ffmpeg's resolution order (FFPROBE_PATH, then PATH, then
4450
* ffmpeg's own directory) so replacing its ffprobe call does not narrow where
4551
* the binary may live for self-hosters.
4652
*/
4753
function resolveFfprobePath(): string {
48-
ensureFfmpeg()
4954
if (ffprobePath) return ffprobePath
5055

5156
const binary = process.platform === 'win32' ? 'ffprobe.exe' : 'ffprobe'
5257
const configured = process.env.FFPROBE_PATH?.trim()
53-
const sibling = ffmpegPath ? path.join(path.dirname(ffmpegPath), binary) : undefined
58+
if (configured && existsSync(configured)) {
59+
ffprobePath = configured
60+
return ffprobePath
61+
}
5462

55-
ffprobePath =
56-
configured && existsSync(configured)
57-
? configured
58-
: sibling && existsSync(sibling)
59-
? sibling
60-
: binary
63+
// Deliberately initFfmpegPath, not ensureFfmpeg: a missing ffmpeg must not
64+
// stop a probe, since ffprobe may still be on PATH.
65+
initFfmpegPath()
66+
const sibling = ffmpegPath ? path.join(path.dirname(ffmpegPath), binary) : undefined
67+
ffprobePath = sibling && existsSync(sibling) ? sibling : binary
6168
return ffprobePath
6269
}
6370

0 commit comments

Comments
 (0)