diff --git a/photomap/backend/embeddings.py b/photomap/backend/embeddings.py index af661f4e..deabfba3 100644 --- a/photomap/backend/embeddings.py +++ b/photomap/backend/embeddings.py @@ -50,7 +50,11 @@ from .thumbnail_cache import keep_hashes_for, thumbnail_dir from .thumbnail_cache import prune as prune_thumbnails from .util import atomic_savez -from .video import VIDEO_METADATA_KEY, extract_video_frame +from .video import ( + VIDEO_METADATA_KEY, + extract_video_frame, + ffmpeg_known_unavailable, +) from .video_cache import VideoFrameCache from .video_transcode import TranscodeCache @@ -1349,10 +1353,43 @@ def _register_unreadable_files_warning( return count = len(result.bad_files) noun, verb = ("file", "was") if count == 1 else ("files", "were") - progress_tracker.add_completion_warning( - album_key, - f"{count} {noun} could not be read and {verb} skipped.", - ) + message = f"{count} {noun} could not be read and {verb} skipped." + + # On a platform with no ffmpeg binary *every* video fails, and the + # generic notice above sends the user hunting for corrupt files that + # are in fact fine. Naming the cause is the difference between "my + # videos are broken" and "this machine needs ffmpeg". + # + # ``ffmpeg_known_unavailable`` rather than ``ffmpeg_exe()``: this runs + # on the asyncio event loop (both callers are ``async def``), and + # ffmpeg_exe can stat a hung network mount or spawn an untimed + # ``ffmpeg -version``. It only reports the probe every video failure + # already performed on a worker thread. + # + # Worded as what this process observed, not as a claim about the + # machine: imageio memoizes its own negative, so a binary that exists + # but could not be executed once (an AV scanner holding a freshly + # unpacked ffmpeg.exe) stays unavailable for the process lifetime, and + # "there is no ffmpeg here" would be a lie to someone who can see it. + videos = sum(1 for path in result.bad_files if is_video(path)) + if videos and ffmpeg_known_unavailable(): + video_noun = "video" if videos == 1 else "videos" + if videos == count: + message = ( + f"{count} {video_noun} could not be indexed and {verb} " + "skipped: PhotoMapAI could not find a working ffmpeg." + ) + else: + # Plural here is independent of ``verb``: the mixed branch + # always has count >= 2, but ``videos`` can still be 1. + needs = "needs" if videos == 1 else "need" + message = ( + f"{count} {noun} could not be read and {verb} skipped, " + f"including {videos} {video_noun} that {needs} ffmpeg, " + "which PhotoMapAI could not find." + ) + + progress_tracker.add_completion_warning(album_key, message) logger.warning( f"Skipped {count} unreadable {noun} in album '{album_key}': " + ", ".join(p.name for p in result.bad_files[:5]) diff --git a/photomap/backend/video.py b/photomap/backend/video.py index 0ae92d46..ad3e1b1b 100644 --- a/photomap/backend/video.py +++ b/photomap/backend/video.py @@ -247,9 +247,28 @@ def _has_decodable_video_stream(report: str) -> bool: _ffmpeg_exe_cache: str | None = None _ffmpeg_exe_probed = False +_ffmpeg_missing = False _ffmpeg_exe_lock = threading.Lock() +def ffmpeg_known_unavailable() -> bool: + """Did the most recent probe in this process fail to find a binary? + + Reports what already happened instead of asking again. Callers on the + asyncio event loop must not trigger :func:`ffmpeg_exe`'s work: it stats + whatever ``$IMAGEIO_FFMPEG_EXE`` names, which is accepted unchecked by + imageio and may be a hung network mount, and the underlying probe spawns + ``ffmpeg -version`` with no timeout. Every video failure has already + driven a real probe on a worker thread (``_load_video`` -> + ``extract_video_frame`` -> :func:`_run_ffmpeg`), so by the time a run + reports its skipped files this is an answer rather than a guess. + + ``False`` until something has probed: absence of evidence has to read as + "do not blame ffmpeg", or a caller would invent a cause it never observed. + """ + return _ffmpeg_missing + + def ffmpeg_exe() -> str | None: """A runnable ffmpeg, or ``None`` if this platform has none. @@ -270,7 +289,7 @@ def ffmpeg_exe() -> str | None: name ``"ffmpeg"`` for a PATH lookup, so a non-``None`` answer is not on its own evidence that anything is executable. """ - global _ffmpeg_exe_cache, _ffmpeg_exe_probed + global _ffmpeg_exe_cache, _ffmpeg_exe_probed, _ffmpeg_missing if _ffmpeg_exe_probed: return _ffmpeg_exe_cache @@ -286,6 +305,7 @@ def ffmpeg_exe() -> str | None: logger.warning( f"No usable ffmpeg binary found ({e}); video files will be skipped." ) + _ffmpeg_missing = True return None # not memoized — the next call retries resolved = candidate if os.path.isabs(candidate) else shutil.which(candidate) @@ -294,19 +314,22 @@ def ffmpeg_exe() -> str | None: f"ffmpeg reported as {candidate!r} but is not executable; " "video files will be skipped." ) + _ffmpeg_missing = True return None _ffmpeg_exe_cache = resolved _ffmpeg_exe_probed = True + _ffmpeg_missing = False return _ffmpeg_exe_cache def _reset_ffmpeg_exe_cache() -> None: """Test seam: forget the probed binary so the next call re-resolves.""" - global _ffmpeg_exe_cache, _ffmpeg_exe_probed + global _ffmpeg_exe_cache, _ffmpeg_exe_probed, _ffmpeg_missing with _ffmpeg_exe_lock: _ffmpeg_exe_cache = None _ffmpeg_exe_probed = False + _ffmpeg_missing = False def _parse_ffmpeg_banner(stderr: str) -> dict[str, object]: diff --git a/photomap/frontend/static/css/album-manager.css b/photomap/frontend/static/css/album-manager.css index 42075325..7cbf2586 100644 --- a/photomap/frontend/static/css/album-manager.css +++ b/photomap/frontend/static/css/album-manager.css @@ -256,6 +256,12 @@ .index-status.completed { color: green !important; } +/* Completed, but something was skipped. Needs both the !important and the + extra class: .index-status.completed is also !important, so only higher + specificity can beat it — an inline style cannot. */ +.index-status.completed.with-warning { + color: #ff9800 !important; +} .index-status.error { color: #b00020 !important; } diff --git a/photomap/frontend/static/javascript/album-manager.js b/photomap/frontend/static/javascript/album-manager.js index 896f3cf4..f28da170 100644 --- a/photomap/frontend/static/javascript/album-manager.js +++ b/photomap/frontend/static/javascript/album-manager.js @@ -1952,14 +1952,21 @@ export class AlbumManager { updateProgressStatus(status, progress, estimatedTime) { if (progress.status === "completed") { - status.className = AlbumManager.STATUS_CLASSES.COMPLETED; - if (progress.warning_message) { - status.textContent = `Indexing completed — ${progress.warning_message}`; - status.style.color = "#ff9800"; // Orange: completed, but with a caveat - } else { - status.textContent = "Indexing completed successfully"; - status.style.color = "green"; - } + // The colour has to come from the stylesheet, not from style.color: + // `.index-status.completed` carries an author !important, which outranks + // a *normal* inline declaration, so the orange set here was silently + // discarded and every warning rendered in success-green — the one state + // where the colour is carrying the message. + const hasWarning = Boolean(progress.warning_message); + status.className = hasWarning + ? `${AlbumManager.STATUS_CLASSES.COMPLETED} with-warning` + : AlbumManager.STATUS_CLASSES.COMPLETED; + status.textContent = hasWarning + ? `Indexing completed — ${progress.warning_message}` + : "Indexing completed successfully"; + // Clear any inline colour a previous poll tick left behind (the branches + // below still set one), so the rule that applies is this state's. + status.style.color = ""; estimatedTime.textContent = ""; } else if (progress.status === "error") { status.className = AlbumManager.STATUS_CLASSES.ERROR; diff --git a/tests/backend/test_video_guards.py b/tests/backend/test_video_guards.py index 7a1f1398..d46a237d 100644 --- a/tests/backend/test_video_guards.py +++ b/tests/backend/test_video_guards.py @@ -20,8 +20,8 @@ from fixtures import media_fixture_path from PIL import Image -from photomap.backend.embeddings import Embeddings, _open_npz_file -from photomap.backend.progress import ProgressTracker +from photomap.backend.embeddings import Embeddings, IndexResult, _open_npz_file +from photomap.backend.progress import ProgressTracker, progress_tracker from photomap.backend.util import atomic_savez ENCODER_SPEC = "openai-clip:ViT-B/32" @@ -440,6 +440,130 @@ def test_adding_an_empty_warning_is_a_noop(): assert tracker.get_progress("album").warning_message == "kept" +# -------------------------------------------------------------------------- +# Naming ffmpeg as the cause of a skip +# -------------------------------------------------------------------------- + + +def _skip_result(*names: str) -> IndexResult: + """An IndexResult whose only content is a list of files that failed.""" + return IndexResult( + embeddings=np.zeros((0, EMBEDDING_DIM), dtype=np.float32), + filenames=np.array([], dtype=object), + modification_times=np.array([], dtype=float), + metadata=np.array([], dtype=object), + bad_files=[Path(name) for name in names], + ) + + +def _registered_warning(monkeypatch, result: IndexResult, missing: bool) -> str: + """Run the notice builder with ffmpeg observed present/absent.""" + monkeypatch.setattr( + "photomap.backend.embeddings.ffmpeg_known_unavailable", lambda: missing + ) + captured: list[str] = [] + monkeypatch.setattr( + progress_tracker, + "add_completion_warning", + lambda album_key, message: captured.append(message), + ) + Embeddings._register_unreadable_files_warning("warning-test", result) + assert len(captured) == 1 + return captured[0] + + +def test_missing_ffmpeg_is_named_when_only_videos_failed(monkeypatch): + """Otherwise a platform with no ffmpeg wheel (win_arm64, musl, armv7) + reports every video as unreadable and sends the user hunting for corrupt + files that are perfectly fine.""" + warning = _registered_warning( + monkeypatch, _skip_result("a.mp4", "b.mov", "c.mkv"), missing=True + ) + + assert warning == ( + "3 videos could not be indexed and were skipped: PhotoMapAI could " + "not find a working ffmpeg." + ) + + +def test_missing_ffmpeg_notice_agrees_for_a_single_video(monkeypatch): + warning = _registered_warning(monkeypatch, _skip_result("only.mp4"), missing=True) + + assert warning == ( + "1 video could not be indexed and was skipped: PhotoMapAI could not " + "find a working ffmpeg." + ) + + +def test_a_mixed_failure_keeps_the_generic_count_and_adds_the_cause(monkeypatch): + """The images failed for their own reasons, so the notice must not claim + ffmpeg explains all four.""" + warning = _registered_warning( + monkeypatch, + _skip_result("a.mp4", "b.mov", "broken.jpg", "truncated.png"), + missing=True, + ) + + assert warning == ( + "4 files could not be read and were skipped, including 2 videos that " + "need ffmpeg, which PhotoMapAI could not find." + ) + + +def test_a_mixed_failure_with_one_video_still_agrees(monkeypatch): + """The mixed branch always has count >= 2, so ``verb`` is always plural + there — but ``videos`` can be 1, which needs its own verb. One video plus + some unreadable photos is the likeliest mixed shape in the wild, and + pinning only the videos == 2 case let "1 video that need ffmpeg" through. + """ + warning = _registered_warning( + monkeypatch, _skip_result("a.mp4", "broken.jpg"), missing=True + ) + + assert warning == ( + "2 files could not be read and were skipped, including 1 video that " + "needs ffmpeg, which PhotoMapAI could not find." + ) + + +def test_wording_is_unchanged_when_ffmpeg_is_present(monkeypatch): + """A truncated clip on a working install is not an ffmpeg problem, and + saying so would be actively misleading.""" + warning = _registered_warning( + monkeypatch, _skip_result("broken.mp4"), missing=False + ) + + assert warning == "1 file could not be read and was skipped." + + +def test_image_only_failures_never_mention_ffmpeg(monkeypatch): + """ffmpeg must not be consulted at all when no video failed. + + The observation is process-wide, so a machine with no ffmpeg would + otherwise let an all-photos album inherit "could not find ffmpeg" as the + explanation for two corrupt JPEGs. + """ + + def _fail(): + raise AssertionError("ffmpeg must not be consulted for an image-only skip") + + monkeypatch.setattr( + "photomap.backend.embeddings.ffmpeg_known_unavailable", _fail + ) + captured: list[str] = [] + monkeypatch.setattr( + progress_tracker, + "add_completion_warning", + lambda album_key, message: captured.append(message), + ) + + Embeddings._register_unreadable_files_warning( + "image-only", _skip_result("broken.jpg", "truncated.png") + ) + + assert captured == ["2 files could not be read and were skipped."] + + def test_set_completion_warning_still_replaces(): tracker = ProgressTracker() tracker.set_completion_warning("album", "first") diff --git a/tests/backend/test_video_probe.py b/tests/backend/test_video_probe.py index 0fee25fc..b00108dc 100644 --- a/tests/backend/test_video_probe.py +++ b/tests/backend/test_video_probe.py @@ -401,6 +401,82 @@ def test_ffmpeg_exe_points_at_a_real_binary(): assert Path(ffmpeg_exe()).exists() +def test_known_unavailable_tracks_the_last_probe(monkeypatch, tmp_path): + """The indexing completion notice reads this instead of calling + ``ffmpeg_exe`` — it runs on the asyncio event loop, where a stat of a hung + ``$IMAGEIO_FFMPEG_EXE`` mount or an untimed ``ffmpeg -version`` spawn would + freeze every request. So it has to follow the probe in both directions, + including back to False once a binary is found. + """ + video_module._reset_ffmpeg_exe_cache() + try: + # Nothing probed yet: no evidence must not read as "ffmpeg is missing". + assert video_module.ffmpeg_known_unavailable() is False + + stand_in = tmp_path / "ffmpeg" + stand_in.write_text("") + calls = [] + + def flaky(): + calls.append(1) + if len(calls) == 1: + raise OSError("Cannot allocate memory") + return str(stand_in) + + import imageio_ffmpeg + + monkeypatch.setattr(imageio_ffmpeg, "get_ffmpeg_exe", flaky) + + assert video_module.ffmpeg_exe() is None + assert video_module.ffmpeg_known_unavailable() is True + + assert video_module.ffmpeg_exe() == str(stand_in) + assert video_module.ffmpeg_known_unavailable() is False + finally: + video_module._reset_ffmpeg_exe_cache() + + +def test_known_unavailable_is_set_when_the_candidate_is_not_executable( + monkeypatch, tmp_path +): + """The other ``return None`` path: imageio hands back a name that does not + resolve to anything runnable.""" + video_module._reset_ffmpeg_exe_cache() + try: + import imageio_ffmpeg + + monkeypatch.setattr( + imageio_ffmpeg, + "get_ffmpeg_exe", + lambda: str(tmp_path / "definitely-not-here"), + ) + + assert video_module.ffmpeg_exe() is None + assert video_module.ffmpeg_known_unavailable() is True + finally: + video_module._reset_ffmpeg_exe_cache() + + +def test_resetting_the_cache_also_clears_the_unavailable_flag(monkeypatch): + """Otherwise the flag outlives the probe it describes, and the next test + (or a re-probe after the condition clears) inherits a stale verdict.""" + video_module._reset_ffmpeg_exe_cache() + try: + import imageio_ffmpeg + + monkeypatch.setattr( + imageio_ffmpeg, + "get_ffmpeg_exe", + lambda: (_ for _ in ()).throw(OSError("boom")), + ) + assert video_module.ffmpeg_exe() is None + assert video_module.ffmpeg_known_unavailable() is True + finally: + video_module._reset_ffmpeg_exe_cache() + + assert video_module.ffmpeg_known_unavailable() is False + + # -------------------------------------------------------------------------- # Banner parsing against hostile input # -------------------------------------------------------------------------- diff --git a/tests/frontend/album-manager-progress.test.js b/tests/frontend/album-manager-progress.test.js index 34bcf930..76f53c62 100644 --- a/tests/frontend/album-manager-progress.test.js +++ b/tests/frontend/album-manager-progress.test.js @@ -123,7 +123,7 @@ describe("AlbumManager completed status", () => { expect(status.textContent).toBe("Indexing completed successfully"); expect(status.className).toBe(AlbumManager.STATUS_CLASSES.COMPLETED); - expect(status.style.color).toBe("green"); + expect(status.classList.contains("with-warning")).toBe(false); }); test("surfaces a non-fatal warning_message alongside completion", () => { @@ -140,9 +140,35 @@ describe("AlbumManager completed status", () => { expect(status.textContent).toContain("Indexing completed"); expect(status.textContent).toContain("not found on disk"); - expect(status.className).toBe(AlbumManager.STATUS_CLASSES.COMPLETED); // Completion-with-a-caveat is coloured differently from a clean success. - expect(status.style.color).not.toBe("green"); + // Asserted on the class, not style.color: `.index-status.completed` is an + // author !important rule, so an inline colour never reaches the screen. + // The old assertion passed in jsdom (which happily records the ignored + // inline value) while the real page painted the warning success-green. + expect(status.classList.contains("completed")).toBe(true); + expect(status.classList.contains("with-warning")).toBe(true); + }); + + test("drops the warning class when a later run completes cleanly", () => { + const { status, estimatedTime } = makeElements(); + + callUpdate(status, { status: "completed", warning_message: "1 file was skipped." }, estimatedTime); + callUpdate(status, { status: "completed" }, estimatedTime); + + // className is reassigned wholesale, but pin it: a stale with-warning + // would paint a clean run orange for the rest of the session. + expect(status.classList.contains("with-warning")).toBe(false); + }); + + test("does not leave an inline colour that outlives the state that set it", () => { + const { status, estimatedTime } = makeElements(); + + // Scanning sets an inline orange; completing must not inherit it, since + // inline beats the non-!important base `.index-status` rule. + callUpdate(status, { status: "scanning" }, estimatedTime); + callUpdate(status, { status: "completed" }, estimatedTime); + + expect(status.style.color).toBe(""); }); });