diff --git a/invokeai/app/api/routers/videos.py b/invokeai/app/api/routers/videos.py index 446a4aa3009..65f5c03b234 100644 --- a/invokeai/app/api/routers/videos.py +++ b/invokeai/app/api/routers/videos.py @@ -50,7 +50,11 @@ VideoUrlsDTO, ) from invokeai.app.util.video_ingest import VideoIngestError, ingest_media_to_mp4, probe_media_streams -from invokeai.app.util.video_thumbnails import VideoDecodeTimeoutError, extract_video_frame, probe_video_with_codec +from invokeai.app.util.video_thumbnails import ( + VideoDecodeTimeoutError, + extract_representative_video_frame, + probe_video_with_codec, +) videos_router = APIRouter(prefix="/v1/videos", tags=["videos"]) @@ -178,20 +182,24 @@ def _is_mp4_file(path: Path) -> bool: def _probe_decodable_video(path: Path) -> tuple[tuple[int, int, float, Optional[float]], Optional[PILImage.Image]]: - """Probes metadata and proves the video has a decodable first frame. + """Probes metadata and proves the video has a decodable frame. Returns the metadata plus the decoded frame so the save path can reuse it as the - thumbnail source instead of spawning another decode worker. A decode timeout is - contention on a loaded server, not evidence the video is bad — probe_video already - succeeded — so it yields (metadata, None) and the upload proceeds, with save-time - thumbnail extraction as the backstop. + thumbnail source instead of spawning another decode worker. The frame is taken ~1s into + the clip (see representative_thumbnail_frame_index) rather than at index 0 — first + frames are routinely unrepresentative (fade-ins, the synthesized waveform track's empty + first window) — with a frame-0 fallback inside the helper. Acceptance is thereby + slightly WIDER than before: a file whose frame 0 is corrupt but whose ~1s frame decodes + is now accepted rather than 415'd. A decode timeout is contention on a loaded server, not evidence the video is + bad — probe_video already succeeded — so it yields (metadata, None) and the upload + proceeds, with save-time thumbnail extraction as the backstop. """ width, height, duration, fps, codec = probe_video_with_codec(path) if codec is None or codec.lower() not in {"h264", "avc", "avc1", "libx264"}: raise ValueError("Video must use a browser-compatible H.264/AVC codec") metadata = (width, height, duration, fps) try: - first_frame = extract_video_frame(path, frame_index=0, raise_on_timeout=True) + first_frame = extract_representative_video_frame(path, duration, fps, raise_on_timeout=True) except VideoDecodeTimeoutError: return metadata, None if first_frame is None: diff --git a/invokeai/app/services/video_files/video_files_base.py b/invokeai/app/services/video_files/video_files_base.py index 09f6b790507..32100e7cc7b 100644 --- a/invokeai/app/services/video_files/video_files_base.py +++ b/invokeai/app/services/video_files/video_files_base.py @@ -25,10 +25,15 @@ def save( graph: Optional[str] = None, first_frame: Optional[Image.Image] = None, move_source: bool = True, + duration: Optional[float] = None, + fps: Optional[float] = None, ) -> None: """Saves a video by moving the file at `source_path` into storage, then writes a sibling - WEBP thumbnail extracted from the first frame, plus an optional sidecar JSON of metadata/workflow/graph. - A caller that already decoded frame 0 can pass it as `first_frame` to skip the extraction. + WEBP thumbnail, plus an optional sidecar JSON of metadata/workflow/graph. + The thumbnail is extracted from a representative frame (~1s in; see + ``representative_thumbnail_frame_index``), located using ``duration``/``fps`` when the + caller knows them. A caller that already decoded a representative frame can pass it as + `first_frame` to skip the extraction. `source_path` is **consumed** by default: almost every caller hands over a temp file it just wrote, and moving it is both cheaper and the correct lifetime. A caller whose source is a diff --git a/invokeai/app/services/video_files/video_files_disk.py b/invokeai/app/services/video_files/video_files_disk.py index 120de6d04a3..c4e36d99127 100644 --- a/invokeai/app/services/video_files/video_files_disk.py +++ b/invokeai/app/services/video_files/video_files_disk.py @@ -16,7 +16,7 @@ VideoFileSaveException, ) from invokeai.app.util.thumbnails import make_thumbnail -from invokeai.app.util.video_thumbnails import extract_video_frame, get_video_thumbnail_name +from invokeai.app.util.video_thumbnails import extract_representative_video_frame, get_video_thumbnail_name from invokeai.backend.util.logging import InvokeAILogger @@ -52,6 +52,8 @@ def save( graph: Optional[str] = None, first_frame: Optional[Image.Image] = None, move_source: bool = True, + duration: Optional[float] = None, + fps: Optional[float] = None, ) -> None: logger = InvokeAILogger.get_logger() try: @@ -80,12 +82,13 @@ def save( # Thumbnail extraction is best-effort — if both imageio and cv2 fail, we still want # the video record + file in place and the invocation to complete. A missing # thumbnail leaves the gallery with a broken-image placeholder for that item, which - # is annoying but not fatal. The upload path already decoded frame 0 to prove - # decodability and passes it in, saving a decode-worker subprocess per upload. + # is annoying but not fatal. The upload path already decoded a representative frame + # to prove decodability and passes it in, saving a decode-worker subprocess per + # upload; this fallback picks the same ~1s-in frame for generated/derived videos. frame = first_frame if frame is None: try: - frame = extract_video_frame(video_path, frame_index=0) + frame = extract_representative_video_frame(video_path, duration, fps) except Exception as e: logger.warning(f"Thumbnail extraction raised for {video_name}: {e}") frame = None diff --git a/invokeai/app/services/videos/videos_default.py b/invokeai/app/services/videos/videos_default.py index f344f33f8b2..80ffbbcdab9 100644 --- a/invokeai/app/services/videos/videos_default.py +++ b/invokeai/app/services/videos/videos_default.py @@ -115,6 +115,8 @@ def create( graph=graph, first_frame=first_frame, move_source=move_source, + duration=duration, + fps=fps, ) video_dto = self.get_dto(video_name) @@ -177,7 +179,7 @@ def copy(self, source_video_name: str, board_id: Optional[str] = None, user_id: with Image.open(thumbnail_path) as thumbnail: first_frame = thumbnail.copy() except Exception: - # A thumbnail is an optimization only. ``create`` extracts frame zero when absent. + # A thumbnail is an optimization only. ``create`` extracts a representative frame when absent. first_frame = None created = self.create( diff --git a/invokeai/app/util/video_thumbnails.py b/invokeai/app/util/video_thumbnails.py index 678e0847373..b7e9e8fcb7a 100644 --- a/invokeai/app/util/video_thumbnails.py +++ b/invokeai/app/util/video_thumbnails.py @@ -401,6 +401,64 @@ def extract_video_frame( Path(tmp_name).unlink(missing_ok=True) +# Where in a clip the gallery thumbnail is taken from. Frame 0 is a poor representative: +# generated videos commonly fade in from black or start on a conditioning frame, and an +# audio-only upload wrapped in a synthesized waveform track (see video_ingest.py) renders its +# first frame from a near-empty audio window — an all-black tile. About a second in, capped at +# the clip's midpoint so short clips still resolve to a real frame, is far more representative. +THUMBNAIL_FRAME_TARGET_SECONDS = 1.0 +# Used when the container reports no usable fps; matches the video models' native rate and the +# synthesized waveform track's rate. +THUMBNAIL_FRAME_FALLBACK_FPS = 24.0 + + +def representative_thumbnail_frame_index(duration: Optional[float], fps: Optional[float]) -> int: + """The frame index a gallery thumbnail should be taken from. + + Roughly THUMBNAIL_FRAME_TARGET_SECONDS into the clip, capped at its midpoint. Returns 0 + when the duration is unknown or degenerate — callers without metadata keep today's + first-frame behavior. Both inputs are untrusted container metadata, so a non-finite value + degrades to the safe answer rather than raising. + """ + if duration is None or not math.isfinite(duration) or duration <= 0: + return 0 + effective_fps = fps if fps is not None and math.isfinite(fps) and fps > 0 else THUMBNAIL_FRAME_FALLBACK_FPS + target_seconds = min(THUMBNAIL_FRAME_TARGET_SECONDS, duration / 2) + return max(0, int(target_seconds * effective_fps)) + + +def extract_representative_video_frame( + video_path: Path, + duration: Optional[float] = None, + fps: Optional[float] = None, + timeout: float = VIDEO_DECODE_TIMEOUT_SECONDS, + raise_on_timeout: bool = False, +) -> Optional[Image.Image]: + """Extracts the representative thumbnail frame, falling back to frame 0. + + The fallback covers containers whose metadata overstates the decodable range (the + computed index then has no frame): such files still get a thumbnail rather than a + gallery placeholder. In the worst case — a slow decode *failure* at the representative + index — the fallback costs a second decode budget, bounding one call at two timeouts. + + A timeout is never retried, in either mode: it is contention or an adversarial file, + not evidence about the index, and a retry would hold a request worker for another full + budget. Timeouts are detected by always raising internally; with ``raise_on_timeout`` + the VideoDecodeTimeoutError propagates, otherwise the call returns None as + ``extract_video_frame`` would. + """ + frame_index = representative_thumbnail_frame_index(duration, fps) + try: + frame = extract_video_frame(video_path, frame_index=frame_index, timeout=timeout, raise_on_timeout=True) + if frame is None and frame_index > 0: + frame = extract_video_frame(video_path, frame_index=0, timeout=timeout, raise_on_timeout=True) + except VideoDecodeTimeoutError: + if raise_on_timeout: + raise + return None + return frame + + def probe_video_with_codec( video_path: Path, timeout: float = VIDEO_DECODE_TIMEOUT_SECONDS ) -> tuple[int, int, float, Optional[float], Optional[str]]: diff --git a/tests/app/api/test_video_upload_limits.py b/tests/app/api/test_video_upload_limits.py index 1d1df14fd3f..3a5a35f46b6 100644 --- a/tests/app/api/test_video_upload_limits.py +++ b/tests/app/api/test_video_upload_limits.py @@ -43,7 +43,9 @@ def test_configured_upload_slots_bound_peak_double_spool_usage() -> None: def test_upload_probe_requires_a_decodable_frame(monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr(videos, "probe_video_with_codec", lambda path: (48, 32, 1.0, 8.0, "h264")) - monkeypatch.setattr(videos, "extract_video_frame", lambda path, frame_index=0, raise_on_timeout=False: None) + monkeypatch.setattr( + videos, "extract_representative_video_frame", lambda path, duration=None, fps=None, raise_on_timeout=False: None + ) with pytest.raises(ValueError, match="decodable frame"): videos._probe_decodable_video(Path("metadata-only.mp4")) @@ -52,7 +54,11 @@ def test_upload_probe_requires_a_decodable_frame(monkeypatch: pytest.MonkeyPatch def test_upload_probe_accepts_valid_metadata_and_frame(monkeypatch: pytest.MonkeyPatch): frame = MagicMock() monkeypatch.setattr(videos, "probe_video_with_codec", lambda path: (48, 32, 1.0, 8.0, "h264")) - monkeypatch.setattr(videos, "extract_video_frame", lambda path, frame_index=0, raise_on_timeout=False: frame) + monkeypatch.setattr( + videos, + "extract_representative_video_frame", + lambda path, duration=None, fps=None, raise_on_timeout=False: frame, + ) assert videos._probe_decodable_video(Path("valid.mp4")) == ((48, 32, 1.0, 8.0), frame) @@ -61,11 +67,11 @@ def test_upload_probe_timeout_is_inconclusive_not_a_rejection(monkeypatch: pytes """A decode-worker timeout is server contention, not evidence the video is bad — the upload must proceed (without a pre-extracted frame) rather than 415.""" - def _timeout(path, frame_index=0, raise_on_timeout=False): + def _timeout(path, duration=None, fps=None, raise_on_timeout=False): raise VideoDecodeTimeoutError("decode worker timed out") monkeypatch.setattr(videos, "probe_video_with_codec", lambda path: (48, 32, 1.0, 8.0, "h264")) - monkeypatch.setattr(videos, "extract_video_frame", _timeout) + monkeypatch.setattr(videos, "extract_representative_video_frame", _timeout) assert videos._probe_decodable_video(Path("busy-server.mp4")) == ((48, 32, 1.0, 8.0), None) diff --git a/tests/app/routers/test_videos_multiuser.py b/tests/app/routers/test_videos_multiuser.py index f2411e99879..984c8dffb6a 100644 --- a/tests/app/routers/test_videos_multiuser.py +++ b/tests/app/routers/test_videos_multiuser.py @@ -939,7 +939,9 @@ def test_uploaded_video_codec_must_be_browser_compatible( lambda _path: (64, 64, 1.0, 8.0, codec), raising=False, ) - monkeypatch.setattr(videos_router_module, "extract_video_frame", lambda *_args, **_kwargs: MagicMock()) + monkeypatch.setattr( + videos_router_module, "extract_representative_video_frame", lambda *_args, **_kwargs: MagicMock() + ) if is_supported: metadata, _frame = videos_router_module._probe_decodable_video(path) diff --git a/tests/app/services/video_files/test_video_files_disk.py b/tests/app/services/video_files/test_video_files_disk.py index e8a347a7c74..d8c6a44dcc6 100644 --- a/tests/app/services/video_files/test_video_files_disk.py +++ b/tests/app/services/video_files/test_video_files_disk.py @@ -87,7 +87,7 @@ def test_save_failure_in_thumbnail_write_removes_moved_video( # Frame extraction itself is best-effort, but a failure while *writing* the extracted # thumbnail propagates. Simulate that: extraction succeeds, the write blows up. monkeypatch.setattr( - "invokeai.app.services.video_files.video_files_disk.extract_video_frame", + "invokeai.app.services.video_files.video_files_disk.extract_representative_video_frame", lambda *args, **kwargs: MagicMock(), ) broken_thumbnail = MagicMock() diff --git a/tests/app/util/test_video_thumbnails.py b/tests/app/util/test_video_thumbnails.py index a8fb7272770..fc2f0dc92aa 100644 --- a/tests/app/util/test_video_thumbnails.py +++ b/tests/app/util/test_video_thumbnails.py @@ -696,3 +696,124 @@ def test_rejects_later_frame_over_pixel_limit(self, monkeypatch: pytest.MonkeyPa def test_rejects_non_rgb_frames(self, frame: np.ndarray) -> None: with pytest.raises(ValueError, match="RGB"): video_decode_worker._validate_decoded_frame(frame) + + +class TestRepresentativeThumbnailFrame: + """The gallery thumbnail is taken ~1s in (capped at the clip midpoint), not at frame 0 — + first frames are routinely black (fade-ins, the waveform wrap's empty first window).""" + + @pytest.mark.parametrize( + ("duration", "fps", "expected"), + [ + (None, 24.0, 0), # unknown duration -> keep first-frame behavior + (0.0, 24.0, 0), + (-1.0, 24.0, 0), + (float("inf"), 24.0, 0), # untrusted metadata degrades safely + (float("nan"), 24.0, 0), + (10.0, 24.0, 24), # long clip: 1s in + (10.0, 8.0, 8), + (1.0, 24.0, 12), # short clip: capped at the midpoint + (10.0, None, 24), # unknown fps -> 24 fps assumption + (10.0, 0.0, 24), + (10.0, float("inf"), 24), + (0.01, 24.0, 0), # sub-frame midpoint resolves to frame 0 + ], + ) + def test_index_selection(self, duration, fps, expected) -> None: + assert video_thumbnails.representative_thumbnail_frame_index(duration, fps) == expected + + def test_extracts_the_later_frame_through_the_worker(self, synthetic_mp4: Path) -> None: + """The synthetic clip brightens by 16 per frame, so the pixel value identifies the + frame: the representative extraction must not return frame 0.""" + duration = FRAMES / FPS + expected_index = video_thumbnails.representative_thumbnail_frame_index(duration, FPS) + assert expected_index > 0 + frame = video_thumbnails.extract_representative_video_frame(synthetic_mp4, duration, FPS) + assert frame is not None + value = np.asarray(frame)[0, 0, 0].astype(int) + assert abs(value - (32 + expected_index * 16)) <= 8 + assert abs(value - 32) > 8 # decisively not frame 0 + + def test_falls_back_to_frame_zero_when_metadata_overstates(self, synthetic_mp4: Path) -> None: + """Container metadata is untrusted: fps/duration claiming frames that don't exist + (index 100 of a 12-frame clip) must still produce a thumbnail (frame 0), not a + gallery placeholder.""" + frame = video_thumbnails.extract_representative_video_frame(synthetic_mp4, duration=10.0, fps=100.0) + assert frame is not None + value = np.asarray(frame)[0, 0, 0].astype(int) + assert abs(value - 32) <= 8 # frame 0 + + def test_fallback_calls_and_order(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + calls: list[int] = [] + + def fake_extract(path, frame_index=0, timeout=0.0, raise_on_timeout=False): + calls.append(frame_index) + return None if frame_index > 0 else MagicMock() + + monkeypatch.setattr(video_thumbnails, "extract_video_frame", fake_extract) + frame = video_thumbnails.extract_representative_video_frame(tmp_path / "v.mp4", duration=10.0, fps=24.0) + assert frame is not None + assert calls == [24, 0] + + def test_no_fallback_when_index_is_zero(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + calls: list[int] = [] + + def fake_extract(path, frame_index=0, timeout=0.0, raise_on_timeout=False): + calls.append(frame_index) + return None + + monkeypatch.setattr(video_thumbnails, "extract_video_frame", fake_extract) + assert video_thumbnails.extract_representative_video_frame(tmp_path / "v.mp4", duration=None, fps=None) is None + assert calls == [0] + + def test_timeout_propagates_without_a_second_decode(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """A timeout is contention, not evidence about the frame index — retrying at frame 0 + would double the time an adversarial upload holds a request worker.""" + calls: list[int] = [] + + def fake_extract(path, frame_index=0, timeout=0.0, raise_on_timeout=False): + calls.append(frame_index) + raise video_thumbnails.VideoDecodeTimeoutError("busy") + + monkeypatch.setattr(video_thumbnails, "extract_video_frame", fake_extract) + with pytest.raises(video_thumbnails.VideoDecodeTimeoutError): + video_thumbnails.extract_representative_video_frame( + tmp_path / "v.mp4", duration=10.0, fps=24.0, raise_on_timeout=True + ) + assert calls == [24] + + def test_timeout_in_non_raise_mode_returns_none_without_retry( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """The no-retry-on-timeout rule must hold in non-raise mode too (the disk store's + mode): a timeout there must not trigger a second full-budget frame-0 decode.""" + calls: list[int] = [] + + def fake_extract(path, frame_index=0, timeout=0.0, raise_on_timeout=False): + calls.append(frame_index) + raise video_thumbnails.VideoDecodeTimeoutError("busy") + + monkeypatch.setattr(video_thumbnails, "extract_video_frame", fake_extract) + result = video_thumbnails.extract_representative_video_frame( + tmp_path / "v.mp4", duration=10.0, fps=24.0, raise_on_timeout=False + ) + assert result is None + assert calls == [24] + + def test_timeout_during_fallback_is_not_swallowed_in_raise_mode( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + calls: list[int] = [] + + def fake_extract(path, frame_index=0, timeout=0.0, raise_on_timeout=False): + calls.append(frame_index) + if frame_index > 0: + return None + raise video_thumbnails.VideoDecodeTimeoutError("busy") + + monkeypatch.setattr(video_thumbnails, "extract_video_frame", fake_extract) + with pytest.raises(video_thumbnails.VideoDecodeTimeoutError): + video_thumbnails.extract_representative_video_frame( + tmp_path / "v.mp4", duration=10.0, fps=24.0, raise_on_timeout=True + ) + assert calls == [24, 0]