From 0310c46fdd5b03274f761abfb4c8b552e2f8b666 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:00:08 +0200 Subject: [PATCH] fix(uploads): keep videos when thumbnail generation fails (#845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(uploads): keep videos when thumbnail generation fails processUploadedVideo() (ffmpeg probe + thumbnail) was unguarded in both pipeline paths, while the image branch next to each already survives its thumbnail failures: - processUploadedPhotos (sync): the throw failed the whole upload — the video was lost. - processPhoto (async worker, the path real uploads take): the throw marked the row 'failed', and the guest gallery only lists 'complete' — the video became permanently invisible despite being fully uploaded. Both call sites now fall back to extractVideoMetadata() alone and keep the video without a preview; if even the probe fails, the video is kept with no metadata. Idea from the munin92 fork (2026-07-02), reimplemented for both paths + regression test. * fix(uploads): placeholder thumbnail for rescued videos (codex review of #845) A completed video with a NULL thumbnail made the gallery grid fetch the ORIGINAL video file as an blob (thumbnail_url || url) — a potentially multi-GB download for a broken tile. Both fallback paths now generate the existing sharp-rendered play-button placeholder (generateVideoPlaceholder — ffmpeg-free), so rescued videos get a real tile. Test asserts the placeholder key lands in thumbnail_path. --- .../photoProcessor.processPhoto.test.js | 40 +++++++++++ backend/src/services/photoProcessor.js | 66 +++++++++++++++---- 2 files changed, 93 insertions(+), 13 deletions(-) diff --git a/backend/__tests__/services/photoProcessor.processPhoto.test.js b/backend/__tests__/services/photoProcessor.processPhoto.test.js index d5935300..2af9d2f4 100644 --- a/backend/__tests__/services/photoProcessor.processPhoto.test.js +++ b/backend/__tests__/services/photoProcessor.processPhoto.test.js @@ -71,6 +71,7 @@ jest.mock('../../src/services/imageProcessor', () => { const mockExtractCaptureDate = jest.fn(); return { generateThumbnail: mockGenerateThumbnail, + generateVideoPlaceholder: jest.fn(async (filename) => `thumbnails/thumb_${filename.replace(/\.[^.]+$/, '')}.jpg`), extractCaptureDate: mockExtractCaptureDate, withLocalCopy: jest.fn(async (key, fn) => fn(`/tmp/local-copy-${require('path').basename(key)}`) @@ -87,6 +88,7 @@ jest.mock('../../src/services/imageProcessor', () => { jest.mock('../../src/services/videoProcessor', () => ({ processUploadedVideo: jest.fn(), + extractVideoMetadata: jest.fn(), isVideoMimeType: (mime) => typeof mime === 'string' && mime.startsWith('video/'), })); @@ -212,6 +214,44 @@ describe('photoProcessor.processPhoto', () => { expect(watermarkService.generateForPhoto).not.toHaveBeenCalled(); }); + it('keeps a video complete with a placeholder thumbnail when ffmpeg fails', async () => { + dbModule.__setPhoto({ + id: 203, + event_id: 9, + filename: 'drone-clip.mp4', + original_filename: 'drone.mp4', + mime_type: 'video/mp4', + media_type: 'video', + size_bytes: 12345, + captured_at: null, + }); + dbModule.__setEvent({ id: 9, slug: 'wedding', event_name: 'Wedding' }); + + // ffmpeg thumbnail pipeline throws (e.g. unsupported pixel format)… + videoProcessor.processUploadedVideo.mockRejectedValueOnce(new Error('ffmpeg exited with code 1')); + // …but a plain probe still works. + videoProcessor.extractVideoMetadata.mockResolvedValueOnce({ + duration: 42, + videoCodec: 'hevc', + audioCodec: 'aac', + width: 3840, + height: 2160, + }); + + const { processPhoto } = require('../../src/services/photoProcessor'); + await processPhoto(203); + + const finalUpdate = dbModule.__recorded().updateCalls.pop(); + // The row must complete — 'failed' rows are invisible to guests. + expect(finalUpdate.data.processing_status).toBe('complete'); + // Placeholder instead of NULL: a completed video without thumbnail would + // make the grid fetch the original video file for the tile (#845 review). + expect(finalUpdate.data.thumbnail_path).toBe('thumbnails/thumb_drone-clip.jpg'); + expect(imageProcessor.generateVideoPlaceholder).toHaveBeenCalledWith('drone-clip.mp4'); + expect(finalUpdate.data.duration).toBe(42); + expect(finalUpdate.data.video_codec).toBe('hevc'); + }); + it('throws when the photo row no longer exists', async () => { dbModule.__setPhoto(null); dbModule.__setEvent({ id: 1 }); diff --git a/backend/src/services/photoProcessor.js b/backend/src/services/photoProcessor.js index 5820cb1f..7d1da6a7 100644 --- a/backend/src/services/photoProcessor.js +++ b/backend/src/services/photoProcessor.js @@ -1,9 +1,9 @@ const path = require('path'); const fs = require('fs').promises; const { db } = require('../database/db'); -const { generateThumbnail, extractCaptureDate, withLocalCopy, withProcessableImage } = require('./imageProcessor'); +const { generateThumbnail, generateVideoPlaceholder, extractCaptureDate, withLocalCopy, withProcessableImage } = require('./imageProcessor'); const { generatePhotoFilename } = require('../utils/filenameSanitizer'); -const { processUploadedVideo, isVideoMimeType } = require('./videoProcessor'); +const { processUploadedVideo, extractVideoMetadata, isVideoMimeType } = require('./videoProcessor'); const { getStorage } = require('./storage'); const { resolvePhotoStorageKey } = require('./photoResolver'); const logger = require('../utils/logger'); @@ -141,9 +141,27 @@ async function processUploadedPhotos(files, eventId, uploadedBy = 'admin', categ 'thumbnails', `thumb_${newFilename.replace(/\.[^.]+$/, '.jpg')}` ); - const result = await processUploadedVideo(tempPath, videoThumbnailKey); - videoMetadata = result.metadata; - thumbnailPath = result.thumbnailKey; + // A thumbnail/probe failure must not lose the video: without this + // guard the whole upload errors here, while the image branch below + // already survives its thumbnail failures. Fall back to metadata-only + // plus the static play-button placeholder — a completed video with a + // NULL thumbnail would make the grid fetch the ORIGINAL video file + // as an blob (thumbnail_url || url), i.e. a multi-GB download + // for a broken tile (codex review of #845). + try { + const result = await processUploadedVideo(tempPath, videoThumbnailKey); + videoMetadata = result.metadata; + thumbnailPath = result.thumbnailKey; + } catch (videoErr) { + logger.warn(`Video processing failed for ${file.originalname}, using placeholder thumbnail:`, videoErr.message); + try { + videoMetadata = await extractVideoMetadata(tempPath); + } catch (metaErr) { + logger.warn(`Video metadata extraction also failed for ${file.originalname}:`, metaErr.message); + } + // ffmpeg-free (sharp-rendered SVG); returns null on failure. + thumbnailPath = await generateVideoPlaceholder(newFilename); + } } else { // RAW/DNG can't be fed to sharp directly (no raw loader), so extract the // embedded JPEG preview first and thumbnail/measure THAT. Pass-through @@ -457,14 +475,36 @@ async function processPhoto(photoId) { 'thumbnails', `thumb_${photo.filename.replace(/\.[^.]+$/, '.jpg')}` ); - const result = await processUploadedVideo(localPath, videoThumbnailKey); - updateData.thumbnail_path = result.thumbnailKey; - if (result.metadata) { - if (result.metadata.duration != null) updateData.duration = result.metadata.duration; - if (result.metadata.videoCodec) updateData.video_codec = result.metadata.videoCodec; - if (result.metadata.audioCodec) updateData.audio_codec = result.metadata.audioCodec; - if (result.metadata.width) updateData.width = result.metadata.width; - if (result.metadata.height) updateData.height = result.metadata.height; + // A thumbnail/probe failure must not fail the row: processPhoto's caller + // marks failed rows 'failed' and the guest gallery only lists 'complete', + // so the video would become permanently invisible. The image branch below + // already survives its thumbnail failures — mirror that: fall back to + // metadata-only plus the static play-button placeholder. A completed + // video with a NULL thumbnail would make the grid fetch the ORIGINAL + // video file as an blob (thumbnail_url || url) — a multi-GB + // download for a broken tile (codex review of #845). + let videoResult = null; + try { + videoResult = await processUploadedVideo(localPath, videoThumbnailKey); + } catch (videoErr) { + logger.warn(`processPhoto: video processing failed for ${photoId}, using placeholder thumbnail`, { error: videoErr.message }); + try { + videoResult = { metadata: await extractVideoMetadata(localPath) }; + } catch (metaErr) { + logger.warn(`processPhoto: video metadata extraction also failed for ${photoId}`, { error: metaErr.message }); + } + // ffmpeg-free (sharp-rendered SVG); returns null on failure. + const placeholderKey = await generateVideoPlaceholder(photo.filename); + if (placeholderKey) videoResult = { ...(videoResult || {}), thumbnailKey: placeholderKey }; + } + if (videoResult?.thumbnailKey) updateData.thumbnail_path = videoResult.thumbnailKey; + if (videoResult?.metadata) { + const m = videoResult.metadata; + if (m.duration != null) updateData.duration = m.duration; + if (m.videoCodec) updateData.video_codec = m.videoCodec; + if (m.audioCodec) updateData.audio_codec = m.audioCodec; + if (m.width) updateData.width = m.width; + if (m.height) updateData.height = m.height; } } else { // RAW/DNG can't be sharp-decoded directly — extract the embedded JPEG