diff --git a/backend/__tests__/services/videoPlaceholderNoDbLookup.test.js b/backend/__tests__/services/videoPlaceholderNoDbLookup.test.js new file mode 100644 index 00000000..78efac31 --- /dev/null +++ b/backend/__tests__/services/videoPlaceholderNoDbLookup.test.js @@ -0,0 +1,62 @@ +/** + * generateVideoPlaceholder() must not touch the database when the caller + * already supplies width/height (videoProcessor.js's thumbnail-generation + * fallback does exactly this). + * + * Why it matters: processUploadedPhotos() (chunked video upload) holds a + * per-file SQLite transaction open across thumbnail generation. SQLite's + * knex pool defaults to a single connection, so any second, un-transacted + * db() query made while that transaction is open blocks until + * acquireConnectionTimeout (60s in production) — verified directly against + * an isolated SQLite db (codex review of #1371/#1372). Passing explicit + * dimensions must skip getThumbnailSettings()'s db() call entirely, not + * just tolerate its failure. + */ + +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); + +const mockDbSpy = jest.fn(() => { + throw new Error('db() must not be called when width/height are supplied'); +}); +jest.mock('../../src/database/db', () => ({ db: (...args) => mockDbSpy(...args) })); + +const storageModule = require('../../src/services/storage'); +const LocalFsStorage = require('../../src/services/storage/LocalFsStorage'); + +describe('generateVideoPlaceholder skips the settings DB lookup given explicit dimensions', () => { + let storage; + let root; + let imageProcessor; + + beforeAll(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-vidplaceholder-')); + storage = new LocalFsStorage({ root }); + await storage.init(); + storageModule.setStorageForTesting(storage); + imageProcessor = require('../../src/services/imageProcessor'); + }, 30000); + + afterAll(async () => { + storageModule.resetStorage(); + await fs.rm(root, { recursive: true, force: true }).catch(() => {}); + }); + + afterEach(() => mockDbSpy.mockClear()); + + it('never calls db() when width/height are provided', async () => { + const key = await imageProcessor.generateVideoPlaceholder('demo.mp4', { width: 300, height: 300 }); + + expect(key).toBe('thumbnails/thumb_demo.jpg'); + expect(await storage.exists(key)).toBe(true); + expect(mockDbSpy).not.toHaveBeenCalled(); + }); + + it('falls through to defaults (not a throw) when db() fails and no dimensions were given', async () => { + const key = await imageProcessor.generateVideoPlaceholder('demo2.mp4'); + + expect(key).toBe('thumbnails/thumb_demo2.jpg'); + expect(mockDbSpy).toHaveBeenCalled(); + }); +}); diff --git a/backend/src/services/__tests__/videoProcessor.test.js b/backend/src/services/__tests__/videoProcessor.test.js new file mode 100644 index 00000000..a6638b1d --- /dev/null +++ b/backend/src/services/__tests__/videoProcessor.test.js @@ -0,0 +1,125 @@ +jest.mock('../../utils/logger'); +jest.mock('fluent-ffmpeg'); +jest.mock('../storage', () => ({ + getStorage: jest.fn() +})); +jest.mock('../imageProcessor', () => ({ + generateVideoPlaceholder: jest.fn(), + DEFAULT_THUMBNAIL_WIDTH: 300, + DEFAULT_THUMBNAIL_HEIGHT: 300 +})); + +const ffmpeg = require('fluent-ffmpeg'); +const { getStorage } = require('../storage'); +const { generateVideoPlaceholder } = require('../imageProcessor'); +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); + generateVideoPlaceholder.mockResolvedValue('thumbnails/thumb_placeholder.jpg'); + }); + + 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'); + // A real thumbnail already succeeded — never touch the placeholder path. + expect(generateVideoPlaceholder).not.toHaveBeenCalled(); + }); + + it('falls back to the SVG placeholder when thumbnail generation fails, so the gallery never falls back to rendering the raw video as an (codex review)', 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_wedding_001.jpg'); + + expect(result.success).toBe(true); + expect(result.metadata).toEqual(expect.objectContaining({ duration: 5, videoCodec: 'h264' })); + // thumbnailKey is always thumbnails/thumb_.jpg — strip the prefix + // back to a filename so generateVideoPlaceholder recomputes the same key. + // Explicit width/height so generateVideoPlaceholder skips its DB-backed + // settings lookup — this can run inside an open per-file SQLite + // transaction (chunked video upload), where that lookup deadlocks. + expect(generateVideoPlaceholder).toHaveBeenCalledWith('wedding_001.jpg', { width: 300, height: 300 }); + expect(result.thumbnailKey).toBe('thumbnails/thumb_placeholder.jpg'); + expect(storage.putFromFile).not.toHaveBeenCalled(); + }); + + it('throws when metadata, thumbnail generation, AND the placeholder all fail, so the caller surfaces a retryable failure instead of completing with nothing to show (codex review)', 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; + } + })); + generateVideoPlaceholder.mockRejectedValue(new Error('sharp render failed')); + + await expect(processUploadedVideo('/tmp/corrupt.mp4', 'thumbnails/thumb_corrupt.jpg')) + .rejects.toThrow('Unable to generate any thumbnail'); + }); +}); diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index c7e462c6..3b6a8ece 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -610,9 +610,15 @@ async function generateVideoPlaceholder(originalFilename, options = {}) { const thumbnailRelKey = path.posix.join('thumbnails', thumbnailFilename); const storage = getStorage(); - const settings = await getThumbnailSettings(); - const width = settings.width || DEFAULT_THUMBNAIL_WIDTH; - const height = settings.height || DEFAULT_THUMBNAIL_HEIGHT; + // Skip the settings lookup when the caller already supplies dimensions. + // This can run from inside an open per-file SQLite transaction (chunked + // video upload's fallback path in videoProcessor.js) — a second, + // un-transacted db() query for settings there deadlocks against SQLite's + // single-connection pool until acquireConnectionTimeout (60s), reproduced + // directly against an isolated SQLite db (codex review of #1371/#1372). + const settings = (options.width && options.height) ? {} : await getThumbnailSettings(); + const width = options.width || settings.width || DEFAULT_THUMBNAIL_WIDTH; + const height = options.height || settings.height || DEFAULT_THUMBNAIL_HEIGHT; if (options.regenerate) { await storage.delete(thumbnailRelKey).catch(() => {}); @@ -1495,4 +1501,6 @@ module.exports = { extractRawPreview, withProcessableImage, RAW_EXTENSIONS, + DEFAULT_THUMBNAIL_WIDTH, + DEFAULT_THUMBNAIL_HEIGHT, }; diff --git a/backend/src/services/videoProcessor.js b/backend/src/services/videoProcessor.js index f699d284..eedad31d 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,108 @@ 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. metadata is still + * allowed to come back null (ffprobe failed) — a video with no thumbnail + * would fall back to rendering the raw video as an in the gallery + * grid (`photo.thumbnail_url || photo.url`), so this only resolves when a + * real thumbnail or the SVG placeholder produced *something*; if both fail + * (storage backend down, disk full — not a quirk of one file) it throws + * instead, so the caller surfaces a retryable failure rather than silently + * completing with nothing to show. + * * @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}>} */ 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 + }); + } + + // Never return "success" with no thumbnail at all: the gallery grid + // (GridGalleryLayout/JustifiedGalleryLayout) falls back to + // `photo.thumbnail_url || photo.url` when there's no thumbnail, which + // makes AuthenticatedImage download the full ORIGINAL VIDEO and try to + // render it as an — a broken tile and a multi-GB fetch just from + // opening the gallery (codex review, #1371/#1372). Fall back to the same + // ffmpeg-free SVG placeholder the callers already generate for a total + // processing failure, so a bare thumbnail-generation failure degrades to + // that placeholder too, not to "no thumbnail". thumbnailKey is always + // `thumbnails/thumb_.jpg` (see callers) — strip the prefix back to + // a filename so generateVideoPlaceholder recomputes this exact same key. + if (!generatedThumbnailKey) { + try { + const { + generateVideoPlaceholder, + DEFAULT_THUMBNAIL_WIDTH, + DEFAULT_THUMBNAIL_HEIGHT + } = require('./imageProcessor'); + const placeholderFilename = path.basename(thumbnailKey).replace(/^thumb_/, ''); + // Explicit width/height make generateVideoPlaceholder skip its + // configured-thumbnail-size DB lookup (see its own comment) — this + // call can run from inside processUploadedPhotos' open per-file + // SQLite transaction, where that lookup would otherwise deadlock. + const placeholderKey = await generateVideoPlaceholder(placeholderFilename, { + width: DEFAULT_THUMBNAIL_WIDTH, + height: DEFAULT_THUMBNAIL_HEIGHT + }); + if (placeholderKey) { + generatedThumbnailKey = placeholderKey; + } + } catch (error) { + logger.error('Video placeholder generation also failed', { error: error.message, videoPath }); + } + } + + // A real thumbnail AND the ffmpeg-free SVG placeholder both failing points + // at something systemic (storage backend down, disk full) rather than a + // quirk of this one file — that's worth surfacing as a retryable failure + // rather than silently completing with no thumbnail at all, which would + // make the gallery fall back to rendering the raw video as an + // (codex review, #1371/#1372). Metadata (if any was extracted) is lost + // here, same trade-off the callers' own pre-existing total-failure + // handling already makes. + if (!generatedThumbnailKey) { + throw new Error('Unable to generate any thumbnail (real or placeholder) for this video'); + } + + return { + success: true, + metadata, + thumbnailKey: generatedThumbnailKey + }; } /**