From 30c3134891c3098c086c9390fd2997956da3de34 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 10 Sep 2026 13:04:00 +0200 Subject: [PATCH] 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 --- .../services/__tests__/videoProcessor.test.js | 110 ++++++++++++++++++ backend/src/services/videoProcessor.js | 69 +++++++---- 2 files changed, 156 insertions(+), 23 deletions(-) create mode 100644 backend/src/services/__tests__/videoProcessor.test.js diff --git a/backend/src/services/__tests__/videoProcessor.test.js b/backend/src/services/__tests__/videoProcessor.test.js new file mode 100644 index 00000000..2b34eaa8 --- /dev/null +++ b/backend/src/services/__tests__/videoProcessor.test.js @@ -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 }); + }); +}); diff --git a/backend/src/services/videoProcessor.js b/backend/src/services/videoProcessor.js index f699d284..b94268d5 100644 --- a/backend/src/services/videoProcessor.js +++ b/backend/src/services/videoProcessor.js @@ -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 + }; } /**