fix(video): try metadata extraction and thumbnail generation independently

processUploadedVideo() gated everything behind isValidVideo(), which
rejects the whole video if ffprobe can't read even one of
duration/width/height -- common on some iPhone/Lightroom-exported
MP4s (issue 1370). Callers (photoProcessor.js's processPhoto and
processUploadedPhotos) already catch that throw and fall back to a
static placeholder thumbnail plus a metadata-only retry (codex
review of #845), but that fallback never got a REAL thumbnail even
when generateVideoThumbnail() would have succeeded on its own --
thumbnailing doesn't need valid duration/width/height, it just seeks
and grabs a frame.

processUploadedVideo now tries metadata extraction and thumbnail
generation independently, keeping whichever succeeds instead of
discarding both on a single failed field. The callers' existing
throw handling stays as a backstop.

Also: extractVideoMetadata stored duration as 0 (not null) whenever
ffprobe had no duration field, masking "unknown" as a fake real
zero-second clip and defeating downstream `duration != null` checks
meant to skip an untrustworthy value.

Relates to issue 1370
This commit is contained in:
Paul Nothaft
2026-09-10 13:04:00 +02:00
parent 443ec91de9
commit 30c3134891
2 changed files with 156 additions and 23 deletions
@@ -0,0 +1,110 @@
jest.mock('../../utils/logger');
jest.mock('fluent-ffmpeg');
jest.mock('../storage', () => ({
getStorage: jest.fn()
}));
const ffmpeg = require('fluent-ffmpeg');
const { getStorage } = require('../storage');
const {
extractVideoMetadata,
processUploadedVideo
} = require('../videoProcessor');
describe('extractVideoMetadata (#1370)', () => {
afterEach(() => jest.clearAllMocks());
it('returns null duration rather than 0 when ffprobe has none, so "unknown" and "a real 0s clip" stay distinguishable', async () => {
ffmpeg.ffprobe = jest.fn((videoPath, cb) => {
cb(null, {
streams: [{ codec_type: 'video', width: 1920, height: 1080, codec_name: 'hevc' }],
format: {} // no duration field at all
});
});
const metadata = await extractVideoMetadata('/tmp/video.mp4');
expect(metadata.duration).toBeNull();
expect(metadata.width).toBe(1920);
expect(metadata.videoCodec).toBe('hevc');
});
it('floors a real duration', async () => {
ffmpeg.ffprobe = jest.fn((videoPath, cb) => {
cb(null, { streams: [], format: { duration: 12.9 } });
});
const metadata = await extractVideoMetadata('/tmp/video.mp4');
expect(metadata.duration).toBe(12);
});
});
describe('processUploadedVideo degrades gracefully instead of rejecting the whole video (#1370)', () => {
let storage;
beforeEach(() => {
storage = { putFromFile: jest.fn().mockResolvedValue(undefined), exists: jest.fn().mockResolvedValue(true) };
getStorage.mockReturnValue(storage);
});
afterEach(() => jest.clearAllMocks());
it('keeps the thumbnail when only metadata extraction fails', async () => {
ffmpeg.ffprobe = jest.fn((videoPath, cb) => cb(new Error('moov atom not found')));
ffmpeg.mockImplementation(() => ({
screenshots: jest.fn(function screenshots({ filename, folder }) {
require('fs').writeFileSync(require('path').join(folder, filename), 'jpeg-bytes');
return this;
}),
on(event, handler) {
if (event === 'end') setImmediate(handler);
return this;
}
}));
const result = await processUploadedVideo('/tmp/video.mp4', 'thumbnails/thumb_video.jpg');
expect(result.success).toBe(true);
expect(result.metadata).toBeNull();
expect(result.thumbnailKey).toBe('thumbnails/thumb_video.jpg');
});
it('keeps the metadata when only thumbnail generation fails', async () => {
ffmpeg.ffprobe = jest.fn((videoPath, cb) => {
cb(null, {
streams: [{ codec_type: 'video', width: 1080, height: 1920, codec_name: 'h264' }],
format: { duration: 5.4 }
});
});
ffmpeg.mockImplementation(() => ({
screenshots() { return this; },
on(event, handler) {
if (event === 'error') setImmediate(() => handler(new Error('ffmpeg seek failed')));
return this;
}
}));
const result = await processUploadedVideo('/tmp/video.mp4', 'thumbnails/thumb_video.jpg');
expect(result.success).toBe(true);
expect(result.metadata).toEqual(expect.objectContaining({ duration: 5, videoCodec: 'h264' }));
expect(result.thumbnailKey).toBeNull();
expect(storage.putFromFile).not.toHaveBeenCalled();
});
it('still succeeds with both null when metadata AND thumbnail fail — never throws, never blocks the upload', async () => {
ffmpeg.ffprobe = jest.fn((videoPath, cb) => cb(new Error('Invalid data found when processing input')));
ffmpeg.mockImplementation(() => ({
screenshots() { return this; },
on(event, handler) {
if (event === 'error') setImmediate(() => handler(new Error('ffmpeg seek failed')));
return this;
}
}));
const result = await processUploadedVideo('/tmp/corrupt.mp4', 'thumbnails/thumb_corrupt.jpg');
expect(result).toEqual({ success: true, metadata: null, thumbnailKey: null });
});
});
+46 -23
View File
@@ -32,7 +32,10 @@ async function extractVideoMetadata(videoPath) {
const audioStream = metadata.streams.find(s => s.codec_type === 'audio');
const result = {
duration: Math.floor(metadata.format.duration || 0),
// null (not 0) when ffprobe genuinely has no duration — a real
// 0-second clip and "unknown" must stay distinguishable, since
// downstream code treats `duration != null` as "trust this value".
duration: metadata.format.duration != null ? Math.floor(metadata.format.duration) : null,
width: videoStream?.width || null,
height: videoStream?.height || null,
videoCodec: videoStream?.codec_name || null,
@@ -130,35 +133,55 @@ async function getVideoDuration(videoPath) {
* Process an uploaded video: extract metadata and produce a thumbnail through
* the storage backend.
*
* Metadata extraction and thumbnail generation are independent, best-effort
* steps — mirroring how the image pipeline treats thumbnail/dimension/EXIF
* failures (log a warning, keep the upload). This used to gate everything
* behind isValidVideo(), which rejects the whole video if ffprobe can't read
* even one of duration/width/height — common on some iPhone/Lightroom-
* exported MP4s (#1370). Callers (photoProcessor.js's processPhoto and
* processUploadedPhotos) already catch that throw and fall back to a static
* placeholder thumbnail plus a metadata-only retry (codex review of #845),
* but that fallback never got a REAL thumbnail even when
* generateVideoThumbnail() would have succeeded on its own — thumbnailing
* doesn't need valid duration/width/height, it just seeks and grabs a frame.
* Trying both steps independently means a real thumbnail (and whatever
* metadata ffprobe *can* read) survives far more often; the callers' throw
* handling stays as a backstop for anything still unexpected.
*
* @param {string} videoPath - Local path to the source video (ffmpeg requires fs).
* @param {string} thumbnailKey - Relative storage key for the thumbnail.
* @returns {Promise<{success: boolean, metadata: Object, thumbnailKey: string}>}
* @returns {Promise<{success: boolean, metadata: Object|null, thumbnailKey: string|null}>}
*/
async function processUploadedVideo(videoPath, thumbnailKey, options = {}) {
let metadata = null;
try {
const isValid = await isValidVideo(videoPath);
if (!isValid) {
throw new Error('Invalid video file');
}
const metadata = await extractVideoMetadata(videoPath);
await generateVideoThumbnail(videoPath, thumbnailKey, options);
const storage = getStorage();
const exists = await storage.exists(thumbnailKey);
if (!exists) {
throw new Error('Thumbnail generation failed (not in storage)');
}
return {
success: true,
metadata,
thumbnailKey
};
metadata = await extractVideoMetadata(videoPath);
} catch (error) {
logger.error('Error processing video', { error: error.message, videoPath });
throw error;
logger.error('Video metadata extraction failed — continuing without duration/codec/dimensions', {
error: error.message,
videoPath
});
}
let generatedThumbnailKey = null;
try {
await generateVideoThumbnail(videoPath, thumbnailKey, options);
const storage = getStorage();
if (await storage.exists(thumbnailKey)) {
generatedThumbnailKey = thumbnailKey;
}
} catch (error) {
logger.error('Video thumbnail generation failed — continuing without a thumbnail', {
error: error.message,
videoPath
});
}
return {
success: true,
metadata,
thumbnailKey: generatedThumbnailKey
};
}
/**