diff --git a/TESTING.md b/TESTING.md index 15579e31..1808a641 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2301,21 +2301,28 @@ Thumbnail format and settings (#317, `py/api/thumbnail_settings.py`, - Names: `thumbnails/_thumb.webp|.jxl|.jpg` for images and videos alike. Every lookup (listings, duplicates, enrichment, the autotag scan, the history route) takes the first existing preview the file owns, the configured format first: an older - release's `_thumb.png` (or `.jpeg`, `.gif`, the source's own spelling) keeps showing - until it is converted. Two files with one stem in a folder share one preview name - (images first). A request for an older name answers the current preview. Previews + release's `_thumb.png` (or `.jpeg`, `.gif`, the source's own spelling or the + upper-case one) keeps showing, and counts as outdated, until it is converted. History's + cover uses the same lookup as the Images listing. Two files with one stem in a folder + share one preview name, made from the first source in `.png`, `.jpg`, `.jpeg`, + `.webp`, `.gif`, then videos. A request for an older name answers the current preview. Previews are served with an explicit `image/webp` / `image/jxl` type (the platform's mimetypes table may not know them) and the same cache headers; `.jxl` is served from `thumbnails/` only. - Every preview is written by `write_thumbnail`: the first frame (animated sources give a still), RGB, fitted inside the edge, encoded into a hidden temp sibling that is closed, verified to decode (a JPEG XL file Pillow cannot read is checked by its - signature) and renamed over the preview; a failure keeps the old file. + signature) and renamed over the preview; a failure keeps the old file. ffmpeg writes + with `-update 1`, so a `%` in a name is never a sequence pattern, and reads a video as + `file:`. Temp files a crash left (`..part--`) are + removed by the clear and, when older than two minutes, by a sweep at startup. - JPEG XL needs no new dependency: Pillow (natively or with `pillow_jxl`, imported when installed), else ffmpeg with `libjxl`, else `cjxl`, detected once. Quality maps to libjxl's distance (`0.1 + (100 − q) × 0.09` from 30 up: q80 → 1.9, q85 → 1.45; never lossless). Without an encoder, saving `jxl` is a 400 `jxl-unavailable` with the - reason; if the encoder disappears later, previews are written as WebP (logged once) + reason. An encoder that fails makes that write detect again once: another encoder + found is used, none at all writes the file as WebP (logged once) and refreshes the + availability Appearance reads; if JPEG XL has no encoder, previews are written as WebP and count as the format written. Encoder processes (and ffmpeg's frames) are tracked, bounded by a 30 s timeout and killed at the app's cleanup, after which none starts. - A preview in another format than the one written now is *outdated*. @@ -2324,10 +2331,16 @@ Thumbnail format and settings (#317, `py/api/thumbnail_settings.py`, (`POST /jobs/thumbnails {mode: "convert"}`, "Convert outdated thumbnails") runs under the thumbnail job's kind, guarded admission, worker threads and cancel: for each outdated file it writes the new preview from the source (a video with no decodable - frame fails instead of getting the placeholder), then removes that file's old-format - previews (regular files in its `thumbnails/` folder that the source owns, the source - still present). A failure keeps the old preview; orphans (source gone) and links are - never touched. Its final line is "N converted · F failed · → " + frame fails instead of getting the placeholder; a current preview it keeps must + decode), then removes that file's old-format previews (regular files in its + `thumbnails/` folder that the source owns, the source still present; one already gone + counts as removed). Files sharing a preview name are converted once, as a group, from + the first source above, and each file's bytes count once. A failure keeps the old + preview; orphans (source gone) and links are never touched. While it runs, a change of + the thumbnail format, quality or size limit is a 409 `busy` naming it, and should the + format still change underneath, the job stops at the next file without removing + anything ("Stopped: the thumbnail format changed …"). Its final line is "N converted · F + failed · → " (`tests/test_thumbnail_convert.py`, "Convert outdated replaces older-format previews" in `workspace-system-sources.spec.mjs`). Nothing converts by itself: Appearance's format change only makes previews outdated. @@ -2340,7 +2353,11 @@ Thumbnail format and settings (#317, `py/api/thumbnail_settings.py`, - The workspace decodes a 1×1 embedded JPEG XL once (`thumbnail-support.mjs`); where it fails, cards and the inspector cover load the original for a `.jxl` preview, and any preview `` that fails to load falls back to the original (the legacy gallery - already does). Appearance says when this browser cannot show JPEG XL thumbnails. + already does). A video whose preview it cannot show keeps its own element (Images) or + its "Video preview" label (Library), never a broken image. History's cover URL names + no format, so the server serves a `.jxl` preview only when the request's `Accept` + lists `image/jxl`; otherwise the file's preview in another format, else the original + image or a video's placeholder. Appearance says when this browser cannot show JPEG XL thumbnails. Overview (`system-overview.mjs`) reads `GET /prompt_manager/system/overview` once per visit through `latestOnly()` on the section scope; a late answer after the section left diff --git a/__init__.py b/__init__.py index 57f4cec4..e22e7451 100644 --- a/__init__.py +++ b/__init__.py @@ -87,6 +87,15 @@ try: routes = config.routes api = PromptManagerAPI() api.add_routes(routes) + # Temp files of preview writes a crash interrupted (#317), off the + # import path: the walk may cover a large thumbnails folder. + import threading + + threading.Thread( + target=api.sweep_thumbnail_temps, + name="PromptManagerPreviewTempSweep", + daemon=True, + ).start() host_app = getattr(config.server_instance, "app", None) if host_app is not None and hasattr(host_app, "on_cleanup"): host_app.on_cleanup.append(api.cleanup_history_reads) diff --git a/py/api/history.py b/py/api/history.py index 8702f1a5..df258153 100644 --- a/py/api/history.py +++ b/py/api/history.py @@ -20,6 +20,7 @@ from .history_current import ( _OpenedCurrentMedia, _current_media_response, _prepare_current_media, + accepts_jxl, _reauthorize_current_target, candidate_locator, resolve_current_candidate, @@ -794,6 +795,7 @@ class HistoryRoutesMixin: media_options = { "ensure_preview": lambda *preview: needed.append(preview), "placeholder": self._video_placeholder_bytes(), + "accepts_jxl": accepts_jxl(request), } prepared = await execute() if needed: diff --git a/py/api/history_current.py b/py/api/history_current.py index 2765d2e6..bde14c24 100644 --- a/py/api/history_current.py +++ b/py/api/history_current.py @@ -17,7 +17,7 @@ from aiohttp import web from . import thumbnail_settings from .images import _canonical, _public_path, SERVABLE_EXTENSIONS, VIDEO_EXTENSIONS -from .images import media_thumbnail_rel +from .images import existing_media_thumbnail, media_thumbnail_rel try: from ...utils.execution_outputs import ( @@ -241,14 +241,19 @@ def resolve_current_candidate( def _open_current_target( - target: Path, namespace: Path, scope: CurrentOutputScope + target: Path, namespace: Path, scope: CurrentOutputScope, preview: bool = False ) -> BinaryIO: - """Open only the canonical target and verify authority against the actual handle.""" + """Open only the canonical target and verify authority against the actual handle. + + A *preview* (a file under ``thumbnails/``) may also be JPEG XL (#317).""" if _canonical(target) != target: raise CurrentLocationUnavailable("not_authorized", stale=True) if not _authorized(target, namespace, scope): raise CurrentLocationUnavailable("not_authorized") - if target.suffix.lower() not in SERVABLE_EXTENSIONS: + suffix = target.suffix.lower() + if suffix not in SERVABLE_EXTENSIONS and not ( + preview and suffix in thumbnail_settings.THUMBNAIL_EXTENSIONS + ): raise CurrentLocationUnavailable("unsupported_media") flags = ( os.O_RDONLY @@ -321,6 +326,61 @@ def _preview_writable(root: Path, preview: Path) -> bool: ).is_relative_to(_canonical(folder)) +def _is_jxl(path) -> bool: + return Path(path).suffix.lower() == ".jxl" + + +def _preview_without_jxl(root: Path, relative: Path) -> Path | None: + """The first existing preview *relative* owns that is not JPEG XL.""" + return next( + ( + path + for path in thumbnail_settings.owned_thumbnails(root, relative) + if not _is_jxl(path) + ), + None, + ) + + +def accepts_jxl(request) -> bool: + """Whether the request's ``Accept`` lists ``image/jxl``.""" + accept = request.headers.get("Accept", "") + return any( + part.split(";", 1)[0].strip().lower() == "image/jxl" + for part in accept.split(",") + ) + + +def _placeholder_media(placeholder: bytes) -> "_OpenedCurrentMedia": + return _OpenedCurrentMedia(io.BytesIO(placeholder), len(placeholder), "image/jpeg") + + +def _cover_target(resolved, ensure_preview, placeholder, accepts_jxl): + """What the cover opens: a preview path, the placeholder media, or None + for the original (see ``_prepare_current_media``).""" + target = resolved.canonical_target + relative = target.relative_to(resolved.gallery_root) + is_video = target.suffix.lower() in VIDEO_EXTENSIONS + # #317: any preview the file owns (the current format first); a missing + # one is made under the current format's name. + preview = existing_media_thumbnail(resolved.gallery_root, relative) + if preview is not None and not accepts_jxl and _is_jxl(preview): + preview = _preview_without_jxl(resolved.gallery_root, relative) + if preview is None: + if is_video and placeholder is not None: + return _placeholder_media(placeholder) + return None + return _canonical(preview) + if preview is not None: + return _canonical(preview) + preview = resolved.gallery_root / media_thumbnail_rel(relative) + if ensure_preview is not None and _preview_writable(resolved.gallery_root, preview): + ensure_preview(target, preview, is_video) + if is_video and placeholder is not None: + return _placeholder_media(placeholder) + return None + + def _prepare_current_media( resolved: ResolvedCurrentCandidate, scope: CurrentOutputScope, @@ -328,6 +388,7 @@ def _prepare_current_media( thumbnail: bool, ensure_preview=None, placeholder: bytes | None = None, + accepts_jxl: bool = True, ) -> _OpenedCurrentMedia: """Open the current file, or its preview for ``thumbnail``. @@ -335,27 +396,21 @@ def _prepare_current_media( ``ensure_preview(src, dst, is_video)`` (the route makes it and reads again, #249). Without one an image's original is opened and a video is the ``placeholder`` JPEG, never the video inside an ````. + + The cover's URL names no format, so a JPEG XL preview goes only to a + browser whose ``Accept`` lists ``image/jxl`` (*accepts_jxl*, review M3). + Another one gets the file's preview in another format when there is one, + else the original image or a video's placeholder: never a broken image. """ target = resolved.canonical_target + is_preview = False if thumbnail: - relative = target.relative_to(resolved.gallery_root) - is_video = target.suffix.lower() in VIDEO_EXTENSIONS - # #317: any preview the file owns (the current format first); a - # missing one is made under the current format's name. - preview = thumbnail_settings.existing_thumbnail(resolved.gallery_root, relative) - if preview is None: - preview = resolved.gallery_root / media_thumbnail_rel(relative) - if ensure_preview is not None and _preview_writable( - resolved.gallery_root, preview - ): - ensure_preview(target, preview, is_video) - if is_video and placeholder is not None: - return _OpenedCurrentMedia( - io.BytesIO(placeholder), len(placeholder), "image/jpeg" - ) - else: - target = _canonical(preview) - handle = _open_current_target(target, resolved.namespace, scope) + chosen = _cover_target(resolved, ensure_preview, placeholder, accepts_jxl) + if isinstance(chosen, _OpenedCurrentMedia): + return chosen + if chosen is not None: + target, is_preview = chosen, True + handle = _open_current_target(target, resolved.namespace, scope, is_preview) try: return _OpenedCurrentMedia( handle, diff --git a/py/api/images.py b/py/api/images.py index a980e8a3..4f154491 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -29,6 +29,7 @@ from .thumbnail_encode import ( PreviewWriteHeld, replace_unless_held, run_encoder, + sweep_stray_temps, temp_sibling, thumbnail_frame, writer_settings, @@ -453,7 +454,7 @@ def _preview_state(src, folder, stem, settings, folders): names[key] for key in ( os.path.normcase(stem + extension) - for extension in thumbnail_settings.candidate_extensions( + for extension in thumbnail_settings.owned_extensions( src.suffix, settings.extension ) if extension != settings.extension @@ -798,6 +799,15 @@ def media_thumbnail_rel(rel_path, settings=None): return thumbnail_settings.thumbnail_rel(rel_path, settings.extension) +def existing_media_thumbnail(output_path, rel_path): + """The existing preview *rel_path* owns that a listing or cover shows + (the format written now first, then older ones), or None. The Images + listing and History's cover use this one lookup (review M5).""" + return thumbnail_settings.existing_thumbnail( + output_path, rel_path, writer_settings().extension + ) + + def media_thumbnail(output_path, rel_path, root_index, *, root_scope=None): """``(thumbnail_url, ready)`` for *rel_path* under *output_path* (a few blocking stats). The Images listing, the Needs review queue cards and the @@ -815,17 +825,14 @@ def media_thumbnail(output_path, rel_path, root_index, *, root_scope=None): rel_path = Path(rel_path) if "thumbnails" in rel_path.parts[:-1]: return None, False - settings = writer_settings() - existing = thumbnail_settings.existing_thumbnail( - output_path, rel_path, settings.extension - ) + existing = existing_media_thumbnail(output_path, rel_path) thumbnail_url = None if existing is not None: thumb_rel = thumbnail_settings.thumbnail_stem_rel(rel_path) + existing.suffix thumbnail_url = thumbnail_url_for(thumb_rel, existing) ready = thumbnail_url is not None if thumbnail_url is None: - thumb_rel = media_thumbnail_rel(rel_path, settings) + thumb_rel = media_thumbnail_rel(rel_path) quoted = urllib.parse.quote(thumb_rel, safe="/") thumbnail_url = f"/prompt_manager/images/serve/{quoted}?root={root_index}" else: @@ -2329,7 +2336,8 @@ class ImageRoutesMixin: "-v", "error", "-i", - str(video_path), + # Read as a file, never as a protocol URL (review M6). + f"file:{video_path}", "-ss", "00:00:01", "-vframes", @@ -2426,6 +2434,25 @@ class ImageRoutesMixin: {"success": False, "error": _safe_error(exc)}, status=500 ) + def sweep_thumbnail_temps(self): + """Remove the old temp files crashed preview writes left in every + root's ``thumbnails/`` folder (blocking; run at startup on its own + thread). Returns how many went.""" + removed = 0 + try: + output_dirs = self._get_all_output_dirs() + except Exception as exc: + self.logger.debug(f"No roots to sweep for preview temp files: {exc}") + return 0 + for output_path in output_dirs: + folder = Path(output_path) / "thumbnails" + if folder.is_symlink() or not folder.is_dir(): + continue + removed += sweep_stray_temps(folder)[0] + if removed: + self.logger.info(f"Removed {removed} interrupted preview temp files") + return removed + def _clear_gallery_thumbnails_sync( self, output_dirs: list[Path] ) -> tuple[int, int]: @@ -2453,6 +2480,11 @@ class ImageRoutesMixin: cleared_count = 0 cleared_size = 0 for thumbnails_dir in directories: + # Temp files a crashed write left (review I1): the clear holds + # every preview write, so none is in use. + temps, temp_bytes = sweep_stray_temps(thumbnails_dir, min_age=0) + cleared_count += temps + cleared_size += temp_bytes for root, _dirs, files in os.walk(thumbnails_dir): for name in files: path = Path(root) / name diff --git a/py/api/settings_routes.py b/py/api/settings_routes.py index 97814f07..e3c39a3f 100644 --- a/py/api/settings_routes.py +++ b/py/api/settings_routes.py @@ -27,7 +27,7 @@ from . import thumbnail_encode from . import thumbnail_settings from .admin import _current_image_monitor from .job_routes import busy_response -from .jobs import JobBusy +from .jobs import THUMBNAILS_JOB_KIND, JobBusy try: from ...utils.validators import validate_result_timeout, validate_worker_threads @@ -47,6 +47,8 @@ _SETTINGS_FILE_KEYS = { "THUMBNAIL_QUALITY": ("thumbnails", "quality"), "THUMBNAIL_EDGE": ("thumbnails", "edge"), } +# The thumbnail settings a running Convert outdated job fixes (#317). +_THUMBNAIL_SETTINGS = ("THUMBNAIL_FORMAT", "THUMBNAIL_QUALITY", "THUMBNAIL_EDGE") # Attributes that live on GalleryConfig; the others on PromptManagerConfig. _GALLERY_SETTINGS = frozenset({"MONITORING_ENABLED"}) SOURCE_CHANGE_JOB_KIND = "source_change" @@ -321,6 +323,10 @@ class SettingsRoutesMixin: status=400, ) + refused = self._thumbnail_change_refused(updates) + if refused is not None: + return refused + # Off the loop: an accepted root is resolved (filesystem access). new_roots, error = await self._run_in_executor( self._parse_gallery_roots, data @@ -380,6 +386,33 @@ class SettingsRoutesMixin: status=500, ) + def _thumbnail_change_refused(self, updates): + """409 ``busy`` for a thumbnail format, quality or size change while + Convert outdated runs (review I4): the job converts to the settings + it started with, and a change under it would make it remove previews + in the newly chosen format. Unchanged values pass.""" + from ..config import PromptManagerConfig + + changed = any( + key in updates and updates[key] != getattr(PromptManagerConfig, key) + for key in _THUMBNAIL_SETTINGS + ) + job = self._jobs.active(THUMBNAILS_JOB_KIND) if changed else None + if job is None or job.scope.get("mode") != "convert": + return None + return web.json_response( + { + "success": False, + "code": "busy", + "error": ( + f"{job.title} is running. Change the thumbnail settings " + "when it finishes, or cancel it first." + ), + "running": {"id": job.id, "title": job.title}, + }, + status=409, + ) + def _publish_settings(self, updates, new_roots): """Install committed settings at runtime (inside the config commit). diff --git a/py/api/thumbnail_auto.py b/py/api/thumbnail_auto.py index 6a7e64fd..b1a66664 100644 --- a/py/api/thumbnail_auto.py +++ b/py/api/thumbnail_auto.py @@ -97,9 +97,7 @@ except ImportError: # tests import the package top-level PLACEHOLDER_SIZE = (thumbnail_settings.DEFAULT_EDGE, thumbnail_settings.DEFAULT_EDGE) PLACEHOLDER_QUALITY = 85 # Source extensions in the order a shared preview name looks for them (R2). -_SOURCE_ORDER = (".png", ".jpg", ".jpeg", ".webp", ".gif") + tuple( - sorted(VIDEO_EXTENSIONS) -) +_SOURCE_ORDER = thumbnail_settings.IMAGE_SOURCE_ORDER + tuple(sorted(VIDEO_EXTENSIONS)) # Previews views may write at once (the size of the view pool). ON_DEMAND_LIMIT = 3 # View requests that may wait on the gate; one more gets its fallback at once. diff --git a/py/api/thumbnail_convert.py b/py/api/thumbnail_convert.py index c8c34dbc..f83abea4 100644 --- a/py/api/thumbnail_convert.py +++ b/py/api/thumbnail_convert.py @@ -16,14 +16,26 @@ what was converted): 1. the new preview is written from the SOURCE media in the configured format (``_generate_one``: atomic, verified to decode; a video whose frame cannot be decoded fails rather than getting the placeholder), unless a - current preview at least as new as the source is already there; + current preview at least as new as the source is already there and + decodes; 2. only then are that file's old-format previews removed: regular files (no links) in its ``thumbnails/`` folder, owned by that source - (``thumbnail_settings.owns``), with the source still present. + (``thumbnail_settings.owns``), with the source still present. One already + gone counts as removed. + +Files sharing a stem in one folder (``x.png`` and ``x.mp4``) share one +preview (ruling R2), so they are converted once, as a group: the preview is +made from the first source in ``thumbnail_settings.source_rank`` order +(``.png``, ``.jpg``, ``.jpeg``, ``.webp``, ``.gif``, then videos), the old +previews of every source in the group are removed, and each removed or +written file's bytes count once. A failure keeps the old preview. Previews whose source is gone (orphans) are -never planned, so never touched. The final status line reads -"N converted · F failed · → ". +never planned, so never touched. The format the job converts to is fixed +when it starts; Appearance refuses a thumbnail settings change while it runs +(409), and should the format still change underneath, the job stops at the +next file without removing anything in another format. The final status line +reads "N converted · F failed · → ". """ import asyncio @@ -33,6 +45,7 @@ import time from pathlib import Path from . import thumbnail_settings +from .thumbnail_encode import ThumbnailVerifyError, verify_thumbnail, writer_settings from .admin import _plural, _scan_location, _scan_roots, _scan_status_line from .images import _thumbnail_is_fresh, _worker_threads, _thumbnail_batch_size from .prompts import safe_error_message @@ -47,6 +60,10 @@ NOTHING_OUTDATED = "No outdated previews to convert" CANCELLED_BEFORE_CONVERTING = "Cancelled before any preview was converted" SOURCE_GONE = "The file is gone; its old preview was kept" OLD_NOT_REMOVED = "Converted; the old preview could not be removed" +FORMAT_CHANGED = ( + "Stopped: the thumbnail format changed while converting · " + "{converted} converted. Run Convert outdated again." +) _UNITS = ("B", "KB", "MB", "GB", "TB") @@ -76,6 +93,28 @@ def _old_previews(src, dst, settings): return old +def _group_targets(targets): + """``[[(src, dst, is_video)]]``: targets sharing a preview name (one + folder, one stem) in one group, its first source first (R2).""" + groups = {} + for target in targets: + dst = Path(target[1]) + key = os.path.normcase(str(dst.with_suffix(""))) + groups.setdefault(key, []).append(target) + ordered = [] + for group in groups.values(): + group.sort( + key=lambda t: (thumbnail_settings.source_rank(Path(t[0]).suffix), str(t[0])) + ) + ordered.append(group) + return ordered + + +def _format_changed(settings): + """Whether previews are written in another format than *settings* now.""" + return writer_settings().extension != settings.extension + + def _finished_line(totals, remaining): line = ( f"{totals['converted']:,} converted · {totals['failed']:,} failed · " @@ -128,6 +167,13 @@ class ThumbnailConvertMixin: return "finished" totals = await self._convert_targets(job, options, targets, output_dirs) self.invalidate_gallery_cache() + if totals["stopped"]: + job.update(status_line=FORMAT_CHANGED.format(converted=totals["converted"])) + job.log_line( + "Conversion stopped · the thumbnail format changed · " + f"{_plural(totals['converted'], 'preview')} converted" + ) + return "cancelled" if job.cancelled: return self._end_conversion( job, @@ -149,27 +195,33 @@ class ThumbnailConvertMixin: return "cancelled" async def _convert_targets(self, job, options, targets, output_dirs): - """Convert *targets* in worker batches, recording each file.""" + """Convert *targets* in worker batches of preview groups, recording + each file.""" roots = _scan_roots(output_dirs) total = len(targets) + groups = _group_targets(targets) job.update(total=total, status_line=_scan_status_line(0, total, 0, 0)) totals = {"converted": 0, "failed": 0, "before": 0, "after": 0} + totals["stopped"] = False start = time.monotonic() batch_size = _thumbnail_batch_size(_worker_threads()) index = batches = 0 - for batch_start in range(0, total, batch_size): - if job.cancelled: + for batch_start in range(0, len(groups), batch_size): + if job.cancelled or totals["stopped"]: break - batch = targets[batch_start : batch_start + batch_size] + batch = groups[batch_start : batch_start + batch_size] results = await self._run_in_executor( self._convert_batch, batch, options.settings, lambda: job.cancelled ) - for (src, _dst, _video), result in zip(batch, results): - if result["action"] == "cancelled": - continue - index += 1 - self._record_conversion(job, src, result, roots, totals) - job.update(processed=index, progress=index * 100 // total) + for group, group_results in zip(batch, results): + for (src, _dst, _video), result in zip(group, group_results): + if result["action"] == "stopped": + totals["stopped"] = True + if result["action"] in ("cancelled", "stopped"): + continue + index += 1 + self._record_conversion(job, src, result, roots, totals) + job.update(processed=index, progress=index * 100 // total) batches += 1 job.update( status_line=_scan_status_line( @@ -195,27 +247,44 @@ class ThumbnailConvertMixin: detail = result.get("error") or "failed" job.record("errors", {**row, "detail": detail}, f"! {where} · {detail}") - def _convert_batch(self, targets, settings, stop): - """Convert ``targets`` on the configured worker threads (blocking).""" - convert = functools.partial(self._convert_target, settings=settings, stop=stop) - return map_parallel(convert, targets, _worker_threads()) + def _convert_batch(self, groups, settings, stop): + """Convert preview ``groups`` on the configured worker threads + (blocking); one result list per group.""" + convert = functools.partial(self._convert_group, settings=settings, stop=stop) + return map_parallel(convert, groups, _worker_threads()) - def _convert_target(self, target, settings, stop): - src, dst, is_video = target - return self._convert_one(src, dst, is_video, settings, stop=stop) + def _convert_group(self, group, settings, stop): + """Convert the preview a group of same-stem files shares, once. - def _convert_one(self, src, dst, is_video, settings, stop=None): + The first source writes it; the others are recorded with the same + outcome and no bytes of their own.""" + src, dst, is_video = group[0] + siblings = [Path(target[0]) for target in group[1:]] + result = self._convert_one( + src, dst, is_video, settings, stop=stop, siblings=siblings + ) + if result["action"] != "converted": + return [result] * len(group) + shared = {**result, "before": 0, "after": 0} + shared["detail"] = f"{result['detail']} (shares {Path(src).name}'s preview)" + return [result] + [shared] * len(siblings) + + def _convert_one(self, src, dst, is_video, settings, stop=None, siblings=()): """Convert one file's preview (blocking; never raises). - ``converted`` with the bytes before and after, ``error`` (the old - preview kept) or ``cancelled`` (a file boundary). + ``converted`` with the bytes removed and written, ``error`` (the old + preview kept), ``cancelled`` (a file boundary) or ``stopped`` (the + format changed underneath, review I4). *siblings* share the preview + (R2): their old previews go too. """ if stop is not None and stop(): return {"action": "cancelled"} + if _format_changed(settings): + return {"action": "stopped"} dst = Path(dst) try: - old = _old_previews(src, dst, settings) - if not _thumbnail_is_fresh(src, dst): + old = self._old_group_previews([Path(src), *siblings], dst, settings) + if not self._current_preview_usable(src, dst, settings): made = self._generate_one( src, dst, settings, is_video, rebuild=True, placeholder=False ) @@ -226,22 +295,57 @@ class ThumbnailConvertMixin: return {"action": "error", "error": "The new preview was not written"} if not os.path.isfile(src): return {"action": "error", "error": SOURCE_GONE} - kept = [] - for path, _size in old: - try: - os.remove(path) - except OSError as exc: - self.logger.warning(f"Could not remove old preview {path}: {exc}") - kept.append(path) + if _format_changed(settings): + return {"action": "stopped"} + removed, kept = self._remove_old_previews(old) if kept: return {"action": "error", "error": OLD_NOT_REMOVED} label = thumbnail_settings.FORMAT_LABELS[settings.format] return { "action": "converted", - "before": sum(size for _path, size in old), + "before": removed, "after": dst.stat().st_size, "detail": f"Converted to {label}", } except Exception as exc: self.logger.warning(f"Conversion failed for {src.name}", exc_info=True) return {"action": "error", "error": safe_error_message(exc)} + + @staticmethod + def _old_group_previews(sources, dst, settings): + """``[(path, size)]`` of every source's old previews, each path once.""" + seen, old = set(), [] + for source in sources: + for path, size in _old_previews(source, dst, settings): + key = os.path.normcase(str(path)) + if key not in seen: + seen.add(key) + old.append((path, size)) + return old + + @staticmethod + def _current_preview_usable(src, dst, settings): + """A current preview at least as new as *src* that decodes (M4).""" + if not _thumbnail_is_fresh(src, dst): + return False + try: + verify_thumbnail(dst, settings.format) + except ThumbnailVerifyError: + return False + return True + + def _remove_old_previews(self, old): + """``(bytes removed, [kept paths])``; a file already gone counts as + removed, with no bytes (another run or a sibling removed it).""" + removed, kept = 0, [] + for path, size in old: + try: + os.remove(path) + except FileNotFoundError: + continue + except OSError as exc: + self.logger.warning(f"Could not remove old preview {path}: {exc}") + kept.append(path) + continue + removed += size + return removed, kept diff --git a/py/api/thumbnail_encode.py b/py/api/thumbnail_encode.py index 6ecfa22c..ac90033e 100644 --- a/py/api/thumbnail_encode.py +++ b/py/api/thumbnail_encode.py @@ -20,8 +20,10 @@ Quality maps to a JPEG XL distance with libjxl's own (q80 → d1.9, q85 → d1.45, q90 → d1.0), the quadratic below 30, and q100 → d0.1 (a preview is never lossless). Pillow's plugin applies the same mapping inside libjxl. Detection is cached (``detect_jxl_encoder(refresh=True)`` -probes again). If JPEG XL is configured but no encoder is left at write -time, that file is written as WebP and the fallback is logged once. +probes again). An encoder that fails (its binary gone, or a non-zero exit) +makes one write detect again: another encoder found is used for that file, +none at all writes it as WebP (logged once) and refreshes what Appearance +reads (``encoder_status``). The same encoder found again raises. Encoder processes (and ffmpeg's video frames, images.py) go through ``run_encoder``: tracked, bounded by a timeout, and killed by @@ -31,9 +33,11 @@ Encoder processes (and ffmpeg's video frames, images.py) go through import importlib import io import os +import re import shutil import subprocess # nosec B404 - fixed argv lists, never a shell import threading +import time from dataclasses import replace from pathlib import Path @@ -100,6 +104,46 @@ def replace_unless_held(tmp, dst, guard=None): os.replace(tmp, dst) +# ``temp_sibling``'s names: ``..--``. +_TEMP_NAME = re.compile(r"^\..+\.(?:part|src)-(\d+)-\d+\.[A-Za-z0-9]+$") +# A temp at least this old (seconds) outlived its write: no encode runs longer. +STRAY_TEMP_AGE = 4 * ENCODE_TIMEOUT + + +def sweep_stray_temps(folder, min_age=None): + """Remove the temp files a crashed write left under *folder* (a preview + folder, walked without following links); returns ``(count, bytes)``. + + Only ``temp_sibling`` names are touched, never a link. With *min_age* + (seconds, the startup sweep) a temp younger than that, or one this + process may still be writing, is left; ``min_age=0`` (the clear, which + holds every preview write) removes them all. + """ + min_age = STRAY_TEMP_AGE if min_age is None else min_age + now = time.time() + own = str(os.getpid()) + count = size = 0 + for root, _dirs, files in os.walk(folder): + for name in files: + match = _TEMP_NAME.match(name) + if match is None: + continue + path = Path(root) / name + try: + stat = path.lstat() + if path.is_symlink() or not path.is_file(): + continue + if min_age and (match.group(1) == own or now - stat.st_mtime < min_age): + continue + path.unlink() + except OSError as exc: + logger.debug(f"Could not remove stray temp {name}: {exc}") + continue + count += 1 + size += stat.st_size + return count, size + + def _unlink_quietly(path): try: Path(path).unlink() @@ -318,8 +362,29 @@ def _check(done, tmp, name): raise EncoderFailed(f"{name} could not encode the preview: {detail.strip()}") +class _EncoderGone(RuntimeError): + """No JPEG XL encoder is left: this file is written as WebP.""" + + def _write_jxl(img, tmp, settings, track): + """Encode with the detected encoder; one that fails makes the write + detect again once (another one is used, none raises _EncoderGone).""" encoder = detect_jxl_encoder() + try: + _encode_jxl(encoder, img, tmp, settings, track) + return + except (OSError, EncoderFailed) as exc: + again = detect_jxl_encoder(refresh=True) + if again is None: + raise _EncoderGone(str(exc)) from exc + if again == encoder: + raise + logger.info(f"JPEG XL encoder {encoder} failed; using {again}") + _unlink_quietly(tmp) + _encode_jxl(again, img, tmp, settings, track) + + +def _encode_jxl(encoder, img, tmp, settings, track): distance = f"{jxl_distance(settings.quality):.2f}" if encoder == "pillow": img.save( @@ -334,7 +399,8 @@ def _write_jxl(img, tmp, settings, track): cmd = [ "ffmpeg", "-v", "error", "-f", "png_pipe", "-i", "-", "-frames:v", "1", "-c:v", "libjxl", "-distance", distance, - "-effort", str(JXL_EFFORT), "-f", "image2", "-y", str(tmp), + "-effort", str(JXL_EFFORT), "-f", "image2", "-update", "1", + "-y", str(tmp), ] # fmt: skip _check(run_encoder(cmd, input=_png_bytes(img), track=track), tmp, "ffmpeg") return @@ -407,6 +473,15 @@ def write_thumbnail(frame, dst, settings, *, guard=None, track=None): if fmt == "jxl" and detect_jxl_encoder() is None: _jxl_fallback() fmt, dst = "webp", dst.with_suffix(".webp") + try: + return _write_verified(frame, dst, fmt, settings, guard, track) + except _EncoderGone: + _jxl_fallback() + dst = dst.with_suffix(".webp") + return _write_verified(frame, dst, "webp", settings, guard, track) + + +def _write_verified(frame, dst, fmt, settings, guard, track): tmp = temp_sibling(dst) try: _WRITERS[fmt](frame, tmp, settings, track) diff --git a/py/api/thumbnail_settings.py b/py/api/thumbnail_settings.py index b9687cb6..c6bbc5d3 100644 --- a/py/api/thumbnail_settings.py +++ b/py/api/thumbnail_settings.py @@ -124,6 +124,19 @@ def thumbnail_rel(rel_path, extension=None): return thumbnail_stem_rel(rel_path) + (extension or current().extension) +# Ruling R2: files sharing a stem share one preview, made from the first +# source in this order: these images, then videos (by extension name). +IMAGE_SOURCE_ORDER = (".png", ".jpg", ".jpeg", ".webp", ".gif") + + +def source_rank(source_extension): + """Sort key of a source extension in R2's order (images first).""" + extension = source_extension.lower() + if extension in IMAGE_SOURCE_ORDER: + return (0, IMAGE_SOURCE_ORDER.index(extension), extension) + return (1, 0, extension) + + def owns(thumb_extension, source_extension): """Whether a ``_thumb`` preview can belong to a source with *source_extension* (ruling R4).""" @@ -154,6 +167,18 @@ def candidate_extensions(source_extension, preferred=None): return tuple(seen) +def owned_extensions(source_extension, preferred=None): + """``candidate_extensions`` plus the other spellings an older release + may have given a legacy preview (review M1): the source's own + (``X_thumb.JPG`` for ``X.JPG``) and the upper-case legacy one.""" + extensions = list(candidate_extensions(source_extension, preferred)) + legacy = _legacy_extension(source_extension) + for spelling in (source_extension, legacy.upper()): + if spelling not in extensions and owns(spelling, source_extension): + extensions.append(spelling) + return tuple(extensions) + + def _is_preview_file(path): try: return not path.is_symlink() and path.is_file() @@ -165,14 +190,8 @@ def owned_in_folder(folder, stem, source_extension, preferred=None): """Yield the existing previews ``/`` a source with *source_extension* owns, in lookup order (regular files, no links; one file found under two spellings on a case-insensitive disk once).""" - extensions = list(candidate_extensions(source_extension, preferred)) - if source_extension != source_extension.lower() and owns( - source_extension, source_extension - ): - # An older release kept the source's own spelling (``b_thumb.JPG``). - extensions.append(source_extension) seen = set() - for extension in extensions: + for extension in owned_extensions(source_extension, preferred): path = Path(folder) / (stem + extension) if not _is_preview_file(path): continue diff --git a/tests/js/workspace-image-card-media.test.js b/tests/js/workspace-image-card-media.test.js index 32b3098c..671bb763 100644 --- a/tests/js/workspace-image-card-media.test.js +++ b/tests/js/workspace-image-card-media.test.js @@ -56,3 +56,16 @@ test('a video whose preview fails still swaps back to its own element', () => wi assert.equal(holder.children[0].tagName, 'video'); assert.equal(holder.children[0].src, 'http://host/serve/c.mp4?root=0'); })); + +test('a video with a JPEG XL thumbnail keeps its own element where JPEG XL cannot be shown (review M2)', async () => { + support.resetJxlSupport(); + await support.detectJxlSupport({ createImage: () => ({ set src(_) { queueMicrotask(() => this.onerror()); } }) }); + const dom = mountedDom(); + dom.win.ApiClient = { resolveUrl: url => `http://host${url}` }; + try { + const node = cardMedia(image('/serve/thumbnails/c_thumb.jxl?root=0', + { path: 'c.mp4', filename: 'c.mp4', url: '/serve/c.mp4?root=0', is_video: true })); + assert.equal(node.tagName, 'video'); + assert.equal(node.src, 'http://host/serve/c.mp4?root=0'); + } finally { support.resetJxlSupport(); dom.restore(); } +}); diff --git a/tests/js/workspace-thumbnail-support.test.js b/tests/js/workspace-thumbnail-support.test.js index 465883b1..4338d13d 100644 --- a/tests/js/workspace-thumbnail-support.test.js +++ b/tests/js/workspace-thumbnail-support.test.js @@ -72,3 +72,30 @@ test('a thumbnail that fails to load falls back to the original once', () => { listen[0][2](); assert.equal(scoped.src, '/serve/b.png'); }); + +test('a video card shows its thumbnail only when this browser can show it (review M2)', () => { + const video = { is_video: true, url: '/serve/c.mp4?root=0', thumbnail_url: '/serve/thumbnails/c_thumb.jxl?root=0' }; + assert.equal(support.showsThumbnail(video, false), false, 'JPEG XL unsupported: the video keeps its own element'); + assert.equal(support.showsThumbnail(video, true), true); + assert.equal(support.showsThumbnail(video, undefined), true, 'unknown: try it, the error swaps'); + assert.equal(support.showsThumbnail({ ...video, thumbnail_url: '/serve/thumbnails/c_thumb.webp' }, false), true); + assert.equal(support.showsThumbnail({ ...video, thumbnail_url: null }, true), false); + assert.equal(support.showsThumbnail({ url: '/serve/a.png', thumbnail_url: '/serve/thumbnails/a_thumb.jxl' }, false), true, + 'an image always shows something: its original'); +}); + +test('a video thumbnail that fails to load is replaced once, never left broken', () => { + const listeners = {}; + let replaced = null; + const node = { addEventListener(type, handler, options) { listeners[type] = { handler, options }; }, + replaceWith(next) { replaced = next; } }; + support.replaceOnError(node, () => 'label'); + assert.equal(listeners.error.options.once, true); + listeners.error.handler(); + assert.equal(replaced, 'label'); + const listen = []; + support.replaceOnError(node, () => 'other', (...args) => listen.push(args)); + assert.equal(listen[0][1], 'error'); + listen[0][2](); + assert.equal(replaced, 'other'); +}); diff --git a/tests/test_api_admin.py b/tests/test_api_admin.py index e1744bac..525faf70 100644 --- a/tests/test_api_admin.py +++ b/tests/test_api_admin.py @@ -1424,6 +1424,71 @@ class TestThumbnailSettings(AdminAPITestCase): self.assertEqual(self._read_config()["thumbnails"]["format"], "jxl") self.assertEqual(PromptManagerConfig.THUMBNAIL_FORMAT, "jxl") + async def _running_job(self, mode): + from py.api.jobs import THUMBNAILS_JOB_KIND + from py.api.thumbnail_convert import CONVERT_JOB_TITLE + + gate = asyncio.Event() + + async def runner(job): + await gate.wait() + job.finish("finished") + + job = self.api._jobs.start( + THUMBNAILS_JOB_KIND, + CONVERT_JOB_TITLE if mode == "convert" else "Thumbnail generation", + admission="guarded", + scope={"mode": mode}, + runner=runner, + ) + + async def release(): + gate.set() + await asyncio.wait_for(job.task, 5) + + self.addAsyncCleanup(release) + return job + + async def test_thumbnail_changes_wait_for_a_running_conversion(self): + # Review I4: a convert job's format is fixed; changing the format, + # quality or size limit under it is refused with the job named. + PromptManagerConfig.update_config( + {"thumbnails": {"format": "webp", "quality": 80, "edge": 512}} + ) + job = await self._running_job("convert") + for change in ( + {"thumbnail_format": "jpeg"}, + {"thumbnail_quality": 70}, + {"thumbnail_edge": 300}, + ): + resp = await self.client.request("POST", self.URL, json=change) + body = await resp.json() + self.assertEqual(resp.status, 409, change) + self.assertEqual(body["code"], "busy") + self.assertEqual(body["running"]["id"], job.id) + self.assertIn("Convert outdated thumbnails", body["error"]) + self.assertIn("thumbnail", body["error"].lower()) + self.assertEqual( + ( + PromptManagerConfig.THUMBNAIL_FORMAT, + PromptManagerConfig.THUMBNAIL_QUALITY, + PromptManagerConfig.THUMBNAIL_EDGE, + ), + ("webp", 80, 512), + ) + # The same values, or other settings, still save. + resp = await self.client.request( + "POST", self.URL, json={"thumbnail_format": "webp", "thumbnail_quality": 80} + ) + self.assertEqual(resp.status, 200) + + async def test_a_missing_previews_job_does_not_block_thumbnail_changes(self): + await self._running_job("missing") + resp = await self.client.request( + "POST", self.URL, json={"thumbnail_format": "jpeg"} + ) + self.assertEqual(resp.status, 200) + class TestDiagnostics(AdminAPITestCase): diff --git a/tests/test_api_history_outputs.py b/tests/test_api_history_outputs.py index 9a4644fe..6392b02c 100644 --- a/tests/test_api_history_outputs.py +++ b/tests/test_api_history_outputs.py @@ -14,6 +14,7 @@ from unittest import skipUnless from unittest.mock import patch from aiohttp import web +from PIL import Image from aiohttp.test_utils import make_mocked_request from tests.test_api_images import ImageAPITestCase, make_png @@ -1174,6 +1175,75 @@ class HistoryOutputAPITests(ImageAPITestCase): await asyncio.wait_for(job.task, 5) self.assertFalse((self.output_dir / "thumbnails").exists()) + async def test_the_cover_is_the_preview_the_images_listing_shows(self): + # Review M5: JPEG XL configured with no encoder left writes WebP; the + # history cover prefers that WebP, like the Images listing. + from py.api import thumbnail_encode + from py.config import PromptManagerConfig + + make_png(self.output_dir / "image.png") + folder = self.output_dir / "thumbnails" + folder.mkdir() + (folder / "image_thumb.jxl").write_bytes(b"\xff\x0a stale jxl") + webp = folder / "image_thumb.webp" + Image.new("RGB", (16, 16)).save(webp, "WEBP") + start = self.capture([self.ref()]) + descriptor = await self.media(start) + with ( + patch.object(PromptManagerConfig, "THUMBNAIL_FORMAT", "jxl"), + patch.object(thumbnail_encode, "detect_jxl_encoder", return_value=None), + ): + response = await self.current(descriptor["thumbnail_url"]) + body = await response.read() + self.assertEqual(response.headers["Content-Type"], "image/webp") + self.assertEqual(body, webp.read_bytes()) + + async def _jxl_cover(self, filename, accept): + from py.api import thumbnail_encode + from py.config import PromptManagerConfig + + folder = self.output_dir / "thumbnails" + folder.mkdir(exist_ok=True) + stem = filename.rsplit(".", 1)[0] + jxl = folder / f"{stem}_thumb.jxl" + jxl.write_bytes(b"\xff\x0a jxl preview") + start = self.capture([self.ref(filename=filename)]) + descriptor = await self.media(start) + headers = {"Accept": accept} if accept else {} + with ( + patch.object(PromptManagerConfig, "THUMBNAIL_FORMAT", "jxl"), + patch.object(thumbnail_encode, "detect_jxl_encoder", return_value="cjxl"), + ): + response = await self.current(descriptor["thumbnail_url"], headers=headers) + body = await response.read() + return response, body, jxl + + async def test_a_jxl_cover_is_served_only_to_a_browser_that_accepts_it(self): + # Review M3: History's cover URL names no format, so the server picks: + # a browser that does not list image/jxl gets the original image. + original = make_png(self.output_dir / "image.png") + response, body, jxl = await self._jxl_cover( + "image.png", "image/avif,image/webp,image/*,*/*;q=0.8" + ) + self.assertEqual(response.status, 200) + self.assertEqual(body, original.read_bytes()) + self.assertEqual(response.headers["Content-Type"], "image/png") + self.assertTrue(jxl.exists()) + + async def test_a_browser_that_lists_image_jxl_gets_the_jxl_cover(self): + make_png(self.output_dir / "image.png") + response, body, jxl = await self._jxl_cover( + "image.png", "image/webp,image/avif,image/jxl,image/*;q=0.8" + ) + self.assertEqual(response.headers["Content-Type"], "image/jxl") + self.assertEqual(body, jxl.read_bytes()) + + async def test_a_video_jxl_cover_is_the_placeholder_without_jxl_support(self): + (self.output_dir / "clip.webm").write_bytes(b"AV1 stream no decoder here") + response, body, _jxl = await self._jxl_cover("clip.webm", "image/*,*/*") + self.assertEqual(response.headers["Content-Type"], "image/jpeg") + self.assertTrue(body.startswith(b"\xff\xd8")) + async def test_a_preview_that_cannot_be_made_serves_the_original(self): broken = self.output_dir / "image.png" broken.write_bytes(b"\x89PNG\r\n\x1a\n not an image") diff --git a/tests/test_thumbnail_convert.py b/tests/test_thumbnail_convert.py index 2bc8777a..0b4124c1 100644 --- a/tests/test_thumbnail_convert.py +++ b/tests/test_thumbnail_convert.py @@ -268,6 +268,99 @@ class TestConvertOutdated(ConvertCase): self.assertEqual(job.counters["errors"], 1) self.assertTrue(old.exists(), "a source gone mid-run makes an orphan") + async def test_same_stem_sources_convert_once_and_count_bytes_once(self): + # Review I2 / R2: x.png and x.jpg share x_thumb.webp. The pair is + # converted once, from the image first in the source order (.png), + # and every removed file's bytes count once. + original = PromptManagerConfig.WORKER_THREADS + PromptManagerConfig.WORKER_THREADS = 2 + self.addCleanup(setattr, PromptManagerConfig, "WORKER_THREADS", original) + from py.api.thumbnail_convert import readable_bytes + + png = self.output_dir / "x.png" + Image.new("RGB", (300, 300), (220, 20, 20)).save(png) + jpg = self.output_dir / "x.jpg" + Image.new("RGB", (300, 300), (20, 20, 220)).save(jpg) + old_png = big_png(self._thumb_path("x", ".png"), size=(200, 200)) + old_jpg = self._thumb_path("x", ".jpg") + Image.effect_noise((200, 200), 40).convert("RGB").save(old_jpg, "JPEG") + later = max(png.stat().st_mtime, jpg.stat().st_mtime) + 5 + for path in (old_png, old_jpg): + os.utime(path, (later, later)) + before = old_png.stat().st_size + old_jpg.stat().st_size + + _, job = await self._convert() + + self.assertEqual(job.state, "finished") + self.assertEqual( + (job.counters["generated"], job.counters["errors"]), (2, 0), job.log + ) + new = self._thumb_path("x") + self.assertFalse(old_png.exists() or old_jpg.exists()) + with Image.open(new) as img: + red, _green, blue = img.convert("RGB").getpixel((5, 5)) + self.assertGreater(red, blue, "the image first in the source order wins") + self.assertEqual( + job.status_line, + f"2 converted · 0 failed · {readable_bytes(before)} → " + f"{readable_bytes(new.stat().st_size)}", + ) + + async def test_an_old_preview_already_gone_counts_as_removed(self): + old = self._legacy("g") + real = self.api._generate_one + + def remove_first(src, *args, **kwargs): + os.remove(old) + return real(src, *args, **kwargs) + + self.api._generate_one = remove_first + _, job = await self._convert() + self.assertEqual((job.counters["generated"], job.counters["errors"]), (1, 0)) + self.assertRegex(job.status_line, r"^1 converted · 0 failed · 0 B → ") + + async def test_a_current_preview_that_does_not_decode_is_written_again(self): + # Review M4: a kept current preview is decoded before old ones go. + old = self._legacy("m") + current = self._thumb_path("m") + current.write_bytes(b"RIFF not a webp") + later = old.stat().st_mtime + 5 + os.utime(current, (later, later)) + _, job = await self._convert() + self.assertEqual((job.counters["generated"], job.counters["errors"]), (1, 0)) + with Image.open(current) as img: + self.assertEqual(img.format, "WEBP") + self.assertFalse(old.exists()) + + async def test_a_format_change_underneath_stops_the_job(self): + # Review I4 (defense in depth): the job never removes previews in a + # format chosen after it started. + original = PromptManagerConfig.WORKER_THREADS + PromptManagerConfig.WORKER_THREADS = 1 + self.addCleanup(setattr, PromptManagerConfig, "WORKER_THREADS", original) + olds = [self._legacy(f"f{i}") for i in range(4)] + real = self.api._convert_one + calls = [] + + def switching(*args, **kwargs): + result = real(*args, **kwargs) + calls.append(result) + if len(calls) == 1: + PromptManagerConfig.update_config( + {"thumbnails": {"format": "jpeg", "quality": 80, "edge": 512}} + ) + jpeg = self._thumb_path("f3", ".jpg") + Image.new("RGB", (16, 16)).save(jpeg, "JPEG") + return result + + self.api._convert_one = switching + _, job = await self._convert() + self.assertEqual(job.state, "cancelled") + self.assertIn("format changed", job.status_line) + self.assertEqual(job.counters["generated"], 1) + self.assertTrue(self._thumb_path("f3", ".jpg").exists(), "the new format stays") + self.assertEqual(sum(1 for p in olds if p.exists()), 3) + def test_byte_sizes_are_readable(self): from py.api.thumbnail_convert import readable_bytes diff --git a/tests/test_thumbnail_encode.py b/tests/test_thumbnail_encode.py index ed018c50..330d83cb 100644 --- a/tests/test_thumbnail_encode.py +++ b/tests/test_thumbnail_encode.py @@ -277,6 +277,161 @@ class TestWrite(TempCase): te.verify_thumbnail(path, "jxl") +class TestFfmpegNames(TempCase): + """Review I1: ffmpeg's image2 muxer reads ``%d`` in an output name as a + sequence pattern; ``-update 1`` keeps the temp name literal.""" + + def test_the_ffmpeg_command_writes_one_literal_file(self): + seen = [] + + def fake(cmd, **kwargs): + seen.append(cmd) + Path(cmd[-1]).write_bytes(JXL_CODESTREAM + b"\x00" * 8) + return subprocess.CompletedProcess(cmd, 0, b"", b"") + + with ( + patch.object(te, "detect_jxl_encoder", return_value="ffmpeg"), + patch.object(te, "run_encoder", side_effect=fake), + patch.object(te, "_pillow_decodes_jxl", return_value=False), + ): + te.write_thumbnail( + Image.new("RGB", (8, 8)), + self.tmp / "a_thumb.jxl", + ThumbnailSettings("jxl", 80, 64), + ) + cmd = seen[0] + self.assertEqual(cmd[cmd.index("-update") + 1], "1") + self.assertLess(cmd.index("-update"), len(cmd) - 1) + + @unittest.skipUnless(_ffmpeg_has_libjxl(), "ffmpeg without libjxl") + def test_a_percent_in_the_name_is_written_under_that_name(self): + dst = self.tmp / "50%done_thumb.jxl" + with patch.object(te, "detect_jxl_encoder", return_value="ffmpeg"): + written = te.write_thumbnail( + Image.new("RGB", (32, 32), (10, 200, 10)), + dst, + ThumbnailSettings("jxl", 80, 64), + ) + self.assertEqual(written, dst) + self.assertTrue(dst.is_file()) + self.assertEqual(self._leftovers(), [], "no stray sequence-named temp") + + +class TestStrayTemps(TempCase): + """Review I1: a temp left by a crash (or the old ``%`` bug) is swept.""" + + def _touch(self, name, age=3600): + path = self.tmp / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b"x" * 10) + when = path.stat().st_mtime - age + os.utime(path, (when, when)) + return path + + def test_only_old_temp_siblings_are_removed(self): + strays = [ + self._touch(".a_thumb.part-12-34.webp"), + self._touch(".501one.part-1-2.jxl"), + self._touch("sub/.b_thumb.src-5-6.png"), + ] + kept = [ + self._touch("a_thumb.webp"), + self._touch(".hidden"), + self._touch("notes.part-1-2.webp"), + self._touch(".c_thumb.part-x-2.webp"), + self._touch(".young_thumb.part-7-8.webp", age=0), + ] + own = self._touch(f".mine_thumb.part-{os.getpid()}-9.webp") + count, size = te.sweep_stray_temps(self.tmp, min_age=60) + self.assertEqual((count, size), (3, 30)) + for path in strays: + self.assertFalse(path.exists(), path.name) + for path in kept + [own]: + self.assertTrue(path.exists(), path.name) + + def test_without_an_age_every_temp_goes_but_links_stay(self): + stray = self._touch(".a_thumb.part-1-2.webp", age=0) + own = self._touch(f".mine_thumb.part-{os.getpid()}-9.webp", age=0) + target = self._touch("target.bin") + link = self.tmp / ".l_thumb.part-1-2.webp" + try: + link.symlink_to(target) + except (OSError, NotImplementedError): + link = None + self.assertEqual(te.sweep_stray_temps(self.tmp, min_age=0)[0], 2) + self.assertFalse(stray.exists() or own.exists()) + if link is not None: + self.assertTrue(link.is_symlink()) + + def test_a_missing_folder_sweeps_nothing(self): + self.assertEqual(te.sweep_stray_temps(self.tmp / "none"), (0, 0)) + + +class TestEncoderVanishes(TempCase): + """Review I3: an encoder that disappears after detection is detected + again once; with none left, that file is written as WebP and the + availability Appearance reads is refreshed.""" + + def test_a_missing_binary_falls_back_to_webp_and_refreshes(self): + found = iter(["cjxl", None]) + with ( + patch.object(te, "_detect", side_effect=lambda: next(found)), + patch.object(te, "run_encoder", side_effect=FileNotFoundError("cjxl")), + self.assertLogs("prompt_manager.thumbnails", logging.WARNING), + ): + self.assertTrue(te.encoder_status()["jxl"]["available"]) + written = te.write_thumbnail( + Image.new("RGB", (16, 16)), + self.tmp / "v_thumb.jxl", + ThumbnailSettings("jxl", 80, 64), + ) + status = te.encoder_status()["jxl"] + self.assertEqual(written.name, "v_thumb.webp") + with Image.open(written) as img: + self.assertEqual(img.format, "WEBP") + self.assertEqual((status["available"], status["encoder"]), (False, None)) + self.assertEqual(self._leftovers(), []) + + def test_another_encoder_found_again_is_used(self): + found = iter(["cjxl", "ffmpeg"]) + used = [] + + def fake(cmd, **kwargs): + used.append(cmd[0]) + if cmd[0] == "cjxl": + raise FileNotFoundError("cjxl") + Path(cmd[-1]).write_bytes(JXL_CODESTREAM + b"\x00" * 8) + return subprocess.CompletedProcess(cmd, 0, b"", b"") + + with ( + patch.object(te, "_detect", side_effect=lambda: next(found)), + patch.object(te, "run_encoder", side_effect=fake), + patch.object(te, "_pillow_decodes_jxl", return_value=False), + ): + written = te.write_thumbnail( + Image.new("RGB", (16, 16)), + self.tmp / "w_thumb.jxl", + ThumbnailSettings("jxl", 80, 64), + ) + self.assertEqual(written.name, "w_thumb.jxl") + self.assertEqual(used, ["cjxl", "ffmpeg"]) + self.assertEqual(self._leftovers(), []) + + def test_the_same_encoder_failing_again_raises(self): + failed = subprocess.CompletedProcess([], 1, b"", b"bad input") + with ( + patch.object(te, "_detect", return_value="cjxl") as detect, + patch.object(te, "run_encoder", return_value=failed), + ): + with self.assertRaises(te.EncoderFailed): + te.write_thumbnail( + Image.new("RGB", (16, 16)), + self.tmp / "x_thumb.jxl", + ThumbnailSettings("jxl", 80, 64), + ) + self.assertEqual(detect.call_count, 2, "detected again once") + + class TestEncoderProcesses(TempCase): SLEEPER = [sys.executable, "-c", "import time; time.sleep(60)"] diff --git a/tests/test_thumbnail_format.py b/tests/test_thumbnail_format.py index 18fa0796..1571bde0 100644 --- a/tests/test_thumbnail_format.py +++ b/tests/test_thumbnail_format.py @@ -336,6 +336,50 @@ class TestPlanStatusAndJobs(FormatCase): self.assertTrue((self.output_dir / "k.png").exists()) +class TestReviewFixes(FormatCase): + async def test_a_legacy_preview_in_another_case_is_outdated(self): + # Review M1: X.JPG's older X_thumb.JPG, and y.png's y_thumb.PNG. + upper = self.output_dir / "X.JPG" + Image.new("RGB", (64, 64)).save(upper, "JPEG") + make_png(self.output_dir / "y.png") + for name in ("X_thumb.JPG", "y_thumb.PNG"): + legacy = self.output_dir / "thumbnails" / name + legacy.parent.mkdir(parents=True, exist_ok=True) + Image.new("RGB", (32, 32)).save(legacy, "PNG" if "PNG" in name else "JPEG") + body = await (await self.client.get(STATUS)).json() + self.assertEqual((body["outdated"], body["pending"]), (2, 0)) + resp = await self.client.post(JOB, json={"mode": "convert"}) + job = await self._finished((await resp.json())["job"]["id"]) + self.assertEqual(job.counters["generated"], 2, job.log) + names = sorted(os.listdir(self.output_dir / "thumbnails")) + self.assertEqual(names, ["X_thumb.webp", "y_thumb.webp"]) + + async def test_clear_also_removes_stray_temp_files(self): + make_png(self.output_dir / "k.png") + make_png(self._thumb("k")) + folder = self.output_dir / "thumbnails" + for name in (".k_thumb.part-1-2.webp", ".501one.part-3-4.jxl"): + (folder / name).write_bytes(b"partial") + keep = folder / "notes.txt" + keep.write_bytes(b"x") + resp = await self.client.post("/prompt_manager/images/clear-thumbnails") + self.assertEqual((await resp.json())["cleared_files"], 3) + self.assertEqual(os.listdir(folder), ["notes.txt"]) + + def test_the_startup_sweep_removes_only_old_temp_files(self): + folder = self.output_dir / "thumbnails" / "sub" + folder.mkdir(parents=True) + old = folder / ".a_thumb.part-1-2.webp" + young = folder / ".b_thumb.part-1-2.webp" + for path in (old, young): + path.write_bytes(b"partial") + when = old.stat().st_mtime - 3600 + os.utime(old, (when, when)) + self.assertEqual(self.api.sweep_thumbnail_temps(), 1) + self.assertFalse(old.exists()) + self.assertTrue(young.exists()) + + class TestRemoval(FormatCase): def test_removing_a_source_removes_its_previews_in_every_format(self): make_png(self.output_dir / "r.png") diff --git a/tests/test_thumbnail_jobs.py b/tests/test_thumbnail_jobs.py index 33c97cdc..c03333a8 100644 --- a/tests/test_thumbnail_jobs.py +++ b/tests/test_thumbnail_jobs.py @@ -617,6 +617,8 @@ class TestThumbnailJob(ThumbnailJobCase): "scale='min(iw,320)':'min(ih,320)':force_original_aspect_ratio=decrease", ) self.assertEqual(cmd[-5:], ["-f", "image2pipe", "-vcodec", "png", "-"]) + # Review M6: the input is always read as a file, never a protocol URL. + self.assertTrue(cmd[cmd.index("-i") + 1].startswith("file:")) with Image.open(self._thumb_path("clip")) as thumb: self.assertEqual((thumb.format, thumb.size), ("WEBP", (320, 180))) diff --git a/web/js/workspace/image-card.mjs b/web/js/workspace/image-card.mjs index 0158c8d2..4058fe7f 100644 --- a/web/js/workspace/image-card.mjs +++ b/web/js/workspace/image-card.mjs @@ -9,7 +9,7 @@ import { cardTitle, cardSubtitle, imageKey, mediaRatio } from './images-state.mj import { createWorkspaceIcon } from './icons.mjs'; import { createTagButton } from './image-tagging.mjs'; import { paintStar } from './images-favorites.mjs'; -import { fallBackToOriginal, thumbnailSource } from './thumbnail-support.mjs'; +import { fallBackToOriginal, replaceOnError, showsThumbnail, thumbnailSource } from './thumbnail-support.mjs'; function el(tag, text, className) { const node = document.createElement(tag); @@ -33,11 +33,11 @@ function videoMedia(image) { */ export function cardMedia(image, lazy = false) { const { resolveUrl } = window.ApiClient; - if (image.is_video && !image.thumbnail_url) return videoMedia(image); + if (!showsThumbnail(image)) return videoMedia(image); const node = el('img'); node.src = resolveUrl(thumbnailSource(image)); // replaceWith does nothing for a card no longer in the page. - if (image.is_video) node.addEventListener('error', () => node.replaceWith(videoMedia(image)), { once: true }); + if (image.is_video) replaceOnError(node, () => videoMedia(image)); else fallBackToOriginal(node, resolveUrl(image.url)); // The concept card image: alt is the card title, with the listing's intrinsic size when known. node.alt = cardTitle(image); node.decoding = 'async'; if (lazy) node.loading = 'lazy'; diff --git a/web/js/workspace/library.mjs b/web/js/workspace/library.mjs index 2385ff7a..382d7b06 100644 --- a/web/js/workspace/library.mjs +++ b/web/js/workspace/library.mjs @@ -15,7 +15,7 @@ import {pageHeading} from './heading.mjs'; import {mountToolbar} from './toolbar.mjs'; import {libraryFilters,libraryViewState} from './saved-views.mjs'; import {openRefineDialog,openContentDialog} from './workspace-dialogs.mjs'; -import {fallBackToOriginal,thumbnailSource} from './thumbnail-support.mjs'; +import {fallBackToOriginal,replaceOnError,showsThumbnail,thumbnailSource} from './thumbnail-support.mjs'; let shelfVisit=null; // Every confirmed receipt advances this; a shelf mounted before it never restores its cached stream. @@ -214,10 +214,12 @@ export function mountLibrary(main,api,parentScope,{positiveId,savedState=null,on const art=el('div',undefined,'card-art');art.setAttribute('aria-hidden','true'); const previews=item.previews || []; for(const image of previews.slice(0,2)) { - if(image.is_video && !image.thumbnail_url) art.append(el('span','Video preview','small')); + // #317: a video with no preview, or one this browser cannot show (JPEG XL), keeps its label. + if(!showsThumbnail(image)) art.append(el('span','Video preview','small')); else { - // #317: a JPEG XL preview this browser cannot show, or one that fails, loads the original. - const img=el('img');img.src=window.ApiClient.resolveUrl(thumbnailSource(image));if(!image.is_video) fallBackToOriginal(img,window.ApiClient.resolveUrl(image.url));img.alt='';img.loading='lazy';img.decoding='async';img.width=image.width || 800;img.height=image.height || 600;art.append(img); + // #317: an image's JPEG XL preview this browser cannot show, or one that fails, loads the original; + // a video's preview that fails becomes its label, never a broken image. + const img=el('img');img.src=window.ApiClient.resolveUrl(thumbnailSource(image));if(image.is_video) replaceOnError(img,()=>el('span','Video preview','small'));else fallBackToOriginal(img,window.ApiClient.resolveUrl(image.url));img.alt='';img.loading='lazy';img.decoding='async';img.width=image.width || 800;img.height=image.height || 600;art.append(img); } } if(!previews.length) art.append(el('span','No available image previews','small')); diff --git a/web/js/workspace/thumbnail-support.mjs b/web/js/workspace/thumbnail-support.mjs index 1c939bed..119bde06 100644 --- a/web/js/workspace/thumbnail-support.mjs +++ b/web/js/workspace/thumbnail-support.mjs @@ -8,6 +8,9 @@ * original when the thumbnail is JPEG XL and this browser cannot decode it. While the answer is unknown the * thumbnail is tried; `fallBackToOriginal` then catches a failure. * - `fallBackToOriginal(img, url)`: a thumbnail `` that fails to load shows the original instead (once). + * - `showsThumbnail(image)`: false for a video whose thumbnail is missing or one this browser cannot show (its URL + * would be the video itself): the card keeps the video's own element or label, never a broken image. + * - `replaceOnError(img, next)`: a video thumbnail `` that fails to load is replaced by `next()` (once). */ // A 1×1 JPEG XL image (cjxl -d 1), decoded once to learn whether this browser shows JPEG XL. @@ -60,6 +63,20 @@ export function thumbnailSource(image, supported = answer) { return thumbnail; } +/** Whether a card shows `image`'s thumbnail (always for an image: its original at worst). */ +export function showsThumbnail(image, supported = answer) { + if (!image?.is_video) return true; + const source = thumbnailSource(image, supported); + return Boolean(source) && source !== image.url; +} + +/** A video thumbnail `` that fails to load is replaced by `next()`, once (`listen` as in fallBackToOriginal). */ +export function replaceOnError(node, next, listen = null) { + const swap = () => node.replaceWith(next()); + if (listen) listen(node, 'error', swap, { once: true }); + else node.addEventListener('error', swap, { once: true }); +} + /** * A thumbnail `` that fails to load shows `original` instead, once. `listen(target, type, handler, options)` is the * caller's scoped listener (a scope's `listen`); without one a plain once-listener is used.