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:
@@ -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 });
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -32,7 +32,10 @@ async function extractVideoMetadata(videoPath) {
|
|||||||
const audioStream = metadata.streams.find(s => s.codec_type === 'audio');
|
const audioStream = metadata.streams.find(s => s.codec_type === 'audio');
|
||||||
|
|
||||||
const result = {
|
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,
|
width: videoStream?.width || null,
|
||||||
height: videoStream?.height || null,
|
height: videoStream?.height || null,
|
||||||
videoCodec: videoStream?.codec_name || 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
|
* Process an uploaded video: extract metadata and produce a thumbnail through
|
||||||
* the storage backend.
|
* 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} videoPath - Local path to the source video (ffmpeg requires fs).
|
||||||
* @param {string} thumbnailKey - Relative storage key for the thumbnail.
|
* @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 = {}) {
|
async function processUploadedVideo(videoPath, thumbnailKey, options = {}) {
|
||||||
|
let metadata = null;
|
||||||
try {
|
try {
|
||||||
const isValid = await isValidVideo(videoPath);
|
metadata = await extractVideoMetadata(videoPath);
|
||||||
if (!isValid) {
|
} catch (error) {
|
||||||
throw new Error('Invalid video file');
|
logger.error('Video metadata extraction failed — continuing without duration/codec/dimensions', {
|
||||||
|
error: error.message,
|
||||||
|
videoPath
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
const metadata = await extractVideoMetadata(videoPath);
|
let generatedThumbnailKey = null;
|
||||||
|
try {
|
||||||
await generateVideoThumbnail(videoPath, thumbnailKey, options);
|
await generateVideoThumbnail(videoPath, thumbnailKey, options);
|
||||||
|
|
||||||
const storage = getStorage();
|
const storage = getStorage();
|
||||||
const exists = await storage.exists(thumbnailKey);
|
if (await storage.exists(thumbnailKey)) {
|
||||||
if (!exists) {
|
generatedThumbnailKey = thumbnailKey;
|
||||||
throw new Error('Thumbnail generation failed (not in storage)');
|
}
|
||||||
|
} catch (error) {
|
||||||
|
logger.error('Video thumbnail generation failed — continuing without a thumbnail', {
|
||||||
|
error: error.message,
|
||||||
|
videoPath
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
success: true,
|
success: true,
|
||||||
metadata,
|
metadata,
|
||||||
thumbnailKey
|
thumbnailKey: generatedThumbnailKey
|
||||||
};
|
};
|
||||||
} catch (error) {
|
|
||||||
logger.error('Error processing video', { error: error.message, videoPath });
|
|
||||||
throw error;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
Reference in New Issue
Block a user