From d5e23c7d5f2838e1e89eae947000c25c1624cdfe Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Wed, 23 Sep 2026 10:47:34 -0700 Subject: [PATCH] perf(thumbs): the gallery shipped 77 MB to render 250px tiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operator found this in about a minute of using the live Desk: "images load at full resolution instead of calculated thumbnails, which means they load VERY slowly and are tiny." MEASURED on the live set: sindra-corpus-v1 66 images 77.5 MB 1024x1024 each sindra-sfw-pool 59 images 71.7 MB sindra 30 images 61.6 MB 2.1 MB average sindra-bakeoff 40 images 57.2 MB A tile renders around 250px, so the grid shipped roughly 16x the pixels that reach the screen. ⚠ OUR PARKING RATIONALE WAS WRONG IN AN INSTRUCTIVE WAY. ROADMAP parked progressive loading on "the largest gallery is 66 images; at that size a lazy grid is almost certainly fine", and the parking-lot row said "270 may be fine". Both count IMAGES. Neither weighs BYTES. We measured the dimension that was easy to measure rather than the one that determines the experience, and 66 really is a fine count sitting on a terrible payload. booth/thumbs.py caches WebP at 512px longest side inside the booth at `.thumbs/.webp` — inside on purpose, so a cache can never outlive what it describes. Pillow is an optional import: absent, every tile falls back to the original, so the page is heavier and never broken. Generation is lazy, atomic (temp + os.replace), rebuilt when the source is newer, and NEVER RAISES. ?thumb=1 rides the EXISTING file route rather than growing a new one, because that route's traversal guard is already correct and a second route is a second place to get it wrong. ALSO FIXES A PRE-EXISTING LEAK THE CACHE WOULD HAVE WALKED INTO. booth_items and zip_booth both tested `p.name.startswith(".")` — the FILE's name — so `.thumbs/a.png` (name `a.png`) would have rendered as a gallery item and shipped inside every zip. CLAUDE.md invariant 2 promises a dotfile costs nothing in item counts, galleries or zips; that was true only at the top level. Both now skip every dot-prefixed path COMPONENT. AND THE FILMSTRIP, which is the same defect in a worse place: it shows EVERY ring item at a few dozen pixels, so full-resolution frames there cost more than the grid did. The stage is untouched and stays full size, because that is the full-size review. Item.thumb is derived in the resolver, not by a template reasoning about `kind` (INV-1). build_gallery had to carry it too — a missing key there rendered as a SILENT fallback to the full image, which is exactly where a new Item field gets dropped with nothing failing. 754 green. --- booth/app.py | 28 ++++++- booth/items.py | 14 +++- booth/templates/booth.html | 2 +- booth/templates/view.html | 4 +- booth/thumbs.py | 99 ++++++++++++++++++++++ pyproject.toml | 1 + tests/test_items.py | 25 ++++++ tests/test_thumbs.py | 163 +++++++++++++++++++++++++++++++++++++ 8 files changed, 328 insertions(+), 8 deletions(-) create mode 100644 booth/thumbs.py create mode 100644 tests/test_thumbs.py diff --git a/booth/app.py b/booth/app.py index f130d57..ad293b9 100644 --- a/booth/app.py +++ b/booth/app.py @@ -144,6 +144,7 @@ def set_blurred(booth: Path, rel: str, on: bool) -> set[str]: # The link-board logic lives in booth/links.py (stdlib only) so the `booth` CLI # can use it without pulling FastAPI in. Re-exported here because call sites and # tests already reference these names through app. +from booth.thumbs import ensure_thumb from booth.asks import ( # noqa: E402 ANSWER_SUFFIX, ASK_SUFFIX, @@ -706,6 +707,10 @@ def build_gallery(child: Path) -> list[dict]: "section": it.section, # U7. Derived in the resolver (INV-1); this only carries it. "group": it.group, + # Same: the tile's image source. Undefined here rendered as a + # SILENT fallback to the full image — the adapter is exactly + # where a new Item field gets dropped without anything failing. + "thumb": it.thumb, # R2 C1. Same rule: the resolver numbers, this carries. "ordinal": it.ordinal, "caption": it.caption, @@ -727,8 +732,12 @@ def zip_booth(booth: Path) -> bytes: buf = io.BytesIO() with zipfile.ZipFile(buf, "w", zipfile.ZIP_DEFLATED) as zf: for p in sorted(booth.rglob("*")): - if p.is_file() and not p.name.startswith("."): - zf.write(p, p.relative_to(booth).as_posix()) + # Same rule as booth_items: the Booth's dot-namespace is every + # component, not just the leaf. A `.thumbs/` cache would otherwise + # ship inside every download. + rel = p.relative_to(booth) + if p.is_file() and not any(part.startswith(".") for part in rel.parts): + zf.write(p, rel.as_posix()) return buf.getvalue() @@ -1787,7 +1796,10 @@ def create_app( flagged_rels = flagged_targets(marks) # recorded above, before this read: the current item counts as seen seen = read_seen(booth) & set(ring) - film = [{"name": r, "url": by_rel[r].url, "ordinal": by_rel[r].ordinal, + # `thumb` rides along for the filmstrip and the tray. NOT for the + # stage, which is the full-size review and must stay full size. + film = [{"name": r, "url": by_rel[r].url, "thumb": by_rel[r].thumb, + "ordinal": by_rel[r].ordinal, "kind": by_rel[r].kind, "blurred": by_rel[r].blurred, "flagged": r in flagged_rels, "seen": r in seen, "current": r == f} for r in ring] @@ -1839,7 +1851,7 @@ def create_app( ) @app.get("/b/{name}/{filepath:path}") - def booth_file(name: str, filepath: str, dl: int = 0): + def booth_file(name: str, filepath: str, dl: int = 0, thumb: int = 0): booth = resolve_booth(name) try: target = (booth / filepath).resolve() @@ -1852,6 +1864,14 @@ def create_app( # in-page with no easy "save". if dl: return FileResponse(str(target), filename=target.name) + # ?thumb=1 rides THIS route on purpose: the traversal guard above is + # already correct and a second route would be a second place to get it + # wrong. A cache miss, a damaged image or no Pillow all fall through to + # the original, so the tile is never broken — only heavier. + if thumb: + cached = ensure_thumb(booth, target.relative_to(booth).as_posix()) + if cached is not None: + return FileResponse(str(cached), media_type="image/webp") return FileResponse(str(target)) @app.post("/upload") diff --git a/booth/items.py b/booth/items.py index 5ad0a38..323e8ee 100644 --- a/booth/items.py +++ b/booth/items.py @@ -30,6 +30,7 @@ except ImportError: # pragma: no cover _markdown = None from booth.asks import is_answer_file, is_ask_file +from booth.thumbs import wants_thumb # Browser-playable media buckets. Anything else renders as a download link. IMAGE_EXTS = {".png", ".jpg", ".jpeg", ".gif", ".webp", ".avif", ".svg", ".bmp"} @@ -103,6 +104,11 @@ class Item: # and nowhere else (INV-1). APPENDED, never inserted: a mid-dataclass field # is a positional-construction break. ordinal: int + # The tile's image source, or None when the original IS the right source + # (vector, video, a type Pillow cannot open, or an image already tile-sized). + # Derived HERE so no template reasons about `kind` to decide — INV-1, which + # is the caption bug in a new field. + thumb: str | None # R2 C2: which items have been looked at full size. UI state, not judgment — @@ -283,7 +289,12 @@ def booth_items(booth: Path) -> list[Item]: """ by_rel: dict[str, Path] = {} for p in booth.rglob("*"): - if not p.is_file() or p.name.startswith("."): + # ⚠ EVERY path component, not just the filename. `p.name.startswith(".")` + # tested only the leaf, so `.thumbs/a.png` (name `a.png`) sailed through + # as a gallery item — and CLAUDE.md invariant 2 promises a dotfile costs + # nothing in item counts, galleries or zips. That promise was true only + # at the top level until the `.thumbs/` cache made it matter. + if not p.is_file() or any(part.startswith(".") for part in p.relative_to(booth).parts): continue if is_ask_file(p.name) or is_answer_file(p.name): continue @@ -331,6 +342,7 @@ def booth_items(booth: Path) -> list[Item]: # the quote() guard skipped takes no number, so the numbers # stay contiguous over what the operator can see. ordinal=len(items) + 1, + thumb=(quote(rel, safe='/') + '?thumb=1') if wants_thumb(rel) else None, ) ) return items diff --git a/booth/templates/booth.html b/booth/templates/booth.html index 3e08a95..76a6549 100644 --- a/booth/templates/booth.html +++ b/booth/templates/booth.html @@ -386,7 +386,7 @@ {% endif %} {% if it.kind == 'image' %} - {{ it.name }} + {{ it.name }} {% elif it.kind == 'video' %} {# preload="none": a booth of a dozen webms was fetching them all at page load ("metadata" still pulls real ranges per diff --git a/booth/templates/view.html b/booth/templates/view.html index eb74b50..9557cf4 100644 --- a/booth/templates/view.html +++ b/booth/templates/view.html @@ -99,7 +99,7 @@
{% for x in tray %} - {%- if x.kind == 'image' %}{% else %}{{ x.kind }}{% endif -%} + {%- if x.kind == 'image' %}{% else %}{{ x.kind }}{% endif -%} {{ num(x.ordinal) }} {% endfor %}
@@ -124,7 +124,7 @@ {% for x in film %} - {%- if x.kind == 'image' %}{% else %}{{ '♪' if x.kind == 'audio' else '▶' }}{% endif -%} + {%- if x.kind == 'image' %}{% else %}{{ '♪' if x.kind == 'audio' else '▶' }}{% endif -%} {{ num(x.ordinal) }} {% endfor %} diff --git a/booth/thumbs.py b/booth/thumbs.py new file mode 100644 index 0000000..872dfd0 --- /dev/null +++ b/booth/thumbs.py @@ -0,0 +1,99 @@ +"""Derived thumbnails, cached inside the booth. + +MEASURED, not assumed. ROADMAP parked progressive loading on "the largest +gallery is 66 images; at that size a lazy grid is almost certainly fine" — which +counted IMAGES and never weighed BYTES. The live set on 2026-09-23: + + sindra-corpus-v1 66 images 77.5 MB 1024x1024 each + sindra-sfw-pool 59 images 71.7 MB + sindra 30 images 61.6 MB 2.1 MB average + +A tile renders around 250px wide, so the gallery shipped roughly 16x the pixels +that reach the screen and 77 MB on one page load. 66 is a fine count sitting on +a terrible payload; the operator found it in about a minute of using the Desk. + +The cache lives at `/.thumbs/.webp` — inside the booth on purpose, +so it is swept with the booth and never outlives what it describes. Both +`booth_items` and `zip_booth` skip every dot-prefixed path COMPONENT, which they +did not do until this module needed them to. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +try: # optional: absence degrades to full-size images, never to a broken page + from PIL import Image as _Image +except ImportError: # pragma: no cover + _Image = None + +THUMB_DIR = ".thumbs" +THUMB_MAX = 512 # longest side, px — comfortably above any tile size +THUMB_QUALITY = 78 + +# What Pillow can open from a plain install. SVG is vector (Pillow cannot read +# it, and it is already small); AVIF needs a plugin we do not require. +THUMBABLE = {".png", ".jpg", ".jpeg", ".webp", ".gif", ".bmp"} + + +def thumb_path(booth: Path, rel: str) -> Path: + """Where `rel`'s thumbnail lives. Mirrors the tree so two files with the + same basename in different folders cannot collide.""" + return booth / THUMB_DIR / (rel + ".webp") + + +def wants_thumb(rel: str) -> bool: + """Whether a rel is a candidate at all — extension only, no file read. + + Called from the resolver for every item on every index load, so it must not + touch the disk.""" + return Path(rel).suffix.lower() in THUMBABLE + + +def ensure_thumb(booth: Path, rel: str) -> Path | None: + """The cached thumbnail for `rel`, generating it if needed. None when there + should not be one — Pillow absent, unsupported type, source already small + enough, or anything at all went wrong. + + NEVER RAISES. A thumbnail is an optimisation; a booth page that will not + load is worse than a page that loads slowly, which is the posture every + other read on this path already takes. + + Regenerates when the source is newer than the cache, so editing a file in + place does not leave the old thumbnail behind forever. + """ + if _Image is None or not wants_thumb(rel): + return None + src = booth / rel + out = thumb_path(booth, rel) + try: + s_stat = src.stat() + try: + if out.stat().st_mtime >= s_stat.st_mtime: + return out + except OSError: + pass # no cache yet, or unreadable — fall through and build one + + with _Image.open(src) as im: + # `open` reads the header only, so this is cheap enough to decide on. + if max(im.size) <= THUMB_MAX: + return None # already tile-sized; serving the original is right + im.thumbnail((THUMB_MAX, THUMB_MAX)) + if im.mode not in ("RGB", "RGBA"): + im = im.convert("RGBA" if "A" in im.getbands() else "RGB") + out.parent.mkdir(parents=True, exist_ok=True) + # Atomic, like every other sidecar this service writes: a reader + # polling the cache never sees a half-encoded image. + tmp = out.with_name(out.name + f".{os.getpid()}.tmp") + try: + im.save(tmp, "WEBP", quality=THUMB_QUALITY, method=4) + os.replace(tmp, out) + finally: + try: + tmp.unlink() + except OSError: + pass + return out + except Exception: # noqa: BLE001 — a bad image costs its own tile, never the page + return None diff --git a/pyproject.toml b/pyproject.toml index 7e950d7..be29b11 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,6 +9,7 @@ dependencies = [ "jinja2>=3.1", "python-multipart>=0.0.9", "markdown>=3.5", + "pillow>=10.0", ] [project.optional-dependencies] diff --git a/tests/test_items.py b/tests/test_items.py index af26cb9..4cf68f2 100644 --- a/tests/test_items.py +++ b/tests/test_items.py @@ -356,3 +356,28 @@ def test_a_huge_caption_sidecar_is_not_read_whole(tmp_path): cap = {it.rel: it.caption for it in booth_items(b)}["a.png"] assert cap is not None and len(cap) <= CAPTION_MAX + + +def test_a_dot_directory_hides_its_whole_subtree(tmp_path): + """CLAUDE.md invariant 2 claims a dotfile costs nothing in item counts, + galleries or zips. That was only true at the TOP LEVEL: both `booth_items` + and `zip_booth` tested the FILE's name, so `.thumbs/a.png` has `p.name == + "a.png"` and sailed through as a gallery item and a zip entry. + + Pre-existing, found while adding a `.thumbs/` cache. Any path component + starting with a dot is the Booth's own namespace. + + Defeating change: back to `p.name.startswith(".")`.""" + import io + import zipfile + + from booth.app import zip_booth + + b = tmp_path / "b" + (b / ".thumbs").mkdir(parents=True) + (b / "real.png").write_bytes(b"\x89PNG") + (b / ".thumbs" / "real.png").write_bytes(b"\x89PNGthumb") + (b / ".marks.json").write_text("{}") + + assert [i.rel for i in booth_items(b)] == ["real.png"] + assert zipfile.ZipFile(io.BytesIO(zip_booth(b))).namelist() == ["real.png"] diff --git a/tests/test_thumbs.py b/tests/test_thumbs.py new file mode 100644 index 0000000..7c77683 --- /dev/null +++ b/tests/test_thumbs.py @@ -0,0 +1,163 @@ +"""Thumbnails — the fix for a gallery that shipped 77 MB to render 250px tiles. + +The operator found this in about a minute of using the live Desk. ROADMAP had +parked it on "the largest gallery is 66 images", which counted IMAGES and never +weighed BYTES; 66 is a fine count sitting on a terrible payload. +""" + +from __future__ import annotations + +import io +import pathlib +import sys + +import pytest +from fastapi.testclient import TestClient + +sys.path.insert(0, str(pathlib.Path(__file__).parent.parent)) + +from booth.app import create_app # noqa: E402 +from booth.items import booth_items # noqa: E402 +from booth.thumbs import THUMB_DIR, THUMB_MAX, ensure_thumb, wants_thumb # noqa: E402 + +PIL = pytest.importorskip("PIL.Image", reason="Pillow is not installed") + + +def _img(path: pathlib.Path, w: int, h: int, fmt="PNG"): + path.parent.mkdir(parents=True, exist_ok=True) + PIL.new("RGB", (w, h), (120, 30, 90)).save(path, fmt) + return path + + +def test_a_big_image_gets_a_much_smaller_thumbnail(tmp_path): + """The whole point, asserted in BYTES rather than in existence — a thumbnail + that is not dramatically smaller has not fixed anything.""" + b = tmp_path / "g" + src = _img(b / "big.png", 1024, 1024) + out = ensure_thumb(b, "big.png") + assert out is not None and out.is_file() + assert max(PIL.open(out).size) <= THUMB_MAX + assert out.stat().st_size * 4 < src.stat().st_size, ( + f"thumb {out.stat().st_size}B vs source {src.stat().st_size}B — not worth the cache" + ) + + +def test_an_already_small_image_gets_no_thumbnail(tmp_path): + """Serving the original is correct when it is already tile-sized. A cache + entry that saves nothing is pure cost. + + Defeating change: generating unconditionally.""" + b = tmp_path / "g" + _img(b / "small.png", 200, 200) + assert ensure_thumb(b, "small.png") is None + + +def test_the_cache_lives_inside_the_booth_and_is_invisible(tmp_path): + """`.thumbs/` is inside the booth so it is swept with it — a cache that + outlives what it describes is a leak. And it must not become gallery items + or zip entries: both skip every dot-prefixed path COMPONENT, which they did + not do until this module needed them to.""" + import zipfile + + from booth.app import zip_booth + + b = tmp_path / "g" + _img(b / "big.png", 1024, 1024) + ensure_thumb(b, "big.png") + assert (b / THUMB_DIR).is_dir(), "the cache is not inside the booth" + assert [i.rel for i in booth_items(b)] == ["big.png"] + assert zipfile.ZipFile(io.BytesIO(zip_booth(b))).namelist() == ["big.png"] + + +def test_a_damaged_image_costs_its_own_tile_not_the_page(tmp_path): + """NEVER RAISES. A thumbnail is an optimisation; a page that will not load + is worse than one that loads slowly. + + Defeating change: letting the Pillow exception out.""" + b = tmp_path / "g" + b.mkdir() + (b / "lies.png").write_bytes(b"\x89PNG\r\n\x1a\n" + b"not an image at all" * 20) + assert ensure_thumb(b, "lies.png") is None + + +def test_a_stale_thumbnail_is_rebuilt(tmp_path): + """Editing a file in place must not leave the old thumbnail forever.""" + import os + + b = tmp_path / "g" + _img(b / "x.png", 1024, 1024) + first = ensure_thumb(b, "x.png") + before = first.stat().st_mtime_ns + _img(b / "x.png", 900, 900) + os.utime(b / "x.png", None) + again = ensure_thumb(b, "x.png") + assert again.stat().st_mtime_ns != before, "the stale thumbnail survived an edit" + + +def test_only_thumbable_types_are_candidates(): + """SVG is vector and Pillow cannot read it; a video is not an image.""" + assert wants_thumb("a.png") and wants_thumb("A.JPG") and wants_thumb("a.webp") + assert not wants_thumb("a.svg") and not wants_thumb("a.webm") and not wants_thumb("a.txt") + + +def test_the_item_record_carries_the_thumb_url(tmp_path): + """INV-1: the resolver decides whether an item has a thumbnail. No template + appends `?thumb=1` by reasoning about `kind` itself.""" + b = tmp_path / "g" + _img(b / "big.png", 1024, 1024) + _img(b / "vec.svg", 10, 10) if False else (b / "vec.svg").write_text("") + by = {i.rel: i for i in booth_items(b)} + assert by["big.png"].thumb == "big.png?thumb=1" + assert by["vec.svg"].thumb is None + + +def test_the_route_serves_the_thumbnail_and_the_original(tmp_path): + """?thumb=1 rides the EXISTING file route, so it inherits that route's + traversal guard rather than growing a second one.""" + b = tmp_path / "g" + src = _img(b / "big.png", 1024, 1024) + c = TestClient(create_app(tmp_path, ttl_hours=24, start_sweeper=False)) + + full = c.get("/b/g/big.png") + thumb = c.get("/b/g/big.png?thumb=1") + assert full.status_code == thumb.status_code == 200 + assert len(thumb.content) * 4 < len(full.content), "the route served the full image" + assert len(full.content) == src.stat().st_size + + +def test_the_gallery_tile_requests_the_thumbnail(tmp_path): + """The operator's actual complaint: the grid pulled full-resolution files.""" + b = tmp_path / "g" + for n in ("a.png", "b.png"): + _img(b / n, 1024, 1024) + c = TestClient(create_app(tmp_path, ttl_hours=24, start_sweeper=False)) + html = c.get("/b/g/").text + assert 'src="a.png?thumb=1"' in html, "the tile still asks for the full image" + + +def test_a_thumb_request_for_a_traversal_path_is_still_refused(tmp_path): + """The guard is the file route's, and it must not be weakened by the new + query parameter.""" + b = tmp_path / "g" + _img(b / "big.png", 1024, 1024) + (tmp_path / "secret.txt").write_text("nope") + c = TestClient(create_app(tmp_path, ttl_hours=24, start_sweeper=False)) + assert c.get("/b/g/../secret.txt?thumb=1").status_code in (404, 400) + + +def test_the_filmstrip_and_tray_use_thumbnails_but_the_stage_does_not(tmp_path): + """The same 77 MB in a different place. The filmstrip shows EVERY ring item + at a few dozen pixels, so full-resolution frames there are worse than the + grid was — while the stage is the full-size review and must stay full size. + + Defeating change: `x.url` in the filmstrip, or `it.thumb` on the stage.""" + b = tmp_path / "g" + for n in ("a.png", "b.png", "c.png"): + _img(b / n, 1024, 1024) + c = TestClient(create_app(tmp_path, ttl_hours=24, start_sweeper=False)) + html = c.get("/b/g/view?f=a.png").text + + assert 'id="vimg" src="a.png"' in html, "the stage must serve the full image" + assert 'src="b.png?thumb=1"' in html, "the filmstrip still pulls full images" + assert 'src="b.png"' not in html.replace('src="b.png?thumb=1"', ""), \ + "a full-size frame survived in the strip"