From 27cce0569a5831cbac93cfdeea051b41ecdfd70d Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 09:45:02 -0500 Subject: [PATCH 01/15] build: declare waitress in requirements.txt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit waitress is a real runtime dependency — pyproject.toml pins waitress>=2.1 and debian/control ships python3-waitress — but it was missing from requirements.txt. Recreating the venv the way AGENTS.md documents therefore produced an environment where the server could not start, which surfaced as test_server_smoke failing on ModuleNotFoundError. Co-Authored-By: Claude Opus 5 (1M context) --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index 35521df3..9bfedf7c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,6 +12,7 @@ pillow-jxl-plugin # JPEG XL support # Web server Flask +waitress # WSGI server with a bounded request-thread pool zeroconf zstd # panel cache compression From f66c39eb1c72c66c4e82636fd97b3ff8be5bdc52 Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 09:45:25 -0500 Subject: [PATCH 02/15] fix(server): stop a hot-reload reverting the image DB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AppState.reload() builds the replacement manager first, and that constructor reads image_manager.json. The outgoing manager was then shut down, and shutdown() flushed its own in-memory snapshot back over that same file. A multi-threaded manager compounded it: shutdown() uses executor.shutdown( wait=False), so its render callbacks keep calling _save_db() long after the swap. Anything written to the DB between the replacement loading it and the old manager going away was silently reverted, and reload() runs on every config save. Only derived fields (slugs, convert_status, timings) are at risk today and they self-heal on the next sync, which is why this went unnoticed. Per-picture dither overrides are about to live in this file too, and those are user-authored — reverting them would be real data loss. Split the teardown in two: shutdown() — flush, then stop workers and freeze writes. Real teardown: process exit and tests, which rely on the flush. retire() — stop workers and freeze writes WITHOUT flushing. For a manager that has been superseded: its replacement already owns the file, and every mutation persisted itself as it happened, so there is nothing left to write. reload() now retires the old manager instead of shutting it down. A _closed flag makes _save_db() a no-op afterwards, so late render callbacks cannot write either. Freezing happens before the workers are stopped, not after, so there is no window between the two. _stop_workers() replaces the previous shutdown() override in the concrete managers, which keeps the flush-then-teardown order in one place. Co-Authored-By: Claude Opus 5 (1M context) --- python/hokku/webserver/app_state.py | 9 ++-- .../hokku/webserver/image_manager_abstract.py | 38 ++++++++++++++++- python/hokku/webserver/image_manager_multi.py | 6 ++- .../hokku/webserver/image_manager_single.py | 3 ++ python/tests/test_image_manager.py | 41 +++++++++++++++++++ python/tests/test_reload.py | 29 +++++++++++++ 6 files changed, 120 insertions(+), 6 deletions(-) diff --git a/python/hokku/webserver/app_state.py b/python/hokku/webserver/app_state.py index 8018eb26..a3305e1b 100644 --- a/python/hokku/webserver/app_state.py +++ b/python/hokku/webserver/app_state.py @@ -93,7 +93,7 @@ def reload(self, new_config: AppConfig) -> None: Always builds a fresh manager — its render dispatch (inline or thread pool) is reconstructed from scratch every reload. The old - manager is shut down outside the lock. + manager is retired outside the lock. Validates that upload_dir and cache_dir exist before touching anything, so callers can surface a 400 if the new config is unusable. @@ -125,8 +125,11 @@ def reload(self, new_config: AppConfig) -> None: self.manager = new_manager self.scheduler = new_scheduler - # Shut the old manager down outside the lock (releases its workers). - old_manager.shutdown() + # Retire, don't shut down: new_manager has already read the image DB, so + # a parting flush from the old one would revert it. retire() stops the + # old workers and freezes its writes without touching the file. Nothing + # is lost — every mutation persists itself as it happens. + old_manager.retire() # Restart mDNS if the hostname changed (or toggled on/off). if new_config.mdns_hostname != old_hostname: diff --git a/python/hokku/webserver/image_manager_abstract.py b/python/hokku/webserver/image_manager_abstract.py index 4799fb84..9dd06fe8 100644 --- a/python/hokku/webserver/image_manager_abstract.py +++ b/python/hokku/webserver/image_manager_abstract.py @@ -105,6 +105,13 @@ def __init__(self, config: AppConfig, classifier=None) -> None: self._progress = ConversionProgress(current_name=None, done=0, total=0) self._batch_failed: int = 0 + # Set by shutdown(); silences every later _save_db(). AppState.reload() + # builds the replacement manager (which loads the DB) *before* shutting + # this one down, and a multi-threaded manager's in-flight render + # callbacks keep firing after that, so without this flag a retiring + # manager writes its stale snapshot over the live one's file. + self._closed = False + # Names of images currently being rendered. Protected by _db_lock. self._inflight: set[str] = set() @@ -150,9 +157,33 @@ def _dispatch_render( # ── lifecycle ──────────────────────────────────────────────── def shutdown(self) -> None: - """Flush DB to disk. Override in subclasses to also tear down workers.""" + """Flush the DB, then stop the workers and freeze further writes. + + For real teardown — process exit and tests. A hot-reload wants + ``retire()`` instead. + """ with self._db_lock: self._save_db() + self.retire() + + def retire(self) -> None: + """Stop the workers and freeze writes, *without* flushing the DB. + + Used when a replacement manager has already been built: it read the DB + file in its constructor, so writing our snapshot over it would revert + whatever it loaded. Nothing is lost by skipping the flush — every + mutation persists itself as it happens, so the file is already current. + + Freezing comes first so that render callbacks still landing from the + workers we are about to stop cannot write either. + """ + with self._db_lock: + self._closed = True + self._stop_workers() + + @abstractmethod + def _stop_workers(self) -> None: + """Tear down any render workers, so no further callbacks are dispatched.""" def wait_for_idle(self, timeout: float = 120.0) -> None: """Block until all in-flight renders have completed. @@ -690,6 +721,11 @@ def _load_db(self) -> None: logger.warning("Skipping malformed db entry %r: %s", name, e) def _save_db(self) -> None: + if self._closed: + # A retired manager's in-flight render callbacks still land here. + # Its successor already owns the file; writing would revert it. + logger.debug("Ignoring _save_db() on a shut-down manager") + return payload = { "version": _DB_VERSION, "images": {n: r.to_dict() for n, r in self._records.items()}, diff --git a/python/hokku/webserver/image_manager_multi.py b/python/hokku/webserver/image_manager_multi.py index 349828f4..5386b7d4 100644 --- a/python/hokku/webserver/image_manager_multi.py +++ b/python/hokku/webserver/image_manager_multi.py @@ -55,6 +55,8 @@ def _dispatch_render( ) ) - def shutdown(self) -> None: + def _stop_workers(self) -> None: + # wait=False: in-flight renders finish on their own threads. Their + # completion callbacks still run, but the DB is frozen by then (see + # AbstractImageManager.retire), so they cannot write. self._executor.shutdown(wait=False) - super().shutdown() diff --git a/python/hokku/webserver/image_manager_single.py b/python/hokku/webserver/image_manager_single.py index 0240906f..b3f9c209 100644 --- a/python/hokku/webserver/image_manager_single.py +++ b/python/hokku/webserver/image_manager_single.py @@ -22,6 +22,9 @@ class SingleThreadedImageManager(AbstractImageManager): def resolved_worker_count(self) -> int: return 1 + def _stop_workers(self) -> None: + """Nothing to stop — renders run inline and are already finished.""" + def _dispatch_render( self, name: str, diff --git a/python/tests/test_image_manager.py b/python/tests/test_image_manager.py index 5b6ff421..62da1a16 100644 --- a/python/tests/test_image_manager.py +++ b/python/tests/test_image_manager.py @@ -10,6 +10,7 @@ from __future__ import annotations +import json from io import BytesIO from pathlib import Path @@ -170,6 +171,46 @@ def test_db_survives_restart(app_config: AppConfig, image_manager_factory, make_ assert rec2 == rec +def test_retire_does_not_flush(app_config: AppConfig, image_manager_factory, make_test_image): + """retire() leaves the DB file alone — its successor already owns it. + + Diverging _records first proves the file is untouched rather than merely + rewritten with identical content. + """ + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png") + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + db_path = Path(app_config.cache_dir) / "image_manager.json" + + mgr._records.clear() + mgr.retire() + + assert "a.png" in json.loads(db_path.read_text())["images"] + + +def test_writes_frozen_after_retire(app_config: AppConfig, image_manager_factory, make_test_image): + """A retired manager cannot write, even when asked directly. + + This is what stops a hot-reload's outgoing manager — whose render callbacks + keep firing after the swap — from reverting the live manager's file. + """ + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png") + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + db_path = Path(app_config.cache_dir) / "image_manager.json" + + mgr.retire() + mgr._records.clear() + mgr._save_db() # a late render callback + mgr.shutdown() # and an explicit teardown afterwards + + assert "a.png" in json.loads(db_path.read_text())["images"] + + def test_disk_change_detected(app_config: AppConfig, image_manager_factory, make_test_image): upload = Path(app_config.upload_dir) make_test_image(upload / "a.png", color=(255, 0, 0)) diff --git a/python/tests/test_reload.py b/python/tests/test_reload.py index 043d63b4..93a2e086 100644 --- a/python/tests/test_reload.py +++ b/python/tests/test_reload.py @@ -174,6 +174,35 @@ def test_reload_builds_new_classifier(app_config: AppConfig, tmp_path: Path): assert state.classifier is not old_classifier +def test_reload_does_not_revert_the_image_db( + app_config: AppConfig, tmp_path: Path, make_test_image +): + """The outgoing manager must not write its snapshot over its successor's file. + + reload() builds the replacement first, and that constructor reads + image_manager.json. The old manager's render callbacks keep firing after + the swap, so if it were still allowed to persist it would revert whatever + the live manager had loaded or has since written. Only derived fields are + at risk today, but per-picture overrides live in this file too. + """ + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png") + state = _make_state(app_config) + state.manager.sync() + state.manager.wait_for_idle() + old_manager = state.manager + + state.reload(_alt_config(app_config, tmp_path)) + assert state.manager is not old_manager + + # Whatever the retired manager still holds, it may no longer persist it. + old_manager._records.clear() + old_manager.shutdown() + + db = json.loads((Path(app_config.cache_dir) / "image_manager.json").read_text()) + assert "a.png" in db["images"] + + def test_reload_manager_wired_with_new_classifier(app_config: AppConfig, tmp_path: Path): """After reload, state.manager._classifier is state.classifier.""" state = _make_state(app_config) From c7c8e2e67f5b2a552dcbbfcb8b84213f4404c324 Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 09:49:52 -0500 Subject: [PATCH 03/15] feat(server): add a strict ImageConfig parser for API input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _image_config_from_dict() merges a stored blob onto the defaults, which is right for a config file written by an older version: a field added since then is simply a field that keeps its default. It is the wrong contract for data arriving over the API, where the same leniency means a request can quietly mean something other than it says. Three ways that bites, all of which currently answer 200 OK: - an unknown key is never read, so a typo'd knob name is dropped and the setting the user asked for is silently not applied; - a missing field keeps a default rather than being reported; - Literal values are unchecked, so an invalid lut_name is accepted here and only rejected later inside a render worker, where it surfaces to the user as a failed conversion rather than as a rejected setting. image_config_from_dict_strict() requires every field, rejects unknown keys, and validates types, enums and ranges. It reports every problem at once so a UI can list them instead of making the user resubmit once per mistake. Types come from the dataclass annotations, so a new field is covered automatically; only value ranges are declared by hand, and only where an out-of-range number crashes a stage or produces nonsense. bool is checked in both directions because it is a subclass of int — `serpentine: 1` would otherwise pass, and `prepare_gamma: true` likewise. The lenient parser is untouched: AppConfig.from_dict and render_worker still use it, and a test pins that a strict-validated config survives the lenient round trip the render worker performs, since the rendered image would otherwise not match the settings its cache slug was computed from. Also drops the hand-copied LUT list in dither_streaming._validate in favour of get_args(LutName). The two had to be kept in step by hand, and a LUT added to the Literal but forgotten there was rejected at render time. Co-Authored-By: Claude Opus 5 (1M context) --- python/hokku/webserver/dither_streaming.py | 18 +- python/hokku/webserver/image_config.py | 164 ++++++++++++++++- python/tests/test_image_config.py | 199 ++++++++++++++++++++- 3 files changed, 364 insertions(+), 17 deletions(-) diff --git a/python/hokku/webserver/dither_streaming.py b/python/hokku/webserver/dither_streaming.py index d90f1748..5e695b82 100644 --- a/python/hokku/webserver/dither_streaming.py +++ b/python/hokku/webserver/dither_streaming.py @@ -15,7 +15,7 @@ import logging from functools import lru_cache -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, get_args import numpy as np from numpy.typing import ArrayLike, NDArray @@ -31,7 +31,7 @@ PrepStripe, UInt8Array, ) -from hokku.webserver.dither_config import DitherConfig +from hokku.webserver.dither_config import DitherConfig, LutName if TYPE_CHECKING: from hokku.screens.display import Display @@ -795,17 +795,9 @@ def _row_pixels(y: int) -> NDArray[np.float32]: def _validate(cfg: DitherConfig) -> None: if cfg.algorithm not in _KERNEL_FOR and cfg.algorithm != "noop": raise ValueError(f"Unknown algorithm: {cfg.algorithm!r}") - if cfg.lut_name not in ( - "euclidean", - "euclidean_weighted", - "hue_aware", - "hue_aware_weighted", - "bw", - "oklab", - "oklab_hue_aware", - "cam16ucs", - "cam16ucs_hue_aware", - ): + # Read the accepted names off the Literal rather than repeating them: a LUT + # added to LutName but forgotten here used to be rejected at render time. + if cfg.lut_name not in get_args(LutName): raise ValueError(f"Unknown lut_name: {cfg.lut_name!r}") diff --git a/python/hokku/webserver/image_config.py b/python/hokku/webserver/image_config.py index a1ea6dfe..9d89213e 100644 --- a/python/hokku/webserver/image_config.py +++ b/python/hokku/webserver/image_config.py @@ -5,8 +5,9 @@ import hashlib import json import logging +from collections.abc import Callable from dataclasses import asdict, dataclass, fields, replace -from typing import Any, Literal +from typing import Any, Literal, get_args, get_origin, get_type_hints from hokku.webserver.dither_config import DitherConfig @@ -78,6 +79,167 @@ def _bw_safe_image_config(cfg: ImageConfig) -> ImageConfig: # unnecessary — a missing field is simply a field that keeps its default. +class ImageConfigError(ValueError): + """A caller-supplied ImageConfig blob was rejected. + + ``errors`` holds every problem found, not just the first, so a UI can list + them all instead of making the user resubmit once per mistake. + """ + + def __init__(self, errors: list[str]) -> None: + self.errors = list(errors) + super().__init__("; ".join(self.errors)) + + +# Value constraints for the fields where an out-of-range number either crashes a +# pipeline stage or silently produces nonsense. Fields absent here are still +# type-checked; they simply have no meaningful bound. Keyed by field name across +# both ImageConfig and DitherConfig — the names do not collide. +_CONSTRAINTS: dict[str, tuple[Callable[[float], bool], str]] = { + # PIL's autocontrast takes this percentage off *each* end of the histogram, + # so 50 or more would clip the whole range away. + "prepare_autocontrast_cutoff": (lambda v: 0.0 <= v < 50.0, "must be >= 0 and < 50"), + # Exponents: zero or negative inverts or flattens the curve into garbage. + "prepare_gamma": (lambda v: v > 0.0, "must be > 0"), + "prepare_midtone": (lambda v: v > 0.0, "must be > 0"), + # PIL enhancement factors: 1.0 is "unchanged", 0 is the degenerate image. + "prepare_brightness": (lambda v: v > 0.0, "must be > 0"), + "prepare_contrast": (lambda v: v > 0.0, "must be > 0"), + "color_enhance": (lambda v: v > 0.0, "must be > 0"), + "saturate_max_enhance": (lambda v: v > 0.0, "must be > 0"), + # Chroma thresholds are distances in Lab/OKLAB space — never negative. + "saturate_low_chroma_thresh": (lambda v: v >= 0.0, "must be >= 0"), + "saturate_high_chroma_thresh": (lambda v: v >= 0.0, "must be >= 0"), + "saturate_low_chroma_thresh_oklab": (lambda v: v >= 0.0, "must be >= 0"), + "saturate_high_chroma_thresh_oklab": (lambda v: v >= 0.0, "must be >= 0"), + "vivid_chroma_low": (lambda v: v >= 0.0, "must be >= 0"), + "vivid_chroma_high": (lambda v: v >= 0.0, "must be >= 0"), + "vivid_chroma_low_oklab": (lambda v: v >= 0.0, "must be >= 0"), + "vivid_chroma_high_oklab": (lambda v: v >= 0.0, "must be >= 0"), + "neutral_chroma": (lambda v: v >= 0.0, "must be >= 0"), + "clahe_clip_limit": (lambda v: v >= 0.0, "must be >= 0 (0 disables CLAHE)"), + # Feather is a fraction of the canvas's short edge. + "clahe_keepout_feather": (lambda v: 0.0 <= v <= 1.0, "must be between 0 and 1"), + "prepare_usm_radius": (lambda v: v >= 0.0, "must be >= 0"), + "prepare_usm_amount": (lambda v: v >= 0, "must be >= 0 (0 disables sharpening)"), + "dither_noise": (lambda v: v >= 0.0, "must be >= 0 (0 disables noise)"), + # A hue angle, in degrees. + "hue_cutoff_deg": (lambda v: 0.0 <= v <= 360.0, "must be between 0 and 360"), +} + + +def _check_value(name: str, value: Any, hint: Any, path: str, errors: list[str]) -> None: + """Type- and range-check one field, appending to *errors* rather than raising.""" + where = f"{path}.{name}" + + if get_origin(hint) is Literal: + allowed = get_args(hint) + if value not in allowed: + errors.append(f"{where}: {value!r} is not one of {', '.join(map(repr, allowed))}") + return + + # bool is a subclass of int in Python, so both directions need the explicit + # check: True must not pass as a number, and 1 must not pass as a flag. + if hint is bool: + if not isinstance(value, bool): + errors.append(f"{where}: must be true or false, got {value!r}") + return + if hint is int: + if isinstance(value, bool) or not isinstance(value, int): + errors.append(f"{where}: must be a whole number, got {value!r}") + return + elif hint is float: + if isinstance(value, bool) or not isinstance(value, (int, float)): + errors.append(f"{where}: must be a number, got {value!r}") + return + else: # pragma: no cover — a field type this function doesn't know about + errors.append(f"{where}: unsupported field type {hint!r}") + return + + constraint = _CONSTRAINTS.get(name) + if constraint is not None and not constraint[0](value): + errors.append(f"{where}: {constraint[1]}, got {value!r}") + + +def _check_exact_keys(blob: dict, expected: set[str], path: str, errors: list[str]) -> None: + """Require *blob* to carry exactly *expected*. Typos must not pass silently.""" + for missing in sorted(expected - blob.keys()): + errors.append(f"{path}.{missing}: required") + for unknown in sorted(blob.keys() - expected): + errors.append(f"{path}.{unknown}: unknown field") + + +def image_config_from_dict_strict( + blob: Any, + *, + field_path: str = "image_config", +) -> ImageConfig: + """Build an ImageConfig from a caller-supplied blob, rejecting anything odd. + + This is the parser for data arriving over the API. It is deliberately the + opposite of ``_image_config_from_dict``, which merges onto defaults so an + older stored config keeps loading: here every field must be present and + valid, and an unrecognised key is an error rather than something to ignore. + + The difference matters because the two have opposite failure modes. Quietly + keeping a default is right for a config file written by a previous version; + it is wrong for a request, where it would answer 200 while discarding the + setting the user actually asked for — a typo'd field name, an invalid + ``lut_name`` or a nonsensical gamma would all appear to succeed and then + either do nothing or fail later inside a render worker. + + Raises: + ImageConfigError: listing every problem found. + """ + errors: list[str] = [] + if not isinstance(blob, dict): + raise ImageConfigError([f"{field_path}: must be an object, got {type(blob).__name__}"]) + + image_hints = get_type_hints(ImageConfig) + dither_hints = get_type_hints(DitherConfig) + image_names = {f.name for f in fields(ImageConfig)} + dither_names = {f.name for f in fields(DitherConfig)} + + _check_exact_keys(blob, image_names, field_path, errors) + + dither_blob = blob.get("dither") + dither_path = f"{field_path}.dither" + if "dither" in blob and not isinstance(dither_blob, dict): + errors.append(f"{dither_path}: must be an object, got {type(dither_blob).__name__}") + dither_blob = None + elif isinstance(dither_blob, dict): + _check_exact_keys(dither_blob, dither_names, dither_path, errors) + for name in sorted(dither_names & dither_blob.keys()): + _check_value(name, dither_blob[name], dither_hints[name], dither_path, errors) + + for name in sorted((image_names & blob.keys()) - {"dither"}): + _check_value(name, blob[name], image_hints[name], field_path, errors) + + if errors: + raise ImageConfigError(errors) + + assert isinstance(dither_blob, dict) # no errors means every key checked out + return ImageConfig( + dither=DitherConfig(**{n: dither_blob[n] for n in dither_names}), + **{n: blob[n] for n in image_names - {"dither"}}, + ) + + +def parse_crop_to_fill_threshold( + value: Any, *, field_path: str = "crop_to_fill_threshold" +) -> float: + """Validate a crop-to-fill threshold, a fraction of the image's long edge. + + Raises: + ImageConfigError: if it is not a number in [0, 1]. + """ + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise ImageConfigError([f"{field_path}: must be a number, got {value!r}"]) + if not 0.0 <= value <= 1.0: + raise ImageConfigError([f"{field_path}: must be between 0 and 1, got {value!r}"]) + return float(value) + + def _image_config_from_dict( blob: Any, *, diff --git a/python/tests/test_image_config.py b/python/tests/test_image_config.py index 2ec5725d..3bd6fdc6 100644 --- a/python/tests/test_image_config.py +++ b/python/tests/test_image_config.py @@ -2,12 +2,20 @@ from __future__ import annotations -from dataclasses import asdict, replace +from dataclasses import asdict, fields, replace +from typing import get_args import pytest -from hokku.webserver.dither_config import DitherConfig -from hokku.webserver.image_config import ImageConfig, _image_config_from_dict +from hokku.webserver.dither_config import DitherConfig, LutName +from hokku.webserver.dither_streaming import _validate +from hokku.webserver.image_config import ( + ImageConfig, + ImageConfigError, + _image_config_from_dict, + image_config_from_dict_strict, + parse_crop_to_fill_threshold, +) from hokku.webserver.presets import FALLBACK_PRESET, PRESET_IMAGE_CONFIGS @@ -183,3 +191,188 @@ def test_renamed_use_adaptive_saturate_still_honoured(): assert _image_config_from_dict(d).adaptive_saturate_space == "off" d["use_adaptive_saturate"] = True assert _image_config_from_dict(d).adaptive_saturate_space == "cielab" + + +# ── strict parser (API input) ───────────────────────────────────────────────── +# +# The strict parser is the mirror image of the lenient one above: the lenient +# path exists so a stored config from an older version keeps loading, while this +# one exists so a request cannot quietly mean something other than it says. + + +def test_strict_accepts_a_complete_config(): + cfg = _default_image_config() + assert image_config_from_dict_strict(asdict(cfg)) == cfg + + +def test_strict_rejects_a_missing_field_and_names_it(): + d = asdict(_default_image_config()) + d.pop("prepare_gamma") + with pytest.raises(ImageConfigError) as exc: + image_config_from_dict_strict(d) + assert exc.value.errors == ["image_config.prepare_gamma: required"] + + +def test_strict_rejects_an_unknown_field(): + """A typo'd knob must fail, not silently keep the default. + + This is the whole reason the strict parser exists: the lenient parser reads + only the fields it knows, so `prepare_gama` would be dropped on the floor + and the request would answer 200 having changed nothing. + """ + d = asdict(_default_image_config()) + d["prepare_gama"] = 0.9 + with pytest.raises(ImageConfigError) as exc: + image_config_from_dict_strict(d) + assert exc.value.errors == ["image_config.prepare_gama: unknown field"] + + +@pytest.mark.parametrize( + ("field", "value"), + [ + ("adaptive_saturate_space", "lab"), + ("drc_l_space", "oklab2"), + ("drc_chroma_space", ""), + ], +) +def test_strict_rejects_bad_enum_values(field: str, value: str): + d = asdict(_default_image_config()) + d[field] = value + with pytest.raises(ImageConfigError, match=field): + image_config_from_dict_strict(d) + + +@pytest.mark.parametrize(("field", "value"), [("algorithm", "sierra"), ("lut_name", "ciecam")]) +def test_strict_rejects_bad_dither_enum_values(field: str, value: str): + """Caught here rather than inside a render worker. + + dither_streaming._validate() does reject these, but only once the image is + already being converted — the user sees a failed conversion instead of a + rejected setting. + """ + d = asdict(_default_image_config()) + d["dither"][field] = value + with pytest.raises(ImageConfigError, match=f"dither.{field}"): + image_config_from_dict_strict(d) + + +def test_strict_rejects_int_for_bool(): + """bool is a subclass of int, so `serpentine: 1` would otherwise sail through.""" + d = asdict(_default_image_config()) + d["dither"]["serpentine"] = 1 + with pytest.raises(ImageConfigError, match="serpentine"): + image_config_from_dict_strict(d) + + +def test_strict_rejects_bool_for_number(): + d = asdict(_default_image_config()) + d["prepare_gamma"] = True + with pytest.raises(ImageConfigError, match="prepare_gamma"): + image_config_from_dict_strict(d) + + +def test_strict_accepts_int_for_float_field(): + """JSON has one number type — an integral gamma must not be rejected.""" + d = asdict(_default_image_config()) + d["prepare_gamma"] = 1 + assert image_config_from_dict_strict(d).prepare_gamma == 1 + + +@pytest.mark.parametrize( + ("field", "value"), + [ + ("prepare_gamma", 0.0), + ("prepare_midtone", -1.0), + ("prepare_contrast", 0.0), + ("prepare_autocontrast_cutoff", 50.0), + ("prepare_autocontrast_cutoff", -0.1), + ("clahe_clip_limit", -0.5), + ("clahe_keepout_feather", 1.5), + ("dither_noise", -1.0), + ("prepare_usm_amount", -10), + ], +) +def test_strict_rejects_out_of_range_values(field: str, value: float): + d = asdict(_default_image_config()) + d[field] = value + with pytest.raises(ImageConfigError, match=field): + image_config_from_dict_strict(d) + + +def test_strict_rejects_out_of_range_hue_cutoff(): + d = asdict(_default_image_config()) + d["dither"]["hue_cutoff_deg"] = 400.0 + with pytest.raises(ImageConfigError, match="hue_cutoff_deg"): + image_config_from_dict_strict(d) + + +def test_strict_reports_every_problem_at_once(): + """The UI lists the errors, so one round trip must find them all.""" + d = asdict(_default_image_config()) + d.pop("prepare_gamma") + d["nonsense"] = 1 + d["prepare_contrast"] = -1.0 + d["dither"]["lut_name"] = "nope" + + with pytest.raises(ImageConfigError) as exc: + image_config_from_dict_strict(d) + + assert len(exc.value.errors) == 4 + + +@pytest.mark.parametrize("blob", [None, "not a dict", 42, []]) +def test_strict_rejects_non_objects(blob): + with pytest.raises(ImageConfigError, match="must be an object"): + image_config_from_dict_strict(blob) + + +def test_strict_rejects_non_object_dither(): + d = asdict(_default_image_config()) + d["dither"] = "floyd_steinberg" + with pytest.raises(ImageConfigError, match="dither: must be an object"): + image_config_from_dict_strict(d) + + +def test_strict_result_survives_the_lenient_round_trip(): + """The render worker re-parses leniently; that must not alter the config. + + render_worker.render_one() receives asdict(image_config) and rebuilds it + with _image_config_from_dict, so a strict-validated override has to come + back out of that path bit-identical or the rendered image would not match + the settings the cache slug was computed from. + """ + cfg = image_config_from_dict_strict(asdict(_default_image_config())) + assert _image_config_from_dict(asdict(cfg)) == cfg + + +def test_strict_field_coverage_matches_the_dataclass(): + """Guards the reflection: every field is reachable, none silently skipped.""" + d = asdict(_default_image_config()) + assert set(d) == {f.name for f in fields(ImageConfig)} + assert set(d["dither"]) == {f.name for f in fields(DitherConfig)} + + +def test_validate_lut_list_matches_the_literal(): + """dither_streaming._validate used to repeat the LUT names by hand. + + A LUT added to LutName but forgotten there was rejected at render time, so + the two must now be the same list by construction. + """ + for lut in get_args(LutName): + _validate(replace(_default_dither(), lut_name=lut)) + with pytest.raises(ValueError, match="Unknown lut_name"): + _validate(replace(_default_dither(), lut_name="not_a_lut")) # type: ignore[arg-type] + + +# ── crop-to-fill threshold ──────────────────────────────────────────────────── + + +@pytest.mark.parametrize("value", [0, 0.0, 0.25, 1, 1.0]) +def test_parse_crop_threshold_accepts_the_unit_interval(value): + assert parse_crop_to_fill_threshold(value) == pytest.approx(float(value)) + + +@pytest.mark.parametrize("value", [-0.1, 1.1, "0.5", None, True]) +def test_parse_crop_threshold_rejects_everything_else(value): + with pytest.raises(ImageConfigError): + parse_crop_to_fill_threshold(value) From 995601097788558fbb996ae3baacf2f9c7207f94 Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 09:52:32 -0500 Subject: [PATCH 04/15] feat(server): carry per-picture dither and crop overrides on ImageRecord MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent nullable fields, stored in the existing image_manager.json rather than in a new file. _register_new() and from_dict() are the only places an ImageRecord is constructed; every other mutation goes through dataclasses.replace(), so the fields survive Clear Cache & Re-convert, a content change to the source file, retry() and render completion, and are dropped exactly when the image is deleted. That is the whole lifecycle, for free. They are independent on purpose: overriding the pipeline says nothing about the crop, and vice versa. Set = this picture ignores the corresponding automatic choice; None = automatic, which is what every existing record means. No _DB_VERSION bump. from_dict() reads named keys with .get(), so a v4 row without them loads unchanged, and an older binary ignores what it does not know — downgrading drops overrides silently rather than breaking. from_dict() swallows its own parse errors. _load_db() skips any record whose from_dict() raises, so letting a malformed override propagate would discard the image's dimensions, slugs and status as well and force a needless re-render of a picture whose cached output is fine. Losing just the override is the smaller failure, and it is logged. Co-Authored-By: Claude Opus 5 (1M context) --- python/hokku/webserver/image_record.py | 59 ++++++++++++ python/tests/test_image_record.py | 124 +++++++++++++++++++++++++ 2 files changed, 183 insertions(+) create mode 100644 python/tests/test_image_record.py diff --git a/python/hokku/webserver/image_record.py b/python/hokku/webserver/image_record.py index 8a026f94..80451418 100644 --- a/python/hokku/webserver/image_record.py +++ b/python/hokku/webserver/image_record.py @@ -3,10 +3,19 @@ from __future__ import annotations import enum +import logging from dataclasses import asdict, dataclass, field +from hokku.webserver.image_config import ( + ImageConfig, + ImageConfigError, + image_config_from_dict_strict, + parse_crop_to_fill_threshold, +) from hokku.webserver.orientation import Orientation +logger = logging.getLogger(__name__) + # Default model used to migrate pre-v4 records (which only had a single # implicit screen model) into the model-keyed ``slugs`` dict. _LEGACY_MODEL = "huessen_epf1301" @@ -35,6 +44,23 @@ class ImageRecord: image_width: int | None = None # pixel dimensions of the source image image_height: int | None = None + # ── per-picture overrides ──────────────────────────────────────────────── + # User-authored, and the only fields here that are not derived from the + # source file: everything else can be recomputed, these cannot. Set = this + # picture ignores the corresponding automatic choice; None = automatic. + # The two are independent — overriding the pipeline says nothing about the + # crop, and vice versa. + # + # Both feed ScreenImageConfig.cache_slug(), so setting one changes the slug + # and the picture re-renders on its own; nothing else has to invalidate it. + # + # Additive and optional, so _DB_VERSION does not move: a v4 row without them + # loads fine. An older binary reading a newer file ignores what it doesn't + # know, so downgrading drops overrides silently — the pictures revert to + # automatic rather than breaking. + image_config: ImageConfig | None = None # None = the classifier picks + crop_to_fill_threshold: float | None = None # None = AppConfig's global value + def slug_for(self, model: str, orientation: Orientation) -> str | None: """Return the cached slug for (model, orientation), or None if not rendered.""" assert orientation != Orientation.NEUTRAL, "slug_for() requires LANDSCAPE or PORTRAIT" @@ -64,6 +90,36 @@ def matches_orientation_filter(self, orientation: Orientation) -> bool: def to_dict(self) -> dict: return asdict(self) + @staticmethod + def _overrides_from_dict(d: dict) -> tuple[ImageConfig | None, float | None]: + """Parse the two override fields, dropping either one if it is corrupt. + + Errors are swallowed on purpose. _load_db() skips any record whose + from_dict() raises, so letting a malformed override propagate would + cost the whole image — its dimensions, its render slugs, its status — + and force a needless re-render. Losing just the override is the + smaller, more recoverable failure, and it is logged. + """ + image_config: ImageConfig | None = None + raw_cfg = d.get("image_config") + if raw_cfg is not None: + try: + image_config = image_config_from_dict_strict( + raw_cfg, field_path="image_config override" + ) + except ImageConfigError as e: + logger.warning("Dropping malformed dither override for %r: %s", d.get("name"), e) + + crop: float | None = None + raw_crop = d.get("crop_to_fill_threshold") + if raw_crop is not None: + try: + crop = parse_crop_to_fill_threshold(raw_crop) + except ImageConfigError as e: + logger.warning("Dropping malformed crop override for %r: %s", d.get("name"), e) + + return image_config, crop + @classmethod def from_dict(cls, d: dict) -> ImageRecord: raw_t = d.get("last_conversion_seconds") @@ -80,6 +136,7 @@ def from_dict(cls, d: dict) -> ImageRecord: slugs[f"{_LEGACY_MODEL}.{Orientation.LANDSCAPE.value}"] = ls if ps: slugs[f"{_LEGACY_MODEL}.{Orientation.PORTRAIT.value}"] = ps + image_config, crop_to_fill_threshold = cls._overrides_from_dict(d) return cls( name=d["name"], name_hash=d["name_hash"], @@ -93,6 +150,8 @@ def from_dict(cls, d: dict) -> ImageRecord: last_conversion_seconds=float(raw_t) if raw_t is not None else None, image_width=int(raw_w) if raw_w is not None else None, image_height=int(raw_h) if raw_h is not None else None, + image_config=image_config, + crop_to_fill_threshold=crop_to_fill_threshold, ) diff --git a/python/tests/test_image_record.py b/python/tests/test_image_record.py new file mode 100644 index 00000000..0b9d045e --- /dev/null +++ b/python/tests/test_image_record.py @@ -0,0 +1,124 @@ +"""ImageRecord: serialisation of the per-picture overrides. + +The overrides are the only user-authored data in image_manager.json — every +other field is derived from the source file and can be recomputed. These tests +pin the two properties that follow from that: they survive a round trip, and a +corrupt one costs only itself. +""" + +from __future__ import annotations + +from dataclasses import asdict, replace + +import pytest + +from hokku.webserver.image_record import ConvertStatus, ImageRecord +from hokku.webserver.presets import PRESET_IMAGE_CONFIGS + + +def _record(**kwargs) -> ImageRecord: + base = ImageRecord( + name="a.png", + name_hash="0123456789abcd", + original_sha1="deadbeef", + original_size_bytes=1234, + original_mtime=1.0, + added_at=2.0, + convert_status=ConvertStatus.OK, + convert_error=None, + slugs={"huessen_epf1301.landscape": "slug0"}, + last_conversion_seconds=3.5, + image_width=800, + image_height=600, + ) + return replace(base, **kwargs) + + +def test_roundtrip_without_overrides(): + rec = _record() + assert rec.image_config is None + assert rec.crop_to_fill_threshold is None + assert ImageRecord.from_dict(rec.to_dict()) == rec + + +def test_roundtrip_with_both_overrides(): + rec = _record( + image_config=PRESET_IMAGE_CONFIGS["atkinson_hue_aware"], + crop_to_fill_threshold=0.25, + ) + restored = ImageRecord.from_dict(rec.to_dict()) + assert restored == rec + assert restored.image_config == PRESET_IMAGE_CONFIGS["atkinson_hue_aware"] + assert restored.crop_to_fill_threshold == pytest.approx(0.25) + + +def test_the_two_overrides_are_independent(): + """Overriding the pipeline says nothing about the crop, and vice versa.""" + pipeline_only = _record(image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + crop_only = _record(crop_to_fill_threshold=0.4) + + assert ImageRecord.from_dict(pipeline_only.to_dict()).crop_to_fill_threshold is None + assert ImageRecord.from_dict(crop_only.to_dict()).image_config is None + assert ImageRecord.from_dict(crop_only.to_dict()).crop_to_fill_threshold == pytest.approx(0.4) + + +def test_a_row_predating_the_fields_still_loads(): + """No _DB_VERSION bump: a v4 row simply has no override keys.""" + d = _record().to_dict() + del d["image_config"] + del d["crop_to_fill_threshold"] + + restored = ImageRecord.from_dict(d) + + assert restored.image_config is None + assert restored.crop_to_fill_threshold is None + assert restored.image_width == 800 # the rest of the row is intact + + +def test_a_zero_crop_override_is_not_confused_with_absent(): + """0.0 is a real setting — always letterbox, never crop.""" + d = _record(crop_to_fill_threshold=0.0).to_dict() + assert ImageRecord.from_dict(d).crop_to_fill_threshold == 0.0 + + +def test_malformed_dither_override_drops_only_itself(caplog): + """A corrupt override must not cost the whole record. + + _load_db() skips any record whose from_dict() raises, so propagating here + would discard the image's dimensions, slugs and status too, and force a + needless re-render of a picture whose cached output is perfectly good. + """ + d = _record(crop_to_fill_threshold=0.3).to_dict() + d["image_config"] = {"dither": {"algorithm": "nonsense"}} # missing nearly every field + + restored = ImageRecord.from_dict(d) + + assert restored.image_config is None # dropped + assert restored.crop_to_fill_threshold == pytest.approx(0.3) # the other one survives + assert restored.image_width == 800 # and so does the rest of the record + assert restored.slugs == {"huessen_epf1301.landscape": "slug0"} + assert "Dropping malformed dither override" in caplog.text + + +def test_malformed_crop_override_drops_only_itself(caplog): + d = _record(image_config=PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]).to_dict() + d["crop_to_fill_threshold"] = 7.5 # outside [0, 1] + + restored = ImageRecord.from_dict(d) + + assert restored.crop_to_fill_threshold is None + assert restored.image_config == PRESET_IMAGE_CONFIGS["atkinson_hue_aware"] + assert "Dropping malformed crop override" in caplog.text + + +def test_override_is_stored_as_plain_json(): + """to_dict() has to be JSON-ready — asdict recurses into the nested config.""" + d = _record(image_config=PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]).to_dict() + assert isinstance(d["image_config"], dict) + assert isinstance(d["image_config"]["dither"], dict) + assert d["image_config"]["dither"]["algorithm"] == "atkinson" + + +def test_asdict_on_the_record_matches_to_dict(): + rec = _record(image_config=PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]) + assert asdict(rec) == rec.to_dict() From c2136ba7a2f699ec0e0bc18425ea034219301825 Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 09:56:28 -0500 Subject: [PATCH 05/15] feat(server): apply per-picture overrides through one decision funnel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _decision_for_record() asks the classifier for its decision, then overlays whichever of the record's two overrides are set. All three places that needed a decision now go through it, so the invalidation check in _reconcile_with_disk() and the render dispatch in _submit_one() cannot disagree about what a picture should look like. That agreement is what makes setting an override re-render exactly one image and leave every other cached render valid. Applied in the manager rather than in ImageClassifier on purpose. The classifier is a content-observation object wired to AppConfig, with no handle on the record store, and it is rebuilt on every config reload while overrides live in the manager's DB and outlive it — the wrong lifetime. Overlaying on the finished decision also keeps clahe_keepout_bboxes and face_crop_bboxes by construction: an override replaces the *choice* of pipeline, never the observations, so a hand-picked dither cannot cost a portrait its skin-tone protection. Also adds decision_for(detect=False), which answers from cached observations only and never constructs the face detector. The read-only lookup the details UI needs would otherwise be able to load the ~57 MB YuNet graph inside a request thread — the server has four — to produce a result nobody is waiting for. Co-Authored-By: Claude Opus 5 (1M context) --- python/hokku/webserver/image_classifier.py | 32 +++++-- .../hokku/webserver/image_manager_abstract.py | 35 ++++++- python/tests/test_image_classifier.py | 51 ++++++++++ python/tests/test_image_manager.py | 94 +++++++++++++++++++ 4 files changed, 202 insertions(+), 10 deletions(-) diff --git a/python/hokku/webserver/image_classifier.py b/python/hokku/webserver/image_classifier.py index 401e5e51..f3faa820 100644 --- a/python/hokku/webserver/image_classifier.py +++ b/python/hokku/webserver/image_classifier.py @@ -65,6 +65,12 @@ class ImageClassifier: 2. Face detection (if ``classifier_face_detect_enabled``). 3. Default. + A picture carrying a per-picture override outranks all three, but that is + applied by the image manager on top of the decision returned here — see + ``AbstractImageManager._decision_for_record``. Overrides live in the + manager's DB and outlive this object, which is rebuilt on every config + reload, so the classifier deliberately knows nothing about them. + Raw observations (``is_bw``, ``has_face``, ``face_bbox``) are persisted in ``/image_classifier.json`` keyed by sha1 of the original file so re-instantiation after restart doesn't require re-detection. @@ -88,12 +94,20 @@ def __init__(self, config: AppConfig) -> None: # ── Public API ─────────────────────────────────────────────────────────── - def decision_for(self, path: Path, sha1: str) -> ImageClassifierDecision: + def decision_for( + self, path: Path, sha1: str, *, detect: bool = True + ) -> ImageClassifierDecision: """Return the ImageClassifierDecision for this image: dither pipeline, crop policy, and any face keep-out bboxes. + + With ``detect=False`` only cached observations are consulted: nothing is + decoded and the face detector is never constructed. Read-only callers + want this — loading the ~57 MB YuNet graph inside a request would block + one of the server's handful of request threads on work whose result + nobody is waiting for. """ cfg = self._config - chosen, face_bboxes = self._classify(path, sha1) + chosen, face_bboxes = self._classify(path, sha1, detect=detect) keepout = face_bboxes if cfg.classifier_face_detect_clahe_keepout else None crop_bboxes = face_bboxes if cfg.classifier_face_aware_crop_enabled else None return ImageClassifierDecision( @@ -155,8 +169,14 @@ def _check_grayscale(path: Path) -> bool: with Image.open(path) as img: return ImageClassifier._is_near_grayscale(img) - def _classify(self, path: Path, sha1: str) -> tuple[ImageConfig, tuple[BoundingBox, ...]]: - """Return (image_config, face_bboxes) for this image.""" + def _classify( + self, path: Path, sha1: str, *, detect: bool = True + ) -> tuple[ImageConfig, tuple[BoundingBox, ...]]: + """Return (image_config, face_bboxes) for this image. + + ``detect=False`` skips any observation that has not been made yet, + falling through to whatever the cached ones imply. + """ cfg = self._config if not (cfg.classifier_bw_detect_enabled or cfg.classifier_face_detect_enabled): return cfg.image_config_default, () @@ -165,11 +185,11 @@ def _classify(self, path: Path, sha1: str) -> tuple[ImageConfig, tuple[BoundingB obs = self._cache.get(sha1, Observations()) dirty = False - if cfg.classifier_bw_detect_enabled and obs.is_bw is None: + if detect and cfg.classifier_bw_detect_enabled and obs.is_bw is None: obs = replace(obs, is_bw=self._check_grayscale(path)) dirty = True - if cfg.classifier_face_detect_enabled and obs.face_bboxes is None: + if detect and cfg.classifier_face_detect_enabled and obs.face_bboxes is None: if self._face_detector is None: self._face_detector = OpenCVYuNetFaceDetector() bboxes = self._face_detector.detect(path) diff --git a/python/hokku/webserver/image_manager_abstract.py b/python/hokku/webserver/image_manager_abstract.py index 9dd06fe8..2bbf9b1d 100644 --- a/python/hokku/webserver/image_manager_abstract.py +++ b/python/hokku/webserver/image_manager_abstract.py @@ -291,7 +291,7 @@ def sync(self) -> None: if rec_now is None: continue try: - decisions[rec.name] = self._classifier.decision_for(src_path, rec_now.original_sha1) + decisions[rec.name] = self._decision_for_record(src_path, rec_now) except Exception as e: logger.warning("Classification failed for %r: %s", rec.name, e) # Phase 3: dispatch renders with the pre-computed ImageClassifierDecisions. @@ -694,6 +694,33 @@ def _preview_path(self, name_hash: str, slug: str) -> Path: def _thumb_path(self, rec: ImageRecord) -> Path: return self._images_dir / f"{rec.name_hash}{_THUMB_SUFFIX}" + def _decision_for_record( + self, src_path: Path, rec: ImageRecord, *, detect: bool = True + ) -> ImageClassifierDecision: + """The classifier's decision for *rec*, with its per-picture overrides applied. + + Every path that needs a decision goes through here, so the invalidation + check in _reconcile_with_disk() and the render dispatch in _submit_one() + cannot disagree about what a picture should look like — which is what + makes an override re-render exactly one image and nothing else. + + The classifier still runs when an override is set. An override replaces + the *choice* of pipeline, not the observations: face and B&W detection + results are what the CLAHE keep-out and face-aware crop are built from, + and /api/status reports them. Overriding on top of the finished decision + rather than in place of it keeps them by construction. + + ``detect=False`` answers from cached observations only — for read-only + callers that must not pay for (or block on) a first-time detection. + """ + decision = self._classifier.decision_for(src_path, rec.original_sha1, detect=detect) + changes = {} + if rec.image_config is not None: + changes["image_config"] = rec.image_config + if rec.crop_to_fill_threshold is not None: + changes["crop_to_fill_threshold"] = rec.crop_to_fill_threshold + return replace(decision, **changes) if changes else decision + def _load_db(self) -> None: if not self._db_path.exists(): return @@ -808,7 +835,7 @@ def _reconcile_with_disk(self) -> None: continue if existing.convert_status == "ok": - decision = self._classifier.decision_for(src_path, existing.original_sha1) + decision = self._decision_for_record(src_path, existing) # Compare against the LANDSCAPE slug specifically — it's the # lifecycle-primary orientation, and PORTRAIT shares the same # decision so its slug changes in lockstep. @@ -966,8 +993,8 @@ def _submit_one(self, name: str, decision: ImageClassifierDecision | None = None if decision is None: # Fallback: classification failed or was skipped; compute now. with self._db_lock: - original_sha1 = self._records[name].original_sha1 - decision = self._classifier.decision_for(src_path, original_sha1) + rec_now = self._records[name] + decision = self._decision_for_record(src_path, rec_now) # _inflight was already populated by sync() under the lock, so no need # to add here. The assert is a safety net during development. diff --git a/python/tests/test_image_classifier.py b/python/tests/test_image_classifier.py index 29e00ccf..97d253d6 100644 --- a/python/tests/test_image_classifier.py +++ b/python/tests/test_image_classifier.py @@ -202,3 +202,54 @@ def test_screen_config_slug_differs_by_dispatch_outcome(tmp_path): clahe_keepout_bboxes=dec_default.clahe_keepout_bboxes, ) assert sc_bw.cache_slug() != sc_default.cache_slug() + + +# ── detect=False (read-only callers) ───────────────────────────────────────── + + +def test_detect_false_never_builds_the_face_detector(tmp_path): + """A read-only lookup must not load the ~57 MB YuNet graph. + + It would run inside a request thread — the server has only a handful — to + produce a result nobody is waiting on. + """ + cfg = _config(tmp_path, face=True) + clf = ImageClassifier(cfg) + + with patch( + "hokku.webserver.image_classifier.OpenCVYuNetFaceDetector", + side_effect=AssertionError("detector must not be constructed"), + ): + dec = clf.decision_for(_COLOUR_LANDSCAPE, _sha1(_COLOUR_LANDSCAPE), detect=False) + + assert dec.image_config == cfg.image_config_default + assert not dec.clahe_keepout_bboxes + + +def test_detect_false_uses_cached_observations(tmp_path): + """Once observed, a read-only lookup gives the same answer as a full one.""" + cfg = _config(tmp_path, bw=True) + clf = ImageClassifier(cfg) + sha = _sha1(_BW_IMAGE) + + warm = clf.decision_for(_BW_IMAGE, sha) # populates the cache + assert warm.image_config == cfg.image_config_bw + + with patch.object( + ImageClassifier, + "_check_grayscale", + side_effect=AssertionError("must not re-detect"), + ): + cold = clf.decision_for(_BW_IMAGE, sha, detect=False) + + assert cold.image_config == cfg.image_config_bw + + +def test_detect_false_does_not_persist_anything(tmp_path): + """Skipping detection must not write an empty observation to the cache file.""" + cfg = _config(tmp_path, bw=True) + clf = ImageClassifier(cfg) + + clf.decision_for(_COLOUR_LANDSCAPE, _sha1(_COLOUR_LANDSCAPE), detect=False) + + assert not (Path(cfg.cache_dir) / "image_classifier.json").exists() diff --git a/python/tests/test_image_manager.py b/python/tests/test_image_manager.py index 62da1a16..121fb5ec 100644 --- a/python/tests/test_image_manager.py +++ b/python/tests/test_image_manager.py @@ -11,16 +11,21 @@ from __future__ import annotations import json +from dataclasses import replace from io import BytesIO from pathlib import Path +from unittest.mock import patch import pytest from PIL import Image as _Image from hokku.screens.registry import DISPLAY_REGISTRY from hokku.webserver.app_config import AppConfig +from hokku.webserver.bounding_box import BoundingBox +from hokku.webserver.image_classifier import ImageClassifierDecision from hokku.webserver.image_manager_abstract import AbstractImageManager from hokku.webserver.orientation import Orientation +from hokku.webserver.presets import PRESET_IMAGE_CONFIGS from hokku.webserver.screen_image_config import ScreenImageConfig from tests._helpers import make_declared_size_png @@ -171,6 +176,95 @@ def test_db_survives_restart(app_config: AppConfig, image_manager_factory, make_ assert rec2 == rec +def test_override_replaces_the_classifier_choice( + app_config: AppConfig, image_manager_factory, make_test_image +): + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png") + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + rec = mgr.status("a.png") + assert rec is not None + + chosen = PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + overridden = replace(rec, image_config=chosen) + decision = mgr._decision_for_record(upload / "a.png", overridden) + + assert decision.image_config == chosen + + +def test_crop_override_is_independent_of_the_pipeline_override( + app_config: AppConfig, image_manager_factory, make_test_image +): + """Each override applies on its own; neither implies the other.""" + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png") + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + rec = mgr.status("a.png") + assert rec is not None + + auto = mgr._decision_for_record(upload / "a.png", rec) + crop_only = mgr._decision_for_record( + upload / "a.png", replace(rec, crop_to_fill_threshold=0.42) + ) + + assert crop_only.crop_to_fill_threshold == pytest.approx(0.42) + assert crop_only.image_config == auto.image_config # pipeline untouched + + +def test_override_keeps_the_classifier_observations( + app_config: AppConfig, image_manager_factory, make_test_image +): + """An override replaces the pipeline choice, not the detection results. + + The face keep-out boxes and face-aware crop anchors come from detection, so + they have to survive an override or a portrait would lose its skin-tone + protection the moment someone hand-picked a dither for it. + """ + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png") + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + rec = mgr.status("a.png") + assert rec is not None + + bboxes = (BoundingBox(x=0.1, y=0.2, w=0.3, h=0.4),) + stub = ImageClassifierDecision( + image_config=PRESET_IMAGE_CONFIGS["atkinson_hue_aware"], + crop_to_fill_threshold=0.1, + clahe_keepout_bboxes=bboxes, + face_crop_bboxes=bboxes, + ) + with patch.object(mgr._classifier, "decision_for", return_value=stub): + decision = mgr._decision_for_record( + upload / "a.png", + replace(rec, image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]), + ) + + assert decision.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + assert decision.clahe_keepout_bboxes == bboxes + assert decision.face_crop_bboxes == bboxes + + +def test_no_override_leaves_the_decision_untouched( + app_config: AppConfig, image_manager_factory, make_test_image +): + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png") + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + rec = mgr.status("a.png") + assert rec is not None + + direct = mgr._classifier.decision_for(upload / "a.png", rec.original_sha1) + assert mgr._decision_for_record(upload / "a.png", rec) == direct + + def test_retire_does_not_flush(app_config: AppConfig, image_manager_factory, make_test_image): """retire() leaves the DB file alone — its successor already owns it. From 4192a9c1cb51dcaffbdbe5c20d06db4a881281be Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 10:03:22 -0500 Subject: [PATCH 06/15] feat(server): set and clear per-picture overrides, and salvage them on a DB wipe set_overrides() takes each field three-valued - absent means leave it alone, None means back to automatic, a value means use this - so the pipeline and the crop can be edited independently in one request. It returns whether anything changed, so a redundant clear does not throw away a perfectly good render. The mutation marks the record pending and clears its slugs. Both halves are load-bearing. Pending is what actually queues the work: the slug comparison in _reconcile_with_disk() only looks at records that are already "ok", so it is the backstop, not the trigger. Clearing slugs makes panel_bytes_for_model_orientation() return None, so a screen polling during the re-render window gets the usual "try again shortly" 503 instead of one more copy of the image the user just changed. effective_decision() answers what a picture renders with right now, overrides included, from cached observations only - for the read-only UI lookup. _load_db() wipes the whole DB when it meets a version it does not understand. That is right for everything derived: slugs, status, timings and dimensions all come back from the source files. It is wrong for the overrides, which are the only thing in that file nothing can reconstruct. They are now picked out before the wipe and handed back as each file is rediscovered, then whatever is left over is dropped at the end of the reconcile - an override belongs to its file, so deleting the picture and later uploading the same name again must not resurrect it. This path is dead code until _DB_VERSION next moves; it exists so that when it does, the bump costs a re-render rather than the user's work. Co-Authored-By: Claude Opus 5 (1M context) --- .../hokku/webserver/image_manager_abstract.py | 127 +++++++++ python/tests/test_image_manager.py | 254 ++++++++++++++++++ 2 files changed, 381 insertions(+) diff --git a/python/hokku/webserver/image_manager_abstract.py b/python/hokku/webserver/image_manager_abstract.py index 2bbf9b1d..6ebb9781 100644 --- a/python/hokku/webserver/image_manager_abstract.py +++ b/python/hokku/webserver/image_manager_abstract.py @@ -22,6 +22,7 @@ from abc import ABC, abstractmethod from dataclasses import asdict, replace from pathlib import Path +from typing import Any import defusedxml.ElementTree as ET import zstd @@ -31,6 +32,7 @@ from hokku.webserver.app_config import AppConfig from hokku.webserver.filesystem import atomic_write_json from hokku.webserver.image_classifier import ImageClassifier, ImageClassifierDecision +from hokku.webserver.image_config import ImageConfig from hokku.webserver.image_record import ( ConversionProgress, ConvertStatus, @@ -52,6 +54,10 @@ _DB_FILENAME = "image_manager.json" _DB_VERSION = 4 # bump whenever ImageRecord schema changes; v3 auto-migrates (see _load_db) + +# Distinguishes "argument not supplied" from an explicit None, which callers use +# to clear an override. +_UNSET: Any = object() # Reference model that is always rendered and used for previews / lifecycle # bookkeeping. Screens self-report others via set_known_models(). _PRIMARY_MODEL = "huessen_epf1301" @@ -105,6 +111,11 @@ def __init__(self, config: AppConfig, classifier=None) -> None: self._progress = ConversionProgress(current_name=None, done=0, total=0) self._batch_failed: int = 0 + # Overrides rescued from a DB that failed its version check, keyed by + # image name. Drained by _register_new() as each file is rediscovered. + # Populated by _load_db(), so it has to exist before that runs. + self._salvaged_overrides: dict[str, tuple[ImageConfig | None, float | None]] = {} + # Set by shutdown(); silences every later _save_db(). AppState.reload() # builds the replacement manager (which loads the DB) *before* shutting # this one down, and a multi-threaded manager's in-flight render @@ -339,6 +350,80 @@ def remove(self, name: str) -> None: del self._records[name] self._save_db() + def set_overrides( + self, + name: str, + *, + image_config: ImageConfig | object | None = _UNSET, + crop_to_fill_threshold: float | object | None = _UNSET, + ) -> bool: + """Set or clear this picture's per-picture overrides. Queues a re-render. + + Each argument is three-valued: left out means "leave as it is", None + means "back to automatic", and a value means "use this". That is what + lets the two be edited independently from one request. + + Returns True if anything changed. A no-op request is not treated as an + error, but it does not queue a pointless re-render either. + + Raises: + FileNotFoundError: if *name* is not registered. + """ + with self._db_lock: + rec = self._records.get(name) + if rec is None: + raise FileNotFoundError(f"Image {name!r} is not registered.") + if rec.image_width is None: + # PIL couldn't open this; it will never render, so pinning a + # pipeline to it would only produce a failed conversion. + return False + + new_cfg = rec.image_config if image_config is _UNSET else image_config + new_crop = ( + rec.crop_to_fill_threshold + if crop_to_fill_threshold is _UNSET + else crop_to_fill_threshold + ) + if new_cfg == rec.image_config and new_crop == rec.crop_to_fill_threshold: + return False + + logger.info( + "Override for %r: pipeline=%s crop=%s", + name, + "custom" if new_cfg is not None else "automatic", + new_crop if new_crop is not None else "automatic", + ) + # Marking pending is what actually queues the work: + # _reconcile_with_disk()'s slug comparison only looks at records that + # are already "ok", so it is the backstop here, not the trigger. + # + # Clearing slugs matters just as much. It makes + # panel_bytes_for_model_orientation() return None, so a screen that + # polls mid-re-render gets the usual "try again shortly" 503 instead + # of one more copy of the image the user just changed. + self._records[name] = replace( + rec, + image_config=new_cfg, # type: ignore[arg-type] # _UNSET resolved above + crop_to_fill_threshold=new_crop, # type: ignore[arg-type] + convert_status=ConvertStatus.PENDING, + convert_error=None, + slugs={}, + ) + self._save_db() + return True + + def effective_decision(self, name: str) -> ImageClassifierDecision | None: + """What *name* renders with right now, overrides included. + + Read-only and cheap: cached observations only, no detection. Returns + None if the image is not registered. + """ + with self._db_lock: + rec = self._records.get(name) + if rec is None: + return None + return self._decision_for_record(self._upload_dir / name, rec, detect=False) + def retry(self, name: str) -> None: """Mark a failed image as pending so the next sync() retries conversion. @@ -721,6 +806,31 @@ def _decision_for_record( changes["crop_to_fill_threshold"] = rec.crop_to_fill_threshold return replace(decision, **changes) if changes else decision + def _salvage_overrides(self, data: dict) -> None: + """Rescue the user-authored fields from a DB we are about to discard. + + Wiping on a version mismatch is the right call for everything derived — + slugs, status, timings, dimensions all come back from the source files. + The two override fields do not: nothing can reconstruct a dither someone + tuned by hand. They are picked out here and handed back to + _register_new() as each file is rediscovered on disk, so an override for + a picture that has since been deleted is correctly forgotten. + + This is dead code until _DB_VERSION next moves. It exists so that when + it does, the bump is a re-render and not a loss of the user's work. + """ + for name, rec_dict in (data.get("images") or {}).items(): + if not isinstance(rec_dict, dict): + continue + overrides = ImageRecord._overrides_from_dict(rec_dict) + if any(v is not None for v in overrides): + self._salvaged_overrides[name] = overrides + if self._salvaged_overrides: + logger.info( + "Salvaged per-picture overrides for %d image(s) across the DB wipe", + len(self._salvaged_overrides), + ) + def _load_db(self) -> None: if not self._db_path.exists(): return @@ -740,6 +850,7 @@ def _load_db(self) -> None: db_version, _DB_VERSION, ) + self._salvage_overrides(data) return for name, rec_dict in data.get("images", {}).items(): try: @@ -762,6 +873,9 @@ def _save_db(self) -> None: def _register_new(self, name: str, src_path: Path) -> None: st = src_path.stat() w, h, dim_err = self._try_read_image_dims(src_path) + # pop, not get: an override survives only the file it belongs to. One + # left over for a picture no longer on disk is simply dropped. + image_config, crop_to_fill_threshold = self._salvaged_overrides.pop(name, (None, None)) self._records[name] = ImageRecord( name=name, name_hash=self._hash_name(name), @@ -773,6 +887,8 @@ def _register_new(self, name: str, src_path: Path) -> None: convert_error=dim_err, image_width=w, image_height=h, + image_config=image_config, + crop_to_fill_threshold=crop_to_fill_threshold, ) def _reconcile_with_disk(self) -> None: @@ -852,6 +968,17 @@ def _reconcile_with_disk(self) -> None: ) logger.info("ScreenImageConfig slug changed for %r: re-converting", name) + # Every file on disk has now been offered its salvaged override, so + # whatever is left belonged to a picture that is gone. Dropping it here + # keeps "delete the file, lose the override" true: uploading the same + # name again later must not resurrect the old one. + if self._salvaged_overrides: + logger.info( + "Discarding %d salvaged override(s) with no matching file", + len(self._salvaged_overrides), + ) + self._salvaged_overrides.clear() + self._save_db() def _delete_cache_files(self, name_hash: str) -> None: diff --git a/python/tests/test_image_manager.py b/python/tests/test_image_manager.py index 121fb5ec..cb59ccb5 100644 --- a/python/tests/test_image_manager.py +++ b/python/tests/test_image_manager.py @@ -176,6 +176,260 @@ def test_db_survives_restart(app_config: AppConfig, image_manager_factory, make_ assert rec2 == rec +def _synced(app_config: AppConfig, image_manager_factory, make_test_image, name: str = "a.png"): + """A manager with one converted image, ready to be overridden.""" + make_test_image(Path(app_config.upload_dir) / name) + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + return mgr + + +def test_set_overrides_queues_a_rerender( + app_config: AppConfig, image_manager_factory, make_test_image +): + mgr = _synced(app_config, image_manager_factory, make_test_image) + before = mgr.status("a.png") + assert before is not None and before.convert_status == "ok" + assert before.slugs # a render happened + + assert mgr.set_overrides("a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + + rec = mgr.status("a.png") + assert rec is not None + assert rec.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + # Pending is the trigger — _reconcile_with_disk() only slug-checks "ok" + # records, so it is the backstop, not what queues the work. + assert rec.convert_status == "pending" + # Cleared slugs stop a polling screen being served the pre-override render. + assert rec.slugs == {} + + +def test_override_changes_the_render(app_config: AppConfig, image_manager_factory, make_test_image): + """End to end: the new slug is different and the picture re-renders under it.""" + mgr = _synced(app_config, image_manager_factory, make_test_image) + old_slug = mgr.status("a.png").slug_for("huessen_epf1301", Orientation.LANDSCAPE) + + mgr.set_overrides("a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + mgr.sync() + mgr.wait_for_idle() + + rec = mgr.status("a.png") + assert rec is not None + assert rec.convert_status == "ok" + new_slug = rec.slug_for("huessen_epf1301", Orientation.LANDSCAPE) + assert new_slug is not None and new_slug != old_slug + assert mgr.panel_bytes_for_orientation("a.png", Orientation.LANDSCAPE) is not None + + +def test_crop_only_override_changes_the_slug( + app_config: AppConfig, image_manager_factory, make_test_image +): + """The crop override has to reach the cache key, not just the renderer.""" + mgr = _synced(app_config, image_manager_factory, make_test_image) + old_slug = mgr.status("a.png").slug_for("huessen_epf1301", Orientation.LANDSCAPE) + + assert mgr.set_overrides("a.png", crop_to_fill_threshold=0.42) + mgr.sync() + mgr.wait_for_idle() + + rec = mgr.status("a.png") + assert rec is not None + assert rec.image_config is None # pipeline still automatic + assert rec.slug_for("huessen_epf1301", Orientation.LANDSCAPE) != old_slug + + +def test_clearing_an_override_restores_the_automatic_render( + app_config: AppConfig, image_manager_factory, make_test_image +): + """Going back to automatic must land on the original slug again.""" + mgr = _synced(app_config, image_manager_factory, make_test_image) + auto_slug = mgr.status("a.png").slug_for("huessen_epf1301", Orientation.LANDSCAPE) + + mgr.set_overrides("a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + mgr.sync() + mgr.wait_for_idle() + assert mgr.status("a.png").slug_for("huessen_epf1301", Orientation.LANDSCAPE) != auto_slug + + assert mgr.set_overrides("a.png", image_config=None) + mgr.sync() + mgr.wait_for_idle() + + assert mgr.status("a.png").slug_for("huessen_epf1301", Orientation.LANDSCAPE) == auto_slug + + +def test_set_overrides_leaves_unmentioned_fields_alone( + app_config: AppConfig, image_manager_factory, make_test_image +): + """Three-valued arguments: absent means leave, None means clear.""" + mgr = _synced(app_config, image_manager_factory, make_test_image) + mgr.set_overrides( + "a.png", + image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"], + crop_to_fill_threshold=0.3, + ) + + mgr.set_overrides("a.png", crop_to_fill_threshold=None) # clear only the crop + + rec = mgr.status("a.png") + assert rec is not None + assert rec.crop_to_fill_threshold is None + assert rec.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + + +def test_set_overrides_is_a_noop_when_nothing_changes( + app_config: AppConfig, image_manager_factory, make_test_image +): + """A redundant clear must not throw away a perfectly good render.""" + mgr = _synced(app_config, image_manager_factory, make_test_image) + + assert mgr.set_overrides("a.png", image_config=None) is False + + rec = mgr.status("a.png") + assert rec is not None + assert rec.convert_status == "ok" + assert rec.slugs # untouched + + +def test_set_overrides_unknown_image_raises(app_config: AppConfig, image_manager_factory): + mgr = image_manager_factory(app_config) + with pytest.raises(FileNotFoundError): + mgr.set_overrides("nope.png", crop_to_fill_threshold=0.5) + + +def test_effective_decision_reports_the_override( + app_config: AppConfig, image_manager_factory, make_test_image +): + mgr = _synced(app_config, image_manager_factory, make_test_image) + assert mgr.effective_decision("nope.png") is None + + auto = mgr.effective_decision("a.png") + assert auto is not None + + mgr.set_overrides("a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + after = mgr.effective_decision("a.png") + + assert after is not None + assert after.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + assert auto.image_config != after.image_config + + +def test_overrides_survive_clear_caches( + app_config: AppConfig, image_manager_factory, make_test_image +): + mgr = _synced(app_config, image_manager_factory, make_test_image) + mgr.set_overrides( + "a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"], crop_to_fill_threshold=0.2 + ) + + mgr.clear_caches() + + rec = mgr.status("a.png") + assert rec is not None + assert rec.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + assert rec.crop_to_fill_threshold == pytest.approx(0.2) + + +def test_overrides_survive_a_content_change( + app_config: AppConfig, image_manager_factory, make_test_image +): + """Re-saving a picture keeps the tuning done for it — it is the same picture.""" + upload = Path(app_config.upload_dir) + make_test_image(upload / "a.png", color=(255, 0, 0)) + mgr = image_manager_factory(app_config) + mgr.sync() + mgr.wait_for_idle() + mgr.set_overrides("a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + + make_test_image(upload / "a.png", color=(0, 0, 255)) + mgr.sync() + mgr.wait_for_idle() + + rec = mgr.status("a.png") + assert rec is not None + assert rec.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + + +def test_overrides_survive_a_restart(app_config: AppConfig, image_manager_factory, make_test_image): + mgr = _synced(app_config, image_manager_factory, make_test_image) + mgr.set_overrides("a.png", crop_to_fill_threshold=0.35) + mgr.shutdown() + + mgr2 = image_manager_factory(app_config) + + rec = mgr2.status("a.png") + assert rec is not None + assert rec.crop_to_fill_threshold == pytest.approx(0.35) + + +def test_deleting_the_image_drops_the_override( + app_config: AppConfig, image_manager_factory, make_test_image +): + upload = Path(app_config.upload_dir) + mgr = _synced(app_config, image_manager_factory, make_test_image) + mgr.set_overrides("a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + + mgr.remove("a.png") + make_test_image(upload / "a.png") + mgr.sync() + mgr.wait_for_idle() + + rec = mgr.status("a.png") + assert rec is not None + assert rec.image_config is None + + +def test_overrides_are_salvaged_across_a_db_version_wipe( + app_config: AppConfig, image_manager_factory, make_test_image +): + """A future _DB_VERSION bump must cost a re-render, not the user's tuning. + + Everything derived is meant to be discarded by the wipe; the overrides are + the only thing in that file nothing can reconstruct. + """ + mgr = _synced(app_config, image_manager_factory, make_test_image) + mgr.set_overrides( + "a.png", image_config=PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"], crop_to_fill_threshold=0.2 + ) + mgr.shutdown() + + db_path = Path(app_config.cache_dir) / "image_manager.json" + db = json.loads(db_path.read_text()) + db["version"] = 99 # a version this build does not understand + db_path.write_text(json.dumps(db)) + + mgr2 = image_manager_factory(app_config) + mgr2.sync() + mgr2.wait_for_idle() + + rec = mgr2.status("a.png") + assert rec is not None + assert rec.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + assert rec.crop_to_fill_threshold == pytest.approx(0.2) + + +def test_salvaged_override_for_a_deleted_file_is_forgotten( + app_config: AppConfig, image_manager_factory, make_test_image +): + """An override belongs to its file. No file, no override to restore.""" + mgr = _synced(app_config, image_manager_factory, make_test_image) + mgr.set_overrides("a.png", crop_to_fill_threshold=0.2) + mgr.shutdown() + + db_path = Path(app_config.cache_dir) / "image_manager.json" + db = json.loads(db_path.read_text()) + db["version"] = 99 + db_path.write_text(json.dumps(db)) + (Path(app_config.upload_dir) / "a.png").unlink() + + mgr2 = image_manager_factory(app_config) + mgr2.sync() + mgr2.wait_for_idle() + + assert mgr2.status("a.png") is None + assert mgr2._salvaged_overrides == {} + + def test_override_replaces_the_classifier_choice( app_config: AppConfig, image_manager_factory, make_test_image ): From 481cd7a7101d972ae9a21af918c451e678ea42f4 Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 10:10:30 -0500 Subject: [PATCH 07/15] feat(server): expose per-picture overrides over HTTP, and fix the preview crop GET/PATCH /hokku/api/image//config. PATCH because the two overrides are independent: an absent key is left alone and an explicit null clears that one, so the same route sets either, clears either, or does both at once. The body is fully validated before anything is written, because a half-applied override would leave a picture rendering with settings the user never approved and there is no undo for that. GET also returns the effective config, so the editor opens on the picture as it looks now rather than on an arbitrary preset. It runs no detection. Re-rendering is requested by waking the watcher rather than by calling sync() inline as the neighbouring routes do. On the default single-threaded manager sync() renders on the calling thread, which would hold one of the server's four request threads for a full conversion - seconds on a Pi. Hence the "queued" in the response. /api/status gains has_image_config_override, crop_to_fill_threshold and a pipeline label, and deliberately not the ImageConfig itself: status is polled for the whole library every few seconds, and a 24-field blob per picture would add a few hundred KB per poll on a large one. The editor fetches the full config for the single picture it is opening. The preview endpoint now parses strictly too. Rendering a preview that quietly differs from the form the user is looking at is worse than refusing: the lenient parser kept a default for an unreadable knob and rendered anyway. It also takes crop_to_fill_threshold and max_side_px. The first fixes a live bug: render_preview_png was called positionally, so the threshold took its 0.0 default and every preview letterboxed, while transform_bboxes_to_canvas_norm on the next line was passed the configured value - so whenever crop-to-fill was active the face-box overlay was computed against geometry the returned PNG did not have. Both now use the same number. max_side_px is for the compare-presets grid. Previews are finally bounded by a semaphore. Their cost is dominated by decoding the full source image, not by the dither, so an unbounded endpoint lets a looping client or a parallel compare grid hold one decoded image per request thread and starve the screen-serving path. Co-Authored-By: Claude Opus 5 (1M context) --- python/hokku/webserver/flask_app.py | 231 ++++++++++++++++++++--- python/tests/test_flask_api.py | 283 +++++++++++++++++++++++++++- 2 files changed, 492 insertions(+), 22 deletions(-) diff --git a/python/hokku/webserver/flask_app.py b/python/hokku/webserver/flask_app.py index 739d2d10..0e49fcf7 100644 --- a/python/hokku/webserver/flask_app.py +++ b/python/hokku/webserver/flask_app.py @@ -19,6 +19,7 @@ from datetime import datetime from importlib.metadata import version as _pkg_version from pathlib import Path +from typing import Any import pillow_jxl # noqa: F401 — PIL plugin registration import psutil @@ -49,8 +50,13 @@ from hokku.webserver.dither_streaming_numba import NumbaStreamingDither from hokku.webserver.firmware_library import FirmwareStore from hokku.webserver.image_abc import transform_bboxes_to_canvas_norm -from hokku.webserver.image_config import _image_config_from_dict -from hokku.webserver.image_record import ConvertStatus +from hokku.webserver.image_classifier import Observations +from hokku.webserver.image_config import ( + ImageConfigError, + image_config_from_dict_strict, + parse_crop_to_fill_threshold, +) +from hokku.webserver.image_record import ConvertStatus, ImageRecord from hokku.webserver.image_renderer import ( IMAGE_EXTENSIONS, MAX_UPLOAD_PIXELS, @@ -170,6 +176,21 @@ def _read_git_describe() -> tuple[str, str | None]: OTA_NVS_MAX_CONCURRENT_BUILDS = 4 _nvs_build_slots = threading.BoundedSemaphore(OTA_NVS_MAX_CONCURRENT_BUILDS) +# Bound concurrent one-off dither previews. Each decodes the full source image — +# up to the decode budget, tens of MB — and that decode, not the dither, +# dominates the cost. Unbounded, a client looping the endpoint or a compare grid +# fired in parallel would hold one decoded image per request thread and starve +# the screen-serving path. Excess requests get a 503 and can retry. +PREVIEW_MAX_CONCURRENT = 2 +_preview_slots = threading.BoundedSemaphore(PREVIEW_MAX_CONCURRENT) + +# Preview canvas bounds, in pixels on the long edge. The default matches +# ImageRenderer's own; the floor keeps a caller from asking for something too +# small to judge. Note that lowering it speeds up the resize and the dither but +# not the decode, so the saving is real but modest. +_PREVIEW_MAX_SIDE_PX = 800 +_PREVIEW_MIN_SIDE_PX = 200 + # Upper byte-bounds on the USB-flash config string fields (in addition to the # 64-byte screen_name cap enforced inline). SSID: 802.11 max; PSK: WPA2 max; URL: # the firmware's server-URL buffer. Rejected with 400 before the NVS generator. @@ -186,6 +207,41 @@ def _busy_retry_seconds(config: AppConfig) -> int: return min(300, calculate_sleep_seconds(config)) +def _request_sync(state: AppState) -> None: + """Ask for a sync as soon as possible, without blocking this request. + + Deliberately not ``manager.sync()``: on the default single-threaded manager + that renders inline, so the caller would hold one of the server's four + request threads for the length of a full conversion — seconds on a Pi. Waking + the watcher hands the work to the thread that owns the sync cadence and + returns immediately, which is why these endpoints answer "queued" rather + than "done". + + Falls back to an inline sync when there is no watcher (tests, embedded use), + where being synchronous is what the caller wants anyway. + """ + if state.watcher is not None: + state.watcher.wake() + else: + state.manager.sync() + + +def _pipeline_label(rec: ImageRecord, obs: Observations | None) -> str: + """Which pipeline a picture renders through, for the UI to show as a chip. + + Mirrors the dispatch order in ImageClassifier plus the override that + outranks it, so the UI can say *why* a picture looks the way it does + without re-deriving the policy. + """ + if rec.image_config is not None: + return "override" + if obs is not None and obs.is_bw: + return "bw" + if obs is not None and obs.face_bboxes: + return "face" + return "default" + + def create_app( state: AppState, *, @@ -614,6 +670,93 @@ def api_retry(name: str): return jsonify({"error": f"image {name!r} not found"}), 404 return jsonify({"ok": True}) + @app.route("/hokku/api/image//config", methods=["GET"]) + def api_image_config_get(name: str): + """This picture's overrides, and what it actually renders with. + + ``effective`` is what the details UI opens its editor on, so tuning + starts from the picture as it looks now rather than from an arbitrary + preset. Cheap by construction: no detection is run. + """ + rec = state.manager.status(name) + if rec is None: + logger.info("Image config: image %r not found", name) + return jsonify({"error": f"image {name!r} not found"}), 404 + + decision = state.manager.effective_decision(name) + obs = state.classifier.observations_for(rec.original_sha1) if rec.original_sha1 else None + return jsonify( + { + "overrides": { + "image_config": asdict(rec.image_config) if rec.image_config else None, + "crop_to_fill_threshold": rec.crop_to_fill_threshold, + }, + "effective": { + "image_config": asdict(decision.image_config), + "crop_to_fill_threshold": decision.crop_to_fill_threshold, + } + if decision is not None + else None, + "pipeline": _pipeline_label(rec, obs), + } + ) + + @app.route("/hokku/api/image//config", methods=["PATCH"]) + def api_image_config_patch(name: str): + """Set or clear this picture's dither and/or crop override. + + PATCH because the two fields are independent: a key that is absent is + left alone, and an explicit null clears that one override. So the same + route covers setting either, clearing either, and doing both at once. + + Nothing is written unless the whole body validates — a rejected request + must leave the picture exactly as it was. + """ + body = request.get_json(silent=True) + if not isinstance(body, dict): + logger.info("Image config: expected JSON object, got %r", type(body).__name__) + return jsonify({"error": "expected JSON object"}), 400 + + unknown = body.keys() - {"image_config", "crop_to_fill_threshold"} + if unknown: + return jsonify( + {"error": f"unknown field(s): {', '.join(sorted(unknown))}"}, + ), 400 + + rec = state.manager.status(name) + if rec is None: + logger.info("Image config: image %r not found", name) + return jsonify({"error": f"image {name!r} not found"}), 404 + if rec.image_width is None: + return jsonify( + {"error": f"image {name!r} cannot be rendered, so it cannot be configured"} + ), 409 + + changes: dict[str, Any] = {} + try: + if "image_config" in body: + raw = body["image_config"] + changes["image_config"] = ( + None if raw is None else image_config_from_dict_strict(raw) + ) + if "crop_to_fill_threshold" in body: + raw = body["crop_to_fill_threshold"] + changes["crop_to_fill_threshold"] = ( + None if raw is None else parse_crop_to_fill_threshold(raw) + ) + except ImageConfigError as e: + logger.info("Image config: rejected override for %r: %s", name, e) + return jsonify({"error": str(e), "errors": e.errors}), 400 + + try: + queued = state.manager.set_overrides(name, **changes) + except FileNotFoundError: + return jsonify({"error": f"image {name!r} not found"}), 404 + + if queued: + _request_sync(state) + return jsonify({"ok": True, "queued": queued}) + @app.route("/hokku/api/show_next/", methods=["POST"]) def api_show_next(name: str): rec = state.manager.status(name) @@ -899,6 +1042,15 @@ def api_status(): "face_bboxes": [[b.x, b.y, b.w, b.h] for b in obs.face_bboxes] if (obs and obs.face_bboxes) else [], + # Enough for the grid chip and for the modal to decide whether + # it is showing an override. Deliberately NOT the ImageConfig + # itself: this runs for the whole library on every poll, and a + # 24-field blob per picture would be a few hundred KB a poll on + # a large library. The editor fetches the full config for the + # one picture it is opening. + "has_image_config_override": r.image_config is not None, + "crop_to_fill_threshold": r.crop_to_fill_threshold, + "pipeline": _pipeline_label(r, obs), } upload_files.append(entry) if r.convert_status == ConvertStatus.FAILED: @@ -1108,8 +1260,11 @@ def api_dither_preview(): """Render a one-off dithered preview for a given image + image_config. Body: {name: str, image: ImageConfig dict, clahe_keepout?: bool, - face_aware_crop?: bool}. Returns PNG bytes. ``clahe_keepout`` and - ``face_aware_crop`` default to the saved config when omitted. + face_aware_crop?: bool, crop_to_fill_threshold?: float, + max_side_px?: int}. Returns PNG bytes. ``clahe_keepout``, + ``face_aware_crop`` and ``crop_to_fill_threshold`` default to the saved + config when omitted; ``max_side_px`` trades preview detail for speed and + is what the compare-presets grid uses for its thumbnails. The ``X-Face-Bboxes`` response header carries face bboxes already transformed into the rendered preview's coordinate space (JSON list of [x, y, w, h] tuples, each normalised 0..1 against the preview @@ -1135,11 +1290,29 @@ def api_dither_preview(): except FileNotFoundError: logger.info("Dither preview: image %r not found", name) return jsonify({"error": f"image {name!r} not found"}), 404 + # Strict, unlike the stored-config path: a preview the user cannot trust + # to be the config they typed is worse than no preview. The lenient + # parser would keep a default for a misspelled knob and render something + # subtly different from what is on screen. try: - cfg = _image_config_from_dict(image_blob) - except (TypeError, ValueError) as e: + cfg = image_config_from_dict_strict(image_blob, field_path="image") + except ImageConfigError as e: logger.info("Dither preview: invalid image config: %s", e) - return jsonify({"error": f"invalid image config: {e}"}), 400 + return jsonify({"error": str(e), "errors": e.errors}), 400 + + crop_threshold = state.config.crop_to_fill_threshold + if body.get("crop_to_fill_threshold") is not None: + try: + crop_threshold = parse_crop_to_fill_threshold(body["crop_to_fill_threshold"]) + except ImageConfigError as e: + return jsonify({"error": str(e), "errors": e.errors}), 400 + + max_side_px = _PREVIEW_MAX_SIDE_PX + if body.get("max_side_px") is not None: + raw_side = body["max_side_px"] + if isinstance(raw_side, bool) or not isinstance(raw_side, int): + return jsonify({"error": "max_side_px must be a whole number"}), 400 + max_side_px = max(_PREVIEW_MIN_SIDE_PX, min(_PREVIEW_MAX_SIDE_PX, raw_side)) # Look up cached face bboxes (original-image normalised) so we can map # them onto the rendered preview's coordinate space below. @@ -1169,20 +1342,36 @@ def api_dither_preview(): if native != Orientation.NEUTRAL: render_orientation = native - logger.debug("Preview: %r", name) - with open_image_for_render(path) as img: - orig_w, orig_h = img.size - png = ImageRenderer( - NumbaStreamingDither(preview_display), preview_display - ).render_preview_png( - img, - cfg, - render_orientation, - clahe_keepout_bboxes_norm=keepout, - crop_anchor_bboxes_norm=crop_anchor, - ) - logger.debug("Preview done: %r", name) + # Previews decode the full source image, which dominates their cost and + # is bounded only by the decode budget. Without a cap, a client looping + # this endpoint (or a compare grid firing in parallel) would hold one + # decoded image per request thread and starve the screen-serving path. + if not _preview_slots.acquire(blocking=False): + logger.info("Dither preview: refused, %d already rendering", PREVIEW_MAX_CONCURRENT) + return jsonify({"error": "busy rendering previews, retry shortly"}), 503 + try: + logger.debug("Preview: %r", name) + with open_image_for_render(path) as img: + orig_w, orig_h = img.size + png = ImageRenderer( + NumbaStreamingDither(preview_display), preview_display + ).render_preview_png( + img, + cfg, + render_orientation, + max_side_px=max_side_px, + crop_to_fill_threshold=crop_threshold, + clahe_keepout_bboxes_norm=keepout, + crop_anchor_bboxes_norm=crop_anchor, + ) + logger.debug("Preview done: %r", name) + finally: + _preview_slots.release() + # Same threshold the render above used. These two used to disagree: the + # render took the 0.0 default because the argument was positional, so it + # always letterboxed, while the overlay was computed for a cover-cropped + # canvas the returned PNG did not have. canvas_bboxes = transform_bboxes_to_canvas_norm( face_bboxes_orig, orig_w, @@ -1190,7 +1379,7 @@ def api_dither_preview(): render_orientation, preview_display.panel_w, preview_display.panel_h, - state.config.crop_to_fill_threshold, + crop_threshold, panel_rotated=preview_display.panel_rotated, crop_anchor_bboxes_norm=crop_anchor, ) diff --git a/python/tests/test_flask_api.py b/python/tests/test_flask_api.py index a6a615e2..a4d871af 100644 --- a/python/tests/test_flask_api.py +++ b/python/tests/test_flask_api.py @@ -31,7 +31,7 @@ from hokku.webserver.app_config import AppConfig from hokku.webserver.app_state import AppState, build_manager -from hokku.webserver.flask_app import create_app +from hokku.webserver.flask_app import PREVIEW_MAX_CONCURRENT, _preview_slots, create_app from hokku.webserver.image_classifier import ImageClassifier from hokku.webserver.presets import PRESET_IMAGE_CONFIGS from hokku.webserver.serve_scheduler import ServeScheduler @@ -441,6 +441,113 @@ def test_dither_preview_non_json_body_returns_400(bare_client): assert resp.status_code == 400 +def test_dither_preview_rejects_a_malformed_config(synced_client): + """Previewing something other than what the user typed is worse than failing. + + The lenient parser this used to call kept a default for an unreadable knob + and rendered anyway, so the preview silently disagreed with the form. + """ + client, _, name = synced_client + img_cfg = asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]) + img_cfg["dither"]["lut_name"] = "not_a_lut" + + resp = client.post("/hokku/api/dither/preview", json={"name": name, "image": img_cfg}) + + assert resp.status_code == 400 + assert any("lut_name" in e for e in resp.get_json()["errors"]) + + +def test_dither_preview_honours_a_crop_threshold(synced_client): + """The rendered PNG has to follow the requested crop. + + It did not: render_preview_png was called positionally, so the threshold + took its 0.0 default and every preview letterboxed, while the face-box + overlay was computed for a cover-cropped canvas the PNG never had. + """ + client, _, name = synced_client + img_cfg = asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]) + + letterboxed = client.post( + "/hokku/api/dither/preview", + json={"name": name, "image": img_cfg, "crop_to_fill_threshold": 0.0}, + ) + cropped = client.post( + "/hokku/api/dither/preview", + json={"name": name, "image": img_cfg, "crop_to_fill_threshold": 1.0}, + ) + + assert letterboxed.status_code == cropped.status_code == 200 + # The fixture is a 1200x300 bar against a 4:3 panel, so cover-cropping it + # produces a visibly different image from letterboxing it. + assert letterboxed.data != cropped.data + + +def test_dither_preview_rejects_a_bad_crop_threshold(synced_client): + client, _, name = synced_client + resp = client.post( + "/hokku/api/dither/preview", + json={ + "name": name, + "image": asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]), + "crop_to_fill_threshold": 5, + }, + ) + assert resp.status_code == 400 + + +def test_dither_preview_max_side_px_shrinks_the_png(synced_client): + """The compare grid asks for smaller tiles.""" + client, _, name = synced_client + img_cfg = asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]) + + big = client.post("/hokku/api/dither/preview", json={"name": name, "image": img_cfg}) + small = client.post( + "/hokku/api/dither/preview", + json={"name": name, "image": img_cfg, "max_side_px": 200}, + ) + + assert big.status_code == small.status_code == 200 + assert len(small.data) < len(big.data) + + +def test_dither_preview_rejects_a_non_integer_max_side(synced_client): + client, _, name = synced_client + resp = client.post( + "/hokku/api/dither/preview", + json={ + "name": name, + "image": asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]), + "max_side_px": "big", + }, + ) + assert resp.status_code == 400 + + +def test_dither_preview_refuses_when_all_slots_are_busy(synced_client): + """Previews are decode-bound; unbounded they would starve the screen path.""" + client, _, name = synced_client + acquired = [_preview_slots.acquire(blocking=False) for _ in range(PREVIEW_MAX_CONCURRENT)] + try: + assert all(acquired) + resp = client.post( + "/hokku/api/dither/preview", + json={"name": name, "image": asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"])}, + ) + assert resp.status_code == 503 + finally: + for _ in acquired: + _preview_slots.release() + + +def test_dither_preview_releases_its_slot(synced_client): + """Two sequential previews must both succeed.""" + client, _, name = synced_client + img_cfg = asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]) + for _ in range(PREVIEW_MAX_CONCURRENT + 1): + resp = client.post("/hokku/api/dither/preview", json={"name": name, "image": img_cfg}) + assert resp.status_code == 200 + + # ── /hokku/api/thumbnail/ GET ────────────────────────────────────────── @@ -597,6 +704,180 @@ def test_screen_mac_is_durable_key_across_rename(bare_client): # ── navigation ──────────────────────────────────────────────────────────────── +# ── /hokku/api/image//config — per-picture overrides ──────────────────── + + +def _override_body(**kwargs) -> dict: + return kwargs + + +def test_image_config_get_reports_automatic(synced_client): + """With nothing overridden: no overrides, but a usable effective config.""" + client, _, name = synced_client + resp = client.get(f"/hokku/api/image/{name}/config") + + assert resp.status_code == 200 + body = resp.get_json() + assert body["overrides"] == {"image_config": None, "crop_to_fill_threshold": None} + assert body["effective"]["image_config"]["dither"]["algorithm"] + assert body["pipeline"] in ("default", "bw", "face") + + +def test_image_config_get_unknown_image_404(bare_client): + client, _ = bare_client + assert client.get("/hokku/api/image/ghost.jpg/config").status_code == 404 + + +def test_patch_sets_the_pipeline_override(synced_client): + client, state, name = synced_client + cfg = asdict(PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + + resp = client.patch(f"/hokku/api/image/{name}/config", json=_override_body(image_config=cfg)) + + assert resp.status_code == 200 + assert resp.get_json() == {"ok": True, "queued": True} + rec = state.manager.status(name) + assert rec.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + assert rec.crop_to_fill_threshold is None # untouched + assert rec.convert_status == "pending" + + +def test_patch_sets_the_crop_override_alone(synced_client): + client, state, name = synced_client + + resp = client.patch( + f"/hokku/api/image/{name}/config", json=_override_body(crop_to_fill_threshold=0.3) + ) + + assert resp.status_code == 200 + rec = state.manager.status(name) + assert rec.crop_to_fill_threshold == pytest.approx(0.3) + assert rec.image_config is None # pipeline still automatic + + +def test_patch_clears_one_override_with_an_explicit_null(synced_client): + """Absent means leave alone; null means clear. Both in one route.""" + client, state, name = synced_client + client.patch( + f"/hokku/api/image/{name}/config", + json=_override_body( + image_config=asdict(PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]), + crop_to_fill_threshold=0.3, + ), + ) + + resp = client.patch( + f"/hokku/api/image/{name}/config", json=_override_body(crop_to_fill_threshold=None) + ) + + assert resp.status_code == 200 + rec = state.manager.status(name) + assert rec.crop_to_fill_threshold is None + assert rec.image_config == PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"] + + +def test_patch_reports_a_noop_as_not_queued(synced_client): + client, _, name = synced_client + resp = client.patch(f"/hokku/api/image/{name}/config", json=_override_body(image_config=None)) + assert resp.status_code == 200 + assert resp.get_json() == {"ok": True, "queued": False} + + +def test_patch_with_a_bad_lut_leaves_the_record_untouched(synced_client): + """A rejected request must change nothing at all. + + Half-applying an override would leave the picture rendering with settings + the user never approved, and there is no undo for that. + """ + client, state, name = synced_client + before = state.manager.status(name) + cfg = asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]) + cfg["dither"]["lut_name"] = "not_a_lut" + + resp = client.patch(f"/hokku/api/image/{name}/config", json=_override_body(image_config=cfg)) + + assert resp.status_code == 400 + assert any("lut_name" in e for e in resp.get_json()["errors"]) + assert state.manager.status(name) == before + + +def test_patch_rejects_a_typo_field(synced_client): + client, state, name = synced_client + before = state.manager.status(name) + cfg = asdict(PRESET_IMAGE_CONFIGS["atkinson_hue_aware"]) + cfg["prepare_gama"] = 0.9 + + resp = client.patch(f"/hokku/api/image/{name}/config", json=_override_body(image_config=cfg)) + + assert resp.status_code == 400 + assert state.manager.status(name) == before + + +def test_patch_rejects_unknown_top_level_fields(synced_client): + client, _, name = synced_client + resp = client.patch(f"/hokku/api/image/{name}/config", json={"orientation": "landscape"}) + assert resp.status_code == 400 + + +def test_patch_rejects_an_out_of_range_crop(synced_client): + client, state, name = synced_client + before = state.manager.status(name) + + resp = client.patch( + f"/hokku/api/image/{name}/config", json=_override_body(crop_to_fill_threshold=2.0) + ) + + assert resp.status_code == 400 + assert state.manager.status(name) == before + + +def test_patch_unknown_image_404(bare_client): + client, _ = bare_client + resp = client.patch("/hokku/api/image/ghost.jpg/config", json={"crop_to_fill_threshold": 0.1}) + assert resp.status_code == 404 + + +def test_patch_non_object_body_400(synced_client): + client, _, name = synced_client + resp = client.patch( + f"/hokku/api/image/{name}/config", data="nope", content_type="application/json" + ) + assert resp.status_code == 400 + + +def test_get_reports_the_override_after_a_patch(synced_client): + client, _, name = synced_client + cfg = asdict(PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]) + client.patch(f"/hokku/api/image/{name}/config", json=_override_body(image_config=cfg)) + + body = client.get(f"/hokku/api/image/{name}/config").get_json() + + assert body["overrides"]["image_config"] == cfg + assert body["effective"]["image_config"] == cfg + assert body["pipeline"] == "override" + + +def test_status_exposes_the_override_summary(synced_client): + """Cheap fields only — the blob itself is fetched per picture on demand.""" + client, _, name = synced_client + client.patch( + f"/hokku/api/image/{name}/config", + json=_override_body( + image_config=asdict(PRESET_IMAGE_CONFIGS["floyd_steinberg_bw"]), + crop_to_fill_threshold=0.25, + ), + ) + + entry = next( + e for e in client.get("/hokku/api/status").get_json()["upload_files"] if e["name"] == name + ) + + assert entry["has_image_config_override"] is True + assert entry["crop_to_fill_threshold"] == pytest.approx(0.25) + assert entry["pipeline"] == "override" + assert "image_config" not in entry # the 24-field blob must stay out of the poll + + def test_root_redirects_to_ui(bare_client): client, _ = bare_client resp = client.get("/", follow_redirects=False) From c9760e98b5921121070f0ac02591d1954c4d139f Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 10:37:01 -0500 Subject: [PATCH 08/15] refactor(server): parse ImageConfig strictly, and migrate old shapes on the chain There were two ImageConfig parsers: a lenient one that merged whatever was stored onto the pipeline defaults on every single load, and the strict one added for API input. The lenient one was carrying upgrade knowledge in the wrong place, and paying for it on every load forever: - a config could stay permanently incomplete, because nothing ever told it to be complete; - a misspelled knob was silently ignored on every load rather than reported once - the parser only ever read the fields it knew about; - enum values were never checked, so a bad lut_name was accepted and only rejected much later inside a render worker, surfacing as a failed conversion instead of a bad setting. Config shape changes are what the migration chain is for, and it already exists. So: v9 -> v10 completes the three stored image_config blobs once - filling absent fields from that pipeline's default, translating the use_adaptive_saturate rename, dropping keys that are no longer fields - and from then on every caller parses strictly. Any future change to ImageConfig's shape gets its own migration for the same reason. The lenient parser is gone. complete_image_config_blob() is what is left of it, dict-in/dict-out, called only from the migration. All five callers now validate: AppConfig.from_dict, the render worker's IPC payload, ScreenImageConfig's round-trip, the API, and the per-picture overrides. Enums are checked everywhere as a result. Two deliberate behaviour changes: - a config that claims the current version but carries an incomplete blob is now a startup error rather than being quietly repaired. An absent key is still fine and takes the pipeline default, like every other field - a hand-edited config that omits a section must not stop the server booting. - a config from a NEWER version is refused with a message saying so. There is no downgrade path, and without this the strict parser would reject an unknown field and report a confusing validation error instead of the real problem. Co-Authored-By: Claude Opus 5 (1M context) --- python/hokku/webserver/app_config.py | 74 +++++--- .../webserver/config/config.json.example | 2 +- python/hokku/webserver/image_config.py | 106 +++++------- python/hokku/webserver/render_worker.py | 11 +- python/hokku/webserver/screen_image_config.py | 4 +- python/tests/test_app_config.py | 73 +++++++- python/tests/test_image_config.py | 160 ++++++++++-------- 7 files changed, 260 insertions(+), 170 deletions(-) diff --git a/python/hokku/webserver/app_config.py b/python/hokku/webserver/app_config.py index 60145b35..a6196bcb 100644 --- a/python/hokku/webserver/app_config.py +++ b/python/hokku/webserver/app_config.py @@ -18,7 +18,8 @@ from hokku.webserver.filesystem import atomic_write_json from hokku.webserver.image_config import ( ImageConfig, - _image_config_from_dict, + complete_image_config_blob, + image_config_from_dict_strict, ) from hokku.webserver.presets import ( DEFAULT_BW_IMAGE_CONFIG, @@ -28,7 +29,7 @@ logger = logging.getLogger(__name__) -_CURRENT_VERSION = 9 +_CURRENT_VERSION = 10 def _migrate_v1_to_v2(d: dict) -> dict: @@ -98,6 +99,29 @@ def _migrate_v8_to_v9(d: dict) -> dict: return d +def _migrate_v9_to_v10(d: dict) -> dict: + """Complete the three image_config blobs so they can be parsed strictly. + + The ImageConfig parser used to merge whatever was stored onto the pipeline + defaults on every single load. That kept old configs working, but it also + meant a config could stay incomplete forever and a misspelled knob was + silently ignored on every load instead of being reported once. Filling the + gaps here — and dropping keys that are no longer fields — means the parser + can be strict from now on, which is what makes a bad value in the UI or the + API an error the user actually sees. + + Any future change to ImageConfig's shape gets its own migration, for the + same reason: the upgrade knowledge belongs on this chain, not in the parser. + """ + for key, default in ( + ("image_config_default", DEFAULT_IMAGE_CONFIG), + ("image_config_bw", DEFAULT_BW_IMAGE_CONFIG), + ("image_config_face", DEFAULT_FACE_IMAGE_CONFIG), + ): + d[key] = complete_image_config_blob(d.get(key), default=default, field_path=key) + return d + + # v(N) → v(N+1) upgrade functions. Populated as the schema evolves. _MIGRATIONS: dict[int, Callable[[dict], dict]] = { 1: _migrate_v1_to_v2, @@ -108,12 +132,25 @@ def _migrate_v8_to_v9(d: dict) -> dict: 6: _migrate_v6_to_v7, 7: _migrate_v7_to_v8, 8: _migrate_v8_to_v9, + 9: _migrate_v9_to_v10, } def _migrate(data: dict) -> dict: - """Walk the migration chain to the current version.""" + """Walk the migration chain to the current version. + + Raises: + ValueError: if the config comes from a newer version than this build + understands. There is no downgrade path — the parser is strict, so + a field this build has never heard of would otherwise surface as a + baffling validation error rather than as what it is. + """ ver = int(data["version"]) + if ver > _CURRENT_VERSION: + raise ValueError( + f"config is version {ver}, but this server understands up to " + f"{_CURRENT_VERSION} — it was written by a newer version of hokku-server" + ) while ver < _CURRENT_VERSION: data = _MIGRATIONS[ver](data) ver += 1 @@ -213,24 +250,19 @@ def from_dict(cls, data: dict[str, Any]) -> AppConfig: data = _migrate(data) - # Each pipeline merges onto its OWN default, so a sparse or older - # image_config_bw keeps B&W behaviour instead of silently inheriting - # the colour pipeline. - image_config_default = _image_config_from_dict( - data.get("image_config_default"), - field_path="image_config_default", - default=DEFAULT_IMAGE_CONFIG, - ) - image_config_bw = _image_config_from_dict( - data.get("image_config_bw"), - field_path="image_config_bw", - default=DEFAULT_BW_IMAGE_CONFIG, - ) - image_config_face = _image_config_from_dict( - data.get("image_config_face"), - field_path="image_config_face", - default=DEFAULT_FACE_IMAGE_CONFIG, - ) + # An absent key means "not configured" and takes the pipeline default, + # exactly like every other field below. A key that IS present must be + # complete and valid: migration has already brought every stored blob up + # to the current shape, so anything still wrong here is a real mistake + # and is reported rather than papered over. + def _pipeline(key: str, default: ImageConfig) -> ImageConfig: + if key not in data: + return default + return image_config_from_dict_strict(data[key], field_path=key) + + image_config_default = _pipeline("image_config_default", DEFAULT_IMAGE_CONFIG) + image_config_bw = _pipeline("image_config_bw", DEFAULT_BW_IMAGE_CONFIG) + image_config_face = _pipeline("image_config_face", DEFAULT_FACE_IMAGE_CONFIG) _image_fields = {"image_config_default", "image_config_bw", "image_config_face"} diff --git a/python/hokku/webserver/config/config.json.example b/python/hokku/webserver/config/config.json.example index ced52ea0..82da4aee 100644 --- a/python/hokku/webserver/config/config.json.example +++ b/python/hokku/webserver/config/config.json.example @@ -126,5 +126,5 @@ ], "server_threads": 4, "upload_dir": "/var/lib/hokku/images", - "version": 9 + "version": 10 } diff --git a/python/hokku/webserver/image_config.py b/python/hokku/webserver/image_config.py index 9d89213e..494b4207 100644 --- a/python/hokku/webserver/image_config.py +++ b/python/hokku/webserver/image_config.py @@ -174,19 +174,17 @@ def image_config_from_dict_strict( *, field_path: str = "image_config", ) -> ImageConfig: - """Build an ImageConfig from a caller-supplied blob, rejecting anything odd. + """Build an ImageConfig from a blob, rejecting anything that isn't exactly right. - This is the parser for data arriving over the API. It is deliberately the - opposite of ``_image_config_from_dict``, which merges onto defaults so an - older stored config keeps loading: here every field must be present and - valid, and an unrecognised key is an error rather than something to ignore. + The only ImageConfig parser. Every field must be present and valid, and an + unrecognised key is an error rather than something to ignore — so a typo'd + knob name, an invalid ``lut_name`` or a nonsensical gamma is reported here + instead of appearing to succeed and then doing nothing (or failing much + later inside a render worker). - The difference matters because the two have opposite failure modes. Quietly - keeping a default is right for a config file written by a previous version; - it is wrong for a request, where it would answer 200 while discarding the - setting the user actually asked for — a typo'd field name, an invalid - ``lut_name`` or a nonsensical gamma would all appear to succeed and then - either do nothing or fail later inside a render worker. + Nothing needs to be lenient because nothing has to cope with an old shape: + a stored config is brought up to the current shape once, by the migration + that introduced the change, via ``complete_image_config_blob``. Raises: ImageConfigError: listing every problem found. @@ -240,79 +238,63 @@ def parse_crop_to_fill_threshold( return float(value) -def _image_config_from_dict( - blob: Any, - *, - field_path: str = "image_config", - default: ImageConfig | None = None, -) -> ImageConfig: - """Build an ImageConfig by merging a stored JSON object onto the defaults. - - Fields present in *blob* win; fields absent keep their value from - *default*. A config that predates a field therefore keeps every setting it - does carry, instead of being discarded. - - This used to reset the whole pipeline to the fallback preset if a single - field was missing, which silently wiped tuning on upgrade — the entire - shipped example config was being reset that way. See the note where - _LENIENT_DEFAULTS used to live. - - Args: - blob: The dict (or None) to parse. - field_path: Used in log/error messages to identify which config field is bad. - default: Base to merge onto. Defaults to the fallback preset; callers - should pass the default for *their* pipeline so a sparse - B&W or face blob falls back to B&W or face values rather - than to the generic default pipeline. - """ - from hokku.webserver.presets import ( # noqa: PLC0415 — deferred to break circular import - FALLBACK_PRESET, - PRESET_IMAGE_CONFIGS, - ) +def complete_image_config_blob(blob: Any, *, default: ImageConfig, field_path: str) -> dict: + """Bring a stored ImageConfig blob up to the current shape. Migration only. - base = default if default is not None else PRESET_IMAGE_CONFIGS[FALLBACK_PRESET] + Returns a plain dict carrying exactly the fields ImageConfig has today: + values present in *blob* are kept, absent ones take *default*'s, renamed + ones are translated, and keys that are no longer fields are dropped. + This is upgrade knowledge, and it belongs in the migration chain rather + than in the parser. It used to run on *every* load, which meant a config + could stay permanently incomplete and — worse — a misspelled knob was + silently ignored forever instead of being reported once. Doing it once, at + the version bump that needs it, lets the parser be strict from then on. + + Pass the default for the pipeline being migrated, so a sparse B&W or face + blob falls back to B&W or face values rather than to the colour pipeline's. + """ + base = asdict(default) if blob is None: return base if not isinstance(blob, dict): raise ValueError(f"config['{field_path}'] must be an object") - # Renames still need handling explicitly — a renamed field is not the same - # thing as a missing one, and dropping it would lose a real setting. blob = dict(blob) # shallow copy — don't mutate the caller's dict + # A renamed field is not a missing one; dropping it would lose a real + # setting rather than fall back to a default. if "adaptive_saturate_space" not in blob and "use_adaptive_saturate" in blob: blob["adaptive_saturate_space"] = "cielab" if blob["use_adaptive_saturate"] else "off" - blob.pop("use_adaptive_saturate", None) # tolerate either presence; ignore now + blob.pop("use_adaptive_saturate", None) dither_blob = blob.get("dither") if dither_blob is not None and not isinstance(dither_blob, dict): raise ValueError(f"config['{field_path}']['dither'] must be an object") dither_blob = dither_blob or {} - dither_kwargs = { - f.name: (dither_blob[f.name] if f.name in dither_blob else getattr(base.dither, f.name)) - for f in fields(DitherConfig) - } - dither = DitherConfig(**dither_kwargs) - image_kwargs: dict[str, Any] = {"dither": dither} - filled_from_default: list[str] = [] + out: dict[str, Any] = { + "dither": { + f.name: dither_blob.get(f.name, base["dither"][f.name]) for f in fields(DitherConfig) + } + } + filled: list[str] = [] for f in fields(ImageConfig): if f.name == "dither": continue if f.name in blob: - image_kwargs[f.name] = blob[f.name] + out[f.name] = blob[f.name] else: - image_kwargs[f.name] = getattr(base, f.name) - filled_from_default.append(f.name) + out[f.name] = base[f.name] + filled.append(f.name) - if filled_from_default: - # Say so. The old reset was completely silent, which is why a shipped - # example config could be discarded on every single load unnoticed. + dropped = sorted(blob.keys() - {f.name for f in fields(ImageConfig)}) + if filled: logger.info( - "config['%s']: %d field(s) absent, kept defaults: %s", + "config['%s']: %d field(s) absent, took defaults: %s", field_path, - len(filled_from_default), - ", ".join(sorted(filled_from_default)), + len(filled), + ", ".join(sorted(filled)), ) - - return ImageConfig(**image_kwargs) + if dropped: + logger.info("config['%s']: dropped unknown field(s): %s", field_path, ", ".join(dropped)) + return out diff --git a/python/hokku/webserver/render_worker.py b/python/hokku/webserver/render_worker.py index 1371f65a..9171b040 100644 --- a/python/hokku/webserver/render_worker.py +++ b/python/hokku/webserver/render_worker.py @@ -13,8 +13,9 @@ Why dicts, not dataclasses? The dataclasses are picklable *today*, but any future refactor that adds a non-picklable field (callable, lock) would silently break workers. - Round-tripping through ``_image_config_from_dict`` keeps the IPC contract - narrow and easy to audit. + Round-tripping through ``image_config_from_dict_strict`` keeps the IPC + contract narrow and easy to audit — and validated, so a malformed config + fails here rather than producing a quietly wrong render. """ from __future__ import annotations @@ -37,7 +38,7 @@ def render_one( Absolute path to the source image file. image_config_dict: ``dataclasses.asdict(image_config)`` — the full ImageConfig as a plain - dict, ready to be reconstructed via ``_image_config_from_dict``. + dict, reconstructed via ``image_config_from_dict_strict``. model: Screen model id (e.g. ``"huessen_epf1301"``, ``"bigme_f7"``) selecting the ``Display`` that drives palette, geometry, and wire packing. @@ -76,11 +77,11 @@ def render_one( from hokku.webserver.bounding_box import BoundingBox # noqa: PLC0415 from hokku.webserver.dither_streaming_numba import NumbaStreamingDither # noqa: PLC0415 from hokku.webserver.image_abc import preview_png_from_panel_bytes # noqa: PLC0415 - from hokku.webserver.image_config import _image_config_from_dict # noqa: PLC0415 + from hokku.webserver.image_config import image_config_from_dict_strict # noqa: PLC0415 from hokku.webserver.image_renderer import ImageRenderer, open_image_for_render # noqa: PLC0415 display = DISPLAY_REGISTRY[model] - cfg = _image_config_from_dict(image_config_dict) + cfg = image_config_from_dict_strict(image_config_dict) renderer = ImageRenderer(NumbaStreamingDither(display), display) # Convert bbox dicts back to BoundingBox instances diff --git a/python/hokku/webserver/screen_image_config.py b/python/hokku/webserver/screen_image_config.py index 60c82a16..9ad37254 100644 --- a/python/hokku/webserver/screen_image_config.py +++ b/python/hokku/webserver/screen_image_config.py @@ -7,7 +7,7 @@ from dataclasses import asdict, dataclass from hokku.webserver.bounding_box import BoundingBox -from hokku.webserver.image_config import ImageConfig, _image_config_from_dict +from hokku.webserver.image_config import ImageConfig, image_config_from_dict_strict from hokku.webserver.orientation import Orientation @@ -62,7 +62,7 @@ def cache_slug(self) -> str: def _screen_image_config_from_dict(d: dict) -> ScreenImageConfig: """Round-trip helper: dict → ScreenImageConfig.""" - image_config = _image_config_from_dict(d.get("image_config"), field_path="image_config") + image_config = image_config_from_dict_strict(d.get("image_config"), field_path="image_config") orientation = Orientation(d["orientation"]) crop_to_fill_threshold = float(d.get("crop_to_fill_threshold", 0.0)) raw = d.get("clahe_keepout_bboxes") diff --git a/python/tests/test_app_config.py b/python/tests/test_app_config.py index c1603c52..15a7ff2b 100644 --- a/python/tests/test_app_config.py +++ b/python/tests/test_app_config.py @@ -9,7 +9,7 @@ import pytest -from hokku.webserver.app_config import _CURRENT_VERSION, AppConfig, _migrate +from hokku.webserver.app_config import _CURRENT_VERSION, _MIGRATIONS, AppConfig, _migrate from hokku.webserver.presets import ( DEFAULT_BW_IMAGE_CONFIG, DEFAULT_FACE_IMAGE_CONFIG, @@ -141,13 +141,14 @@ def test_image_configs_roundtrip(tmp_path: Path): assert loaded.classifier_bw_detect_enabled is True -def test_image_field_with_partial_blob_falls_back_to_default(tmp_path: Path): - """A corrupt image_config_default blob (partial dither) falls back to the default. +def test_partial_image_blob_at_the_current_version_is_rejected(tmp_path: Path): + """A config claiming to be current must actually be current. - Asserts against DEFAULT_IMAGE_CONFIG rather than a named preset: the - contract under test is "a partial blob merges onto the pipeline's default", - not "the default happens to be preset X". Naming the preset made this fail - whenever the default was retuned, which is a false positive. + It used to be merged onto the pipeline default on every load, which meant a + half-written blob looked fine forever and a misspelled knob was ignored + silently. Bringing an old config up to date is the migration chain's job + now, so anything still incomplete at the current version is a real fault + and is reported. """ p = tmp_path / "c.json" p.write_text( @@ -158,8 +159,61 @@ def test_image_field_with_partial_blob_falls_back_to_default(tmp_path: Path): } ) ) + with pytest.raises(SystemExit): + AppConfig.load(p) + + +def test_absent_image_blob_takes_the_pipeline_default(tmp_path: Path): + """Absent is not the same as wrong: an unconfigured pipeline is fine. + + Every other field behaves this way, and a hand-edited config that simply + omits a section must not stop the server from booting. + """ + p = tmp_path / "c.json" + p.write_text(json.dumps({"version": _CURRENT_VERSION, "port": 8080})) + cfg = AppConfig.load(p) + assert cfg.image_config_default == DEFAULT_IMAGE_CONFIG + assert cfg.image_config_bw == DEFAULT_BW_IMAGE_CONFIG + assert cfg.image_config_face == DEFAULT_FACE_IMAGE_CONFIG + + +def test_config_from_a_newer_version_is_refused(tmp_path: Path): + """There is no downgrade path, so say so plainly. + + Without this the strict parser would reject a field it has never heard of + and report a confusing validation error instead of the actual problem. + """ + p = tmp_path / "c.json" + p.write_text(json.dumps({"version": _CURRENT_VERSION + 1})) + with pytest.raises(SystemExit): + AppConfig.load(p) + + +def test_migration_completes_a_sparse_image_blob(tmp_path: Path): + """The upgrade path an older config actually takes. + + v9 stored blobs that could be missing fields; v10 fills them in once so the + parser can be strict from then on. + """ + p = tmp_path / "c.json" + p.write_text( + json.dumps( + { + "version": 9, + "image_config_default": {"prepare_gamma": 0.55}, + } + ) + ) + + cfg = AppConfig.load(p) + + assert cfg.image_config_default.prepare_gamma == pytest.approx(0.55) # kept + assert ( + cfg.image_config_default.clahe_keepout_feather + == DEFAULT_IMAGE_CONFIG.clahe_keepout_feather # filled + ) def test_v1_migrates_to_current(): @@ -231,6 +285,11 @@ def test_old_config_gets_default_server_threads(tmp_path: Path): assert AppConfig.load(p).server_threads == AppConfig().server_threads +def test_every_version_below_current_has_a_migration(): + """A gap in the chain would raise KeyError mid-upgrade.""" + assert set(_MIGRATIONS) == set(range(1, _CURRENT_VERSION)) + + def test_cache_slug_invariant_to_server_threads(): """Serving concurrency doesn't affect rendered output — must not change the slug.""" assert AppConfig(server_threads=2).cache_slug() == AppConfig(server_threads=8).cache_slug() diff --git a/python/tests/test_image_config.py b/python/tests/test_image_config.py index 3bd6fdc6..947fd779 100644 --- a/python/tests/test_image_config.py +++ b/python/tests/test_image_config.py @@ -12,11 +12,10 @@ from hokku.webserver.image_config import ( ImageConfig, ImageConfigError, - _image_config_from_dict, + complete_image_config_blob, image_config_from_dict_strict, parse_crop_to_fill_threshold, ) -from hokku.webserver.presets import FALLBACK_PRESET, PRESET_IMAGE_CONFIGS def _default_dither() -> DitherConfig: @@ -63,7 +62,7 @@ def _default_image_config() -> ImageConfig: def test_default_roundtrip_via_asdict(): cfg = _default_image_config() d = asdict(cfg) - restored = _image_config_from_dict(d) + restored = image_config_from_dict_strict(d) assert restored == cfg @@ -81,14 +80,14 @@ def test_non_default_roundtrip(): neutral_chroma=10.0, ), ) - restored = _image_config_from_dict(asdict(cfg)) + restored = image_config_from_dict_strict(asdict(cfg)) assert restored == cfg def test_cache_slug_stable(): cfg = _default_image_config() assert cfg.cache_slug() == cfg.cache_slug() - assert cfg.cache_slug() == _image_config_from_dict(asdict(cfg)).cache_slug() + assert cfg.cache_slug() == image_config_from_dict_strict(asdict(cfg)).cache_slug() def test_cache_slug_changes_when_brightness_changes(): @@ -107,97 +106,116 @@ def test_cache_slug_length(): assert len(_default_image_config().cache_slug()) == 14 -def test_image_config_from_dict_none_returns_default(): - result = _image_config_from_dict(None) - assert result == PRESET_IMAGE_CONFIGS[FALLBACK_PRESET] +# ── complete_image_config_blob (migration helper) ───────────────────────────── +# +# Bringing an old blob up to the current shape is upgrade knowledge, so it lives +# on the migration chain and runs once, at the version bump that needs it. The +# parser itself stays strict. These tests cover what that one-time repair has to +# get right for a config written by an older build. -def test_image_config_from_dict_missing_dither_keeps_default_dither(): - """An absent dither block keeps the default's dither — the rest survives.""" +def test_complete_fills_an_absent_dither_block(): base = _default_image_config() d = asdict(base) d.pop("dither") d["prepare_brightness"] = 1.42 - restored = _image_config_from_dict(d, default=base) - assert restored.dither == base.dither - assert restored.prepare_brightness == pytest.approx(1.42) + out = complete_image_config_blob(d, default=base, field_path="cfg") + + assert out["dither"] == asdict(base.dither) + assert out["prepare_brightness"] == pytest.approx(1.42) -def test_image_config_from_dict_missing_field_keeps_only_that_field_default(): - """One absent field must not discard every other stored value. - This is the regression that shipped: a single missing field reset the whole - pipeline to the fallback preset, so the example config — and any config - predating a newly added field — silently lost all of its tuning. +def test_complete_keeps_every_other_stored_value(): + """One absent field must not cost the rest of the tuning. + + This is the regression that shipped once already: a single missing field + reset the whole pipeline to the fallback preset, so the example config — and + any config predating a newly added field — silently lost everything. """ base = _default_image_config() d = asdict(base) d.pop("prepare_brightness") - d["prepare_gamma"] = 0.55 # a deliberately non-default value + d["prepare_gamma"] = 0.55 # deliberately non-default d["color_enhance"] = 1.9 - restored = _image_config_from_dict(d) + out = complete_image_config_blob(d, default=base, field_path="cfg") - assert restored.prepare_brightness == base.prepare_brightness # filled from default - assert restored.prepare_gamma == pytest.approx(0.55) # stored value survives - assert restored.color_enhance == pytest.approx(1.9) + assert out["prepare_brightness"] == base.prepare_brightness # taken from default + assert out["prepare_gamma"] == pytest.approx(0.55) # stored value survives + assert out["color_enhance"] == pytest.approx(1.9) -def test_image_config_from_dict_merges_onto_the_supplied_default(): - """Each pipeline merges onto its own default, not onto the fallback preset.""" +def test_complete_uses_the_pipeline_its_given(): + """A sparse B&W blob must fall back to B&W values, not the colour pipeline's.""" face_like = replace(_default_image_config(), clahe_clip_limit=1.25, prepare_usm_amount=130) - restored = _image_config_from_dict({"prepare_gamma": 0.7}, default=face_like) - assert restored.clahe_clip_limit == pytest.approx(1.25) - assert restored.prepare_usm_amount == 130 - assert restored.prepare_gamma == pytest.approx(0.7) - - -def test_image_config_from_dict_not_dict_raises(): - with pytest.raises(ValueError): - _image_config_from_dict("not a dict") + out = complete_image_config_blob({"prepare_gamma": 0.7}, default=face_like, field_path="cfg") -# ── new field leniency ──────────────────────────────────────────────────────── + assert out["clahe_clip_limit"] == pytest.approx(1.25) + assert out["prepare_usm_amount"] == 130 + assert out["prepare_gamma"] == pytest.approx(0.7) -def test_new_fields_use_defaults_when_absent(): - """A config predating several fields keeps everything it does carry.""" +def test_complete_translates_the_renamed_saturate_flag(): + """A rename is not a missing field — the old key still carries a real setting.""" base = _default_image_config() d = asdict(base) - for key in ( - "prepare_midtone", - "clahe_clip_limit", - "prepare_usm_radius", - "prepare_usm_amount", - "dither_noise", - # The field whose omission caused the shipped example to reset. - "clahe_keepout_feather", - ): - d.pop(key, None) - d["prepare_contrast"] = 1.33 - - restored = _image_config_from_dict(d, default=base) - - assert restored.prepare_contrast == pytest.approx(1.33) - for key in ("prepare_midtone", "clahe_clip_limit", "clahe_keepout_feather"): - assert getattr(restored, key) == getattr(base, key) - - -def test_renamed_use_adaptive_saturate_still_honoured(): - """A rename is not a missing field — the old key must still carry meaning.""" - d = asdict(_default_image_config()) d.pop("adaptive_saturate_space") + d["use_adaptive_saturate"] = False - assert _image_config_from_dict(d).adaptive_saturate_space == "off" + assert ( + complete_image_config_blob(d, default=base, field_path="c")["adaptive_saturate_space"] + == "off" + ) d["use_adaptive_saturate"] = True - assert _image_config_from_dict(d).adaptive_saturate_space == "cielab" + assert ( + complete_image_config_blob(d, default=base, field_path="c")["adaptive_saturate_space"] + == "cielab" + ) + + +def test_complete_drops_fields_that_no_longer_exist(): + """Otherwise the strict parser would reject the migrated config.""" + base = _default_image_config() + d = asdict(base) + d["some_retired_knob"] = 3 + + out = complete_image_config_blob(d, default=base, field_path="cfg") + + assert "some_retired_knob" not in out -# ── strict parser (API input) ───────────────────────────────────────────────── +def test_complete_output_parses_strictly(): + """The point of the exercise: migrate once, then parse strictly forever.""" + base = _default_image_config() + d = asdict(base) + d.pop("prepare_midtone") + d.pop("clahe_keepout_feather") + d["use_adaptive_saturate"] = True + d.pop("adaptive_saturate_space") + d["retired"] = "x" + + out = complete_image_config_blob(d, default=base, field_path="cfg") + + assert image_config_from_dict_strict(out).adaptive_saturate_space == "cielab" + + +def test_complete_none_returns_the_default(): + base = _default_image_config() + assert complete_image_config_blob(None, default=base, field_path="cfg") == asdict(base) + + +def test_complete_rejects_a_non_object(): + with pytest.raises(ValueError): + complete_image_config_blob("not a dict", default=_default_image_config(), field_path="cfg") + + +# ── strict parser ───────────────────────────────────────────────────────────── # -# The strict parser is the mirror image of the lenient one above: the lenient -# path exists so a stored config from an older version keeps loading, while this -# one exists so a request cannot quietly mean something other than it says. +# The only ImageConfig parser. Everything that reaches it — a stored config, an +# API request, the render worker's IPC payload — has to be complete and valid, +# so a mistake is reported once instead of being carried silently forever. def test_strict_accepts_a_complete_config(): @@ -333,16 +351,14 @@ def test_strict_rejects_non_object_dither(): image_config_from_dict_strict(d) -def test_strict_result_survives_the_lenient_round_trip(): - """The render worker re-parses leniently; that must not alter the config. +def test_strict_survives_the_render_worker_round_trip(): + """render_one() receives asdict(cfg) and rebuilds it on the other side. - render_worker.render_one() receives asdict(image_config) and rebuilds it - with _image_config_from_dict, so a strict-validated override has to come - back out of that path bit-identical or the rendered image would not match - the settings the cache slug was computed from. + That has to come back bit-identical, or the rendered image would not match + the settings its cache slug was computed from. """ cfg = image_config_from_dict_strict(asdict(_default_image_config())) - assert _image_config_from_dict(asdict(cfg)) == cfg + assert image_config_from_dict_strict(asdict(cfg)) == cfg def test_strict_field_coverage_matches_the_dataclass(): From 675ddc3ebace1f09798c6c1386c2436608c656b4 Mon Sep 17 00:00:00 2001 From: Dennis Fleurbaaij Date: Sun, 2 Aug 2026 10:50:41 -0500 Subject: [PATCH 09/15] feat(ui): per-picture dither and crop overrides, on one shared editor The dither editor was written out three times in the template - once per pipeline - with only the knobs inside it generated. The copies had already drifted: the B&W one silently lacked the face keep-out overlay the other two had. Adding a fourth copy for the per-picture editor would have made that worse, so the whole editor is now generated by mountDitherEditor() and the Config tab mounts three instances of it. The per-picture editor is a fourth instance of the same component, with the same handlers. Supporting changes to make one component serve four mounts: - the three module-level state variables become a ditherStates map keyed by panel id, so nothing has to know how many editors exist; - the preset help popover is wired per instance rather than once at DOMContentLoaded, because at page load no editor exists yet; - updatePresetDescription() takes a panel id instead of being hardcoded to the default pipeline, so B&W and face get preset descriptions too; - runDitherPreview() gains a pinned mode: the per-picture editor emits the same preview - - - - - - - + +
@@ -1118,36 +1101,7 @@

Advanced dithering pipeline

B&W photo pipeline

Used for black-and-white photos — film scans, old family photos, monochrome art. The server detects these automatically and uses a separate pipeline that avoids the colour-boosting steps which would otherwise add an unwanted pink or yellow tint to the grays.

-
- - - - -
- +
@@ -1170,45 +1124,7 @@

Face photo pipeline

Yes
-
- - - - -
- +
@@ -1361,6 +1277,30 @@

+ + +
+

Conversion for this picture

+

+
+
+ + + automatic + +
+

+
+ + + + +
+ +
+
@@ -1632,18 +1572,20 @@

State

} } -// Dither config UI state. `ditherState` is the form's working copy; it's -// initialised once by loadConfig() and never overwritten by status polling. +// Each dither editor's working copy lives in `ditherStates`, keyed by panel id +// (see DITHER_PANEL_IDS below). The Config-tab ones are initialised once by +// loadConfig() and never overwritten by status polling; the per-picture one is +// (re)initialised each time the image details modal opens. // `ditherPresets` is the catalog from /api/config so we don't have to mirror // the backend's preset definitions in JS. -let ditherState = null; let ditherPresets = {}; let configDefaults = null; let lastPanelData = {visual_w: {{visual_w}}, visual_h: {{visual_h}}}; -// Classifier secondary config states — full ImageConfig dicts for B&W / face. -let bwDitherState = null; -let faceDitherState = null; +// Per-picture editor state, valid only while the image details modal is open. +// null crop = this picture uses the global letterbox-fill setting. +let imgcfgFilename = null; +let imgcfgCropOverride = null; // Per-knob help text shown in hover popovers. Plain English, describes the // specific control the user is hovering — never another control's options. @@ -1720,29 +1662,131 @@

State

return document.getElementById('auto-clear-cache').checked; } -// Map panelId ('default', 'bw', 'face') to the DOM id prefix and state vars. +// Every dither editor in the app is an instance of the same component: the +// three global pipelines in Config, and the per-picture one in the image +// details modal. They are generated from one markup function and share every +// handler, so a change to the editor lands in all of them at once. +// +// 'default' keeps the 'dither-' DOM prefix it has always had. +const DITHER_PANEL_IDS = ['default', 'bw', 'face', 'imgcfg']; + function _pfx(panelId) { return (!panelId || panelId === 'default') ? 'dither' : panelId; } -function _getState(panelId) { - if (panelId === 'bw') return bwDitherState; - if (panelId === 'face') return faceDitherState; - return ditherState; + +// Working copy of each editor's ImageConfig, keyed by panel id. +const ditherStates = {}; +function _getState(panelId) { return ditherStates[panelId || 'default']; } +function _setState(panelId, v) { ditherStates[panelId || 'default'] = v; } + +// Which editors are actually on the page. The per-picture one only exists once +// its modal has mounted it, and the Config ones only after loadConfig(). +function _mountedPanelIds() { + return DITHER_PANEL_IDS.filter(id => document.getElementById(_pfx(id) + '-preset')); } -function _setState(panelId, v) { - if (panelId === 'bw') bwDitherState = v; - else if (panelId === 'face') faceDitherState = v; - else ditherState = v; + +// The editor's markup. Built here rather than written out per instance so the +// three Config pipelines and the per-picture editor cannot drift apart — they +// used to be three hand-maintained copies, and had already diverged (the B&W +// one silently lacked the face keep-out overlay the other two had). +// +// opts: +// presetLabel text of the preset row's label +// title/intro heading and blurb for the advanced panel +// pinnedImage preview is fixed to one picture, so no "Preview on:" picker +function ditherEditorMarkup(panelId, opts) { + const pfx = _pfx(panelId); + const previewRow = opts.pinnedImage + // Still a + + ` + : ` + + + `; + + return ` +
+ + + + + + + +
+ + `; } -// Populate the preset dropdown(s). Called once after ditherPresets is loaded. -// Populates all three panels so a single call covers everything. -function populatePresetDropdown() { - ['default', 'bw', 'face'].forEach(panelId => { +// Render an editor into *mountEl* and wire it up. Safe to call again on the +// same mount — the per-picture editor re-mounts every time its modal opens. +function mountDitherEditor(panelId, mountEl, opts = {}) { + if (!mountEl) return; + const pfx = _pfx(panelId); + mountEl.innerHTML = ditherEditorMarkup(panelId, opts); + + const on = (id, event, fn) => { + const el = document.getElementById(id); + if (el) el.addEventListener(event, fn); + }; + on(pfx + '-reset-btn', 'click', () => resetToPreset(panelId)); + on(pfx + '-custom-btn', 'click', () => toggleAdvancedPanel(panelId)); + on(pfx + '-advanced-close', 'click', () => toggleAdvancedPanel(panelId)); + on(pfx + '-preview-btn', 'click', () => runDitherPreview(panelId)); + on(pfx + '-preview-show-faces', 'change', () => updateDitherPreviewFaces(panelId)); + if (opts.onCustomToggle) { + on(pfx + '-custom-btn', 'click', opts.onCustomToggle); + on(pfx + '-advanced-close', 'click', opts.onCustomToggle); + } + wirePresetPopover(panelId); +} + +// Fill the preset dropdown of every mounted editor. Called after ditherPresets +// is loaded, and again whenever an editor is mounted later (the per-picture one). +function populatePresetDropdown(only) { + const ids = only ? [only] : _mountedPanelIds(); + ids.forEach(panelId => { const pfx = _pfx(panelId); const sel = document.getElementById(pfx + '-preset'); if (!sel) return; sel.innerHTML = ''; + // The per-picture editor can hand a picture back to the classifier, which + // the global pipelines cannot — there is nothing above them to defer to. + if (panelId === 'imgcfg') { + const autoOpt = document.createElement('option'); + autoOpt.value = '__auto__'; + autoOpt.textContent = 'Automatic (let the server choose)'; + sel.appendChild(autoOpt); + } Object.entries(ditherPresets).forEach(([key, p]) => { const opt = document.createElement('option'); opt.value = key; @@ -1756,18 +1800,33 @@

State

// Capture panelId for the closure — don't let it be the loop variable. sel.onchange = ((pid) => () => { const v = sel.value; + if (v === '__auto__') { + onDitherPanelChanged(pid); + return; + } if (v !== '__custom__' && ditherPresets[v]) { sel.dataset.lastPreset = v; const {label, description, ...presetFields} = ditherPresets[v]; _setState(pid, deepCopy(presetFields)); renderDitherPanel(pid); - if (pid === 'default') updatePresetDescription(); - if (isAutoClearEnabled()) scrubStaleCache(); + updatePresetDescription(pid); + onDitherPanelChanged(pid); + // Only the saved pipelines invalidate cached renders. The per-picture + // editor is a draft until Apply, so it must not scrub anything. + if (pid !== 'imgcfg' && isAutoClearEnabled()) scrubStaleCache(); } })(panelId); }); } +// Notify an editor's owner that its value changed. Only the per-picture editor +// listens today, to enable its Apply button. +const ditherPanelChangeHooks = {}; +function onDitherPanelChanged(panelId) { + const hook = ditherPanelChangeHooks[panelId || 'default']; + if (hook) hook(); +} + // Pick the preset whose dither config matches state verbatim; fall back to // "Custom" if none match. panelId selects which dropdown to update. function selectPresetMatching(state, panelId) { @@ -1775,6 +1834,9 @@

State

const pfx = _pfx(panelId); const sel = document.getElementById(pfx + '-preset'); if (!sel) return; + // NOTE: this comparison is key-order sensitive, which is fine because both + // sides originate from asdict(ImageConfig) and keep its field order. Do not + // rebuild either object key-by-key in JS. const stateJson = JSON.stringify(state); let matched = '__custom__'; for (const [key, p] of Object.entries(ditherPresets)) { @@ -1783,19 +1845,23 @@

State

} sel.value = matched; if (matched !== '__custom__') sel.dataset.lastPreset = matched; - if (panelId === 'default') updatePresetDescription(); + updatePresetDescription(panelId); } -function updatePresetDescription() { +function updatePresetDescription(panelId) { // Populate the (?) popover. Description text used to live as an inline // , but it bloated the row visually. Now hidden behind // a help button next to the preset selector. - const sel = document.getElementById('dither-preset'); - const pop = document.getElementById('dither-preset-popover'); - if (!pop) return; + const pfx = _pfx(panelId || 'default'); + const sel = document.getElementById(pfx + '-preset'); + const pop = document.getElementById(pfx + '-preset-popover'); + if (!sel || !pop) return; const v = sel.value; let html; - if (v === '__custom__') { + if (v === '__auto__') { + html = '

The server picks a pipeline for this picture from its content — ' + + 'black-and-white detection first, then face detection, then the default.

'; + } else if (v === '__custom__') { html = '

Your edits have diverged from every built-in preset.

'; } else if (ditherPresets[v]) { html = '

' + escapeHtml(ditherPresets[v].label || v) + '

' @@ -1836,23 +1902,22 @@

State

const {label, description, ...presetFields} = ditherPresets[key]; _setState(panelId, deepCopy(presetFields)); renderDitherPanel(panelId); - if (panelId === 'default') updatePresetDescription(); + updatePresetDescription(panelId); + onDitherPanelChanged(panelId); showToast('Reset to preset: ' + ditherPresets[key].label); } } function resetPipelineToDefault() { if (!configDefaults) return; - ditherState = JSON.parse(JSON.stringify(configDefaults.image_config_default || {})); - bwDitherState = JSON.parse(JSON.stringify(configDefaults.image_config_bw || {})); - faceDitherState = JSON.parse(JSON.stringify(configDefaults.image_config_face || {})); - selectPresetMatching(ditherState, 'default'); - selectPresetMatching(bwDitherState, 'bw'); - selectPresetMatching(faceDitherState, 'face'); - renderDitherPanel('default'); - renderDitherPanel('bw'); - renderDitherPanel('face'); - updatePresetDescription(); + // Config-tab pipelines only. 'imgcfg' is per-picture and is not part of the + // server config this button resets. + ['default', 'bw', 'face'].forEach(panelId => { + const key = panelId === 'default' ? 'image_config_default' : 'image_config_' + panelId; + _setState(panelId, deepCopy(configDefaults[key] || {})); + selectPresetMatching(_getState(panelId), panelId); + renderDitherPanel(panelId); + }); const bwOn = !!configDefaults.classifier_bw_detect_enabled; document.getElementById('bw-detect-enabled').checked = bwOn; @@ -1884,10 +1949,11 @@

State

const pfx = _pfx(panelId); const sel = document.getElementById(pfx + '-preset'); if (sel) sel.value = '__custom__'; - if (panelId === 'default') updatePresetDescription(); + updatePresetDescription(panelId); + onDitherPanelChanged(panelId); } -// Build a single knob/row. panelId ('default'|'bw'|'face') selects which +// Build a single knob/row. panelId ('default'|'bw'|'face'|'imgcfg') selects which // state dict to read from and which onchange target to write to. // "check" — single checkbox bound to state[path...] // "num" — number input bound to state[path...] @@ -2100,47 +2166,76 @@

State

document.addEventListener('keydown', (e) => { if (e.key === 'Escape') { closeHelpPopover(); - closePresetPopover(); + DITHER_PANEL_IDS.forEach(closePresetPopover); } }); // Preset popover — shown on hover over the select, hidden on mouse-leave. -function closePresetPopover() { - const pop = document.getElementById('dither-preset-popover'); +function closePresetPopover(panelId) { + const pop = document.getElementById(_pfx(panelId || 'default') + '-preset-popover'); if (pop) pop.hidden = true; } -document.addEventListener('DOMContentLoaded', () => { - const wrap = document.getElementById('dither-preset-wrap'); - const pop = document.getElementById('dither-preset-popover'); - if (!wrap || !pop) return; + +// Hover-help for one editor's preset dropdown. Wired per instance by +// mountDitherEditor rather than once at DOMContentLoaded, because the editors +// are generated — at page load none of them exist yet. +function wirePresetPopover(panelId) { + const pfx = _pfx(panelId); + const wrap = document.getElementById(pfx + '-preset-wrap'); + const pop = document.getElementById(pfx + '-preset-popover'); + const sel = document.getElementById(pfx + '-preset'); + if (!wrap || !pop || !sel) return; let hideTimer; - function showPresetPopover() { + wrap.addEventListener('mouseenter', () => { clearTimeout(hideTimer); - updatePresetDescription(); + updatePresetDescription(panelId); const r = wrap.getBoundingClientRect(); pop.style.top = (r.bottom + 6) + 'px'; pop.style.left = Math.max(8, Math.min(window.innerWidth - 500, r.left)) + 'px'; pop.hidden = false; - } - function scheduleHide() { hideTimer = setTimeout(() => { pop.hidden = true; }, 120); } - wrap.addEventListener('mouseenter', showPresetPopover); - wrap.addEventListener('mouseleave', scheduleHide); + }); + wrap.addEventListener('mouseleave', () => { + hideTimer = setTimeout(() => { pop.hidden = true; }, 120); + }); pop.addEventListener('mouseenter', () => clearTimeout(hideTimer)); pop.addEventListener('mouseleave', () => { pop.hidden = true; }); - // Also update content when the user changes the selected preset. - document.getElementById('dither-preset').addEventListener('change', () => { - if (!pop.hidden) updatePresetDescription(); + sel.addEventListener('change', () => { + if (!pop.hidden) updatePresetDescription(panelId); }); document.addEventListener('click', (e) => { - if (!pop.hidden && !pop.contains(e.target) && !wrap.contains(e.target)) closePresetPopover(); + if (!pop.hidden && !pop.contains(e.target) && !wrap.contains(e.target)) { + closePresetPopover(panelId); + } }); -}); +} // ── Preview button ──────────────────────────────────────────────── + +// Request body for one preview. Split out so the per-picture editor and the +// compare grid can add their own crop threshold and tile size without +// duplicating the shared parts. +function ditherPreviewBody(panelId, filename, extra) { + const body = { + name: filename, + image: _getState(panelId), + clahe_keepout: document.getElementById('face-clahe-keepout-enabled').checked, + face_aware_crop: document.getElementById('face-aware-crop-enabled').checked, + }; + // The per-picture editor previews the crop it is about to save, which may + // differ from the global one; the Config pipelines have no crop of their own + // and let the server fall back to the saved value. + if (panelId === 'imgcfg' && imgcfgCropOverride !== null) { + body.crop_to_fill_threshold = imgcfgCropOverride; + } + return Object.assign(body, extra || {}); +} + function updatePreviewImageOptions(uploadList) { // The preview endpoint renders from the original file, not the cached // binary, so include all uploaded images regardless of dither status. const available = (uploadList || []).map(e => e.name); + // Skips the per-picture editor: its picker is hidden and pinned to one file, + // so rebuilding the option list would throw that pin away. ['default', 'bw', 'face'].forEach(panelId => { const sel = document.getElementById(_pfx(panelId) + '-preview-image'); if (!sel) return; @@ -2187,12 +2282,7 @@

State

const resp = await fetch('/hokku/api/dither/preview', { method: 'POST', headers: {'Content-Type': 'application/json'}, - body: JSON.stringify({ - name: filename, - image: _getState(panelId), - clahe_keepout: document.getElementById('face-clahe-keepout-enabled').checked, - face_aware_crop: document.getElementById('face-aware-crop-enabled').checked, - }), + body: JSON.stringify(ditherPreviewBody(panelId, filename)), }); if (!resp.ok) { let err = `HTTP ${resp.status}`; @@ -2699,13 +2789,13 @@

State

const v = parseInt(document.getElementById('memory-budget-mb').value, 10); return Number.isNaN(v) || v < 1 ? 0 : v; // empty / invalid → 0 = auto })(), - image_config_default: ditherState, + image_config_default: _getState('default'), classifier_bw_detect_enabled: document.getElementById('bw-detect-enabled').checked, - image_config_bw: bwDitherState, + image_config_bw: _getState('bw'), classifier_face_detect_enabled: document.getElementById('face-detect-enabled').checked, classifier_face_detect_clahe_keepout: document.getElementById('face-clahe-keepout-enabled').checked, classifier_face_aware_crop_enabled: document.getElementById('face-aware-crop-enabled').checked, - image_config_face: faceDitherState, + image_config_face: _getState('face'), }; try { const resp = await fetch('/hokku/api/config', { @@ -3363,6 +3453,18 @@

State

let lastUploadFiles = []; let lastServeData = {}; +// A small chip on the card saying which pipeline converted this picture. Only +// shown when it isn't the plain default, so an ordinary library stays quiet. +function pipelineChip(entry) { + const labels = {override: 'Custom', bw: 'B&W', face: 'Face'}; + const text = labels[entry.pipeline]; + if (!text) return ''; + const title = entry.pipeline === 'override' + ? 'Converted with settings chosen for this picture' + : 'Converted with the ' + text + ' pipeline (detected automatically)'; + return ` ${escapeHtml(text)}`; +} + function renderImageGrid(uploadFiles, serveData, nextImages) { lastUploadFiles = uploadFiles; lastServeData = serveData; @@ -3398,7 +3500,7 @@

State

${pendingBadge} ${thumbHtml}
-
${escapeHtml(fname)}
+
${escapeHtml(fname)}${pipelineChip(entry)}
${rotationHtml} @@ -3512,9 +3614,12 @@

State

actions.innerHTML = 'Dithered version not ready yet'; } + setupImageOverrideEditor(fname, entry); + modal.classList.add('show'); const close = () => { modal.classList.remove('show'); + teardownImageOverrideEditor(); closeBtn.removeEventListener('click', close); modal.removeEventListener('click', onBackdrop); document.removeEventListener('keydown', onKey); @@ -3527,6 +3632,332 @@

State

document.addEventListener('keydown', onKey); } +// ── Per-picture conversion override ─────────────────────────────── +// +// The modal mounts a full dither editor — the same component and handlers the +// three Config-tab pipelines use — plus its own letterbox-fill control. The two +// are independent overrides on the image's record, so each has its own Apply +// and its own "Use automatic". + +let imgcfgCompareAbort = null; // cancels an in-flight compare grid +let imgcfgCompareBusy = false; // stops rapid re-clicks queueing more renders +let imgcfgDirty = false; + +function setupImageOverrideEditor(fname, entry) { + imgcfgFilename = fname; + imgcfgCropOverride = null; + imgcfgDirty = false; + + const mount = document.getElementById('imgcfg-editor-mount'); + mountDitherEditor('imgcfg', mount, { + presetLabel: 'Dither:', + title: 'Advanced pipeline for this picture', + intro: 'These settings apply to this picture only. Everything else keeps using the pipelines from the Config tab.', + pinnedImage: true, + // Widen the modal while the knobs are open — they do not fit in 460px. + onCustomToggle: () => { + const panel = document.getElementById('imgcfg-advanced'); + document.querySelector('.img-detail-modal').classList.toggle('wide', panel && !panel.hidden); + }, + }); + populatePresetDropdown('imgcfg'); + // Pin the (hidden) preview picker to this picture so runDitherPreview works + // through exactly the same path as the Config-tab editors. + const previewSel = document.getElementById('imgcfg-preview-image'); + if (previewSel) previewSel.innerHTML = ``; + + ditherPanelChangeHooks['imgcfg'] = () => markImageOverrideDirty(); + + document.getElementById('imgcfg-apply-btn').onclick = applyImageOverride; + document.getElementById('imgcfg-auto-btn').onclick = clearImageOverride; + document.getElementById('imgcfg-compare-btn').onclick = runImageCompareGrid; + document.getElementById('imgcfg-crop-override-btn').onclick = toggleImageCropOverride; + document.getElementById('imgcfg-crop').oninput = (e) => { + imgcfgCropOverride = (parseInt(e.target.value, 10) || 0) / 100; + document.getElementById('imgcfg-crop-val').textContent = e.target.value + '%'; + markImageOverrideDirty(); + }; + + renderImageOverrideCropHint(entry); + loadImageOverrideState(fname); +} + +function teardownImageOverrideEditor() { + if (imgcfgCompareAbort) { imgcfgCompareAbort.abort(); imgcfgCompareAbort = null; } + imgcfgCompareBusy = false; + delete ditherPanelChangeHooks['imgcfg']; + imgcfgFilename = null; + const grid = document.getElementById('imgcfg-compare-grid'); + if (grid) { + // Object URLs outlive the nodes that referenced them. + grid.querySelectorAll('img[data-url]').forEach(img => URL.revokeObjectURL(img.dataset.url)); + grid.innerHTML = ''; + grid.hidden = true; + } + const preview = document.getElementById('imgcfg-preview-img'); + if (preview && preview.dataset.url) { + URL.revokeObjectURL(preview.dataset.url); + delete preview.dataset.url; + } + document.querySelector('.img-detail-modal').classList.remove('wide'); +} + +function markImageOverrideDirty() { + imgcfgDirty = true; + const btn = document.getElementById('imgcfg-apply-btn'); + if (btn) btn.disabled = false; + const status = document.getElementById('imgcfg-apply-status'); + if (status) status.textContent = ''; +} + +// Tell the user what fill percentage this picture would actually need, which +// the details table already computes — here it becomes actionable. +function renderImageOverrideCropHint(entry) { + const hint = document.getElementById('imgcfg-crop-hint'); + if (!hint) return; + hint.textContent = ''; + if (!entry || !entry.image_width || !entry.image_height) return; + const orient = entry.native_orientation || 'landscape'; + const visW = orient === 'landscape' ? lastPanelData.visual_w : lastPanelData.visual_h; + const visH = orient === 'landscape' ? lastPanelData.visual_h : lastPanelData.visual_w; + const scaleFit = Math.min(visW / entry.image_width, visH / entry.image_height); + const scaleCover = Math.max(visW / entry.image_width, visH / entry.image_height); + const zoom = scaleCover / scaleFit - 1.0; + if (zoom > 1e-9) { + hint.textContent = `Needs at least ${Math.ceil(zoom * 100)}% to fill the frame with no letterbox bars.`; + } +} + +async function loadImageOverrideState(fname) { + const status = document.getElementById('imgcfg-apply-status'); + try { + const resp = await fetch('/hokku/api/image/' + encodeURIComponent(fname) + '/config'); + if (!resp.ok) throw new Error(`HTTP ${resp.status}`); + const data = await resp.json(); + if (imgcfgFilename !== fname) return; // modal moved on while we waited + + const overrides = data.overrides || {}; + const effective = data.effective || {}; + // Start the editor from what the picture looks like NOW, so tuning is a + // change to the real thing rather than to an arbitrary preset. + _setState('imgcfg', deepCopy(overrides.image_config || effective.image_config || {})); + + const sel = document.getElementById('imgcfg-preset'); + if (overrides.image_config) { + selectPresetMatching(_getState('imgcfg'), 'imgcfg'); + } else if (sel) { + // No override: say "Automatic" rather than showing whichever preset the + // effective config happens to resemble. + sel.value = '__auto__'; + updatePresetDescription('imgcfg'); + } + renderDitherPanel('imgcfg'); + + imgcfgCropOverride = overrides.crop_to_fill_threshold ?? null; + syncImageCropControl(effective.crop_to_fill_threshold ?? 0); + + document.getElementById('imgcfg-pipeline-note').textContent = + describePipeline(data.pipeline); + document.getElementById('imgcfg-auto-btn').hidden = + !overrides.image_config && overrides.crop_to_fill_threshold == null; + + imgcfgDirty = false; + document.getElementById('imgcfg-apply-btn').disabled = true; + } catch (e) { + if (status) status.textContent = 'Could not load settings: ' + e.message; + } +} + +function describePipeline(pipeline) { + switch (pipeline) { + case 'override': return 'Using settings you chose for this picture.'; + case 'bw': return 'Automatic: detected as black-and-white, using the B&W pipeline.'; + case 'face': return 'Automatic: faces detected, using the face pipeline.'; + default: return 'Automatic: using the default pipeline.'; + } +} + +function syncImageCropControl(effectiveValue) { + const slider = document.getElementById('imgcfg-crop'); + const label = document.getElementById('imgcfg-crop-val'); + const btn = document.getElementById('imgcfg-crop-override-btn'); + const overridden = imgcfgCropOverride !== null; + const shown = overridden ? imgcfgCropOverride : effectiveValue; + slider.value = Math.round(shown * 100); + slider.disabled = !overridden; + label.textContent = overridden ? `${Math.round(shown * 100)}%` + : `automatic (${Math.round(shown * 100)}%)`; + btn.textContent = overridden ? 'Use automatic' : 'Override'; + slider.dataset.effective = String(effectiveValue); +} + +function toggleImageCropOverride() { + const slider = document.getElementById('imgcfg-crop'); + const effective = parseFloat(slider.dataset.effective || '0'); + imgcfgCropOverride = imgcfgCropOverride === null ? effective : null; + syncImageCropControl(effective); + markImageOverrideDirty(); +} + +async function applyImageOverride() { + const fname = imgcfgFilename; + if (!fname) return; + const status = document.getElementById('imgcfg-apply-status'); + const btn = document.getElementById('imgcfg-apply-btn'); + const usingAuto = document.getElementById('imgcfg-preset').value === '__auto__'; + + btn.disabled = true; + status.textContent = 'Saving…'; + try { + const resp = await fetch('/hokku/api/image/' + encodeURIComponent(fname) + '/config', { + method: 'PATCH', + headers: {'Content-Type': 'application/json'}, + body: JSON.stringify({ + image_config: usingAuto ? null : _getState('imgcfg'), + crop_to_fill_threshold: imgcfgCropOverride, + }), + }); + const body = await resp.json(); + if (!resp.ok) { + // The server reports every problem at once; show them all. + throw new Error((body.errors && body.errors.join('; ')) || body.error || `HTTP ${resp.status}`); + } + status.textContent = body.queued ? 'Saved — re-converting this picture…' : 'No change.'; + imgcfgDirty = false; + await loadImageOverrideState(fname); + refreshStatus(); + } catch (e) { + status.textContent = 'Failed: ' + e.message; + btn.disabled = false; + } +} + +async function clearImageOverride() { + const fname = imgcfgFilename; + if (!fname) return; + const status = document.getElementById('imgcfg-apply-status'); + status.textContent = 'Clearing…'; + try { + const resp = await fetch('/hokku/api/image/' + encodeURIComponent(fname) + '/config', { + method: 'PATCH', + headers: {'Content-Type': 'application/json'}, + body: JSON.stringify({image_config: null, crop_to_fill_threshold: null}), + }); + if (!resp.ok) throw new Error(`HTTP ${resp.status}`); + status.textContent = 'Back to automatic.'; + await loadImageOverrideState(fname); + refreshStatus(); + } catch (e) { + status.textContent = 'Failed: ' + e.message; + } +} + +// ── Compare presets ─────────────────────────────────────────────── +// +// Renders a handful of candidates for THIS picture side by side. Trying each +// setting by hand is the slow part of fixing an image that converts badly, and +// the palette LUT — the axis that actually fixes a wrong-coloured print — is +// buried in the advanced knobs where nobody finds it. + +function imageCompareCandidates() { + const current = deepCopy(_getState('imgcfg')); + const tiles = [{label: 'Current', config: current, crop: imgcfgCropOverride}]; + + Object.entries(ditherPresets).forEach(([key, p]) => { + const {label, description, ...fields} = p; + if (JSON.stringify(fields) === JSON.stringify(current)) return; // already "Current" + tiles.push({label: label || key, config: fields, crop: imgcfgCropOverride}); + }); + + // The LUT sweep is the point of the exercise: all the built-in presets share + // one palette LUT, so a picture that comes out the wrong colour cannot be + // fixed by picking a different preset. + [['oklab_hue_aware', 'OKLAB hue-aware'], + ['cam16ucs_hue_aware', 'CAM16-UCS hue-aware'], + ['euclidean_weighted', 'Weighted CIELAB']].forEach(([lut, label]) => { + if (current.dither && current.dither.lut_name === lut) return; + const cfg = deepCopy(current); + cfg.dither.lut_name = lut; + tiles.push({label, config: cfg, crop: imgcfgCropOverride}); + }); + + // Only worth showing when the picture does not already fit the panel. + const slider = document.getElementById('imgcfg-crop'); + const effectiveCrop = parseFloat(slider.dataset.effective || '0'); + const hint = document.getElementById('imgcfg-crop-hint').textContent; + if (hint && effectiveCrop < 1.0) { + tiles.push({label: 'Current, cropped to fill', config: current, crop: 1.0}); + } + return tiles; +} + +async function runImageCompareGrid() { + if (imgcfgCompareBusy) return; // a second click must not queue a second run + const fname = imgcfgFilename; + const grid = document.getElementById('imgcfg-compare-grid'); + const status = document.getElementById('imgcfg-apply-status'); + if (!fname || !grid) return; + + const tiles = imageCompareCandidates(); + grid.innerHTML = ''; + grid.hidden = false; + imgcfgCompareBusy = true; + imgcfgCompareAbort = new AbortController(); + const signal = imgcfgCompareAbort.signal; + + tiles.forEach((t, i) => { + const fig = document.createElement('figure'); + fig.className = 'imgcfg-compare-tile'; + fig.innerHTML = `${escapeHtml(t.label)}
${escapeHtml(t.label)}
`; + fig.addEventListener('click', () => { + _setState('imgcfg', deepCopy(t.config)); + if (t.crop !== undefined && t.crop !== null) { + imgcfgCropOverride = t.crop; + syncImageCropControl(parseFloat(document.getElementById('imgcfg-crop').dataset.effective || '0')); + } + selectPresetMatching(_getState('imgcfg'), 'imgcfg'); + renderDitherPanel('imgcfg'); + markImageOverrideDirty(); + status.textContent = `Loaded “${t.label}” — Apply to keep it.`; + }); + grid.appendChild(fig); + t.node = fig.querySelector('img'); + }); + + try { + // Strictly sequential. Each preview decodes the full source image, and that + // decode — not the dither — is the expensive part; the server serialises it + // anyway, and firing them in parallel would just hold several decoded images + // at once and starve the screen-serving path on a small box. + for (let i = 0; i < tiles.length; i++) { + if (signal.aborted) return; + status.textContent = `Rendering ${i + 1}/${tiles.length}…`; + const body = ditherPreviewBody('imgcfg', fname, {max_side_px: 300}); + body.image = tiles[i].config; + if (tiles[i].crop !== null && tiles[i].crop !== undefined) { + body.crop_to_fill_threshold = tiles[i].crop; + } + const resp = await fetch('/hokku/api/dither/preview', { + method: 'POST', + headers: {'Content-Type': 'application/json'}, + body: JSON.stringify(body), + signal, + }); + if (!resp.ok) throw new Error(`HTTP ${resp.status}`); + const url = URL.createObjectURL(await resp.blob()); + tiles[i].node.src = url; + tiles[i].node.dataset.url = url; // revoked when the modal closes + } + status.textContent = 'Pick the one that looks right, then Apply.'; + } catch (e) { + // Aborting the fetch does NOT stop the render already running on the + // server; the endpoint's own concurrency cap is what protects it. + if (e.name !== 'AbortError') status.textContent = 'Compare failed: ' + e.message; + } finally { + imgcfgCompareBusy = false; + } +} + // Split into two passes: // loadConfig() — called ONCE at page load. Fills every form field // (timezone, refresh times, orientation, dither, etc.) @@ -3599,20 +4030,30 @@

State

document.getElementById('debug-active-banner').style.display = debugOn ? 'block' : 'none'; document.getElementById('debug-secs').textContent = debugSecs; - // Dither presets + current state for all three panels. + // Dither presets + current state for the three Config-tab pipelines. The + // per-picture editor is mounted on demand by the image details modal. lastPanelData = data.panel || {visual_w: {{visual_w}}, visual_h: {{visual_h}}}; configDefaults = data.config_defaults || null; ditherPresets = data.dither_presets || {}; - ditherState = JSON.parse(JSON.stringify(cfg.image_config_default || {})); - bwDitherState = JSON.parse(JSON.stringify(cfg.image_config_bw || {})); - faceDitherState = JSON.parse(JSON.stringify(cfg.image_config_face || {})); - populatePresetDropdown(); // populates default + bw + face dropdowns - selectPresetMatching(ditherState, 'default'); - selectPresetMatching(bwDitherState, 'bw'); - selectPresetMatching(faceDitherState, 'face'); - renderDitherPanel('default'); - renderDitherPanel('bw'); - renderDitherPanel('face'); + mountDitherEditor('default', document.getElementById('default-editor-mount'), { + presetLabel: 'Default dither preset:', + title: 'Advanced dithering pipeline', + }); + mountDitherEditor('bw', document.getElementById('bw-editor-mount'), { + title: 'Advanced B&W pipeline', + intro: 'The image processing pipeline runs top to bottom. Hover any knob label to see what it does.', + }); + mountDitherEditor('face', document.getElementById('face-editor-mount'), { + title: 'Advanced face pipeline', + intro: 'The image processing pipeline runs top to bottom. Hover any knob label to see what it does.', + }); + populatePresetDropdown(); + ['default', 'bw', 'face'].forEach(panelId => { + const key = panelId === 'default' ? 'image_config_default' : 'image_config_' + panelId; + _setState(panelId, deepCopy(cfg[key] || {})); + selectPresetMatching(_getState(panelId), panelId); + renderDitherPanel(panelId); + }); // Pre-fill flash form WiFi fields from remembered credentials. const s1 = document.getElementById('flash-ssid'); diff --git a/python/tests/test_ui_template.py b/python/tests/test_ui_template.py new file mode 100644 index 00000000..3c244d34 --- /dev/null +++ b/python/tests/test_ui_template.py @@ -0,0 +1,96 @@ +"""Checks on the rendered web UI. + +index.html carries ~3000 lines of inline JavaScript that nothing else in the +suite executes, so a syntax error in it ships silently — the page loads, the +script dies on parse, and every control stops working. `node --check` catches +that in a second when a JS runtime is available. + +The rest of the assertions pin the structural contract between the Python side +and the page: the elements the JS mounts into, and the fields it reads. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +from hokku.webserver.app_config import AppConfig +from hokku.webserver.app_state import AppState, build_manager +from hokku.webserver.flask_app import create_app +from hokku.webserver.image_classifier import ImageClassifier +from hokku.webserver.serve_scheduler import ServeScheduler + + +@pytest.fixture +def rendered_ui(app_config: AppConfig, tmp_path: Path) -> str: + clf = ImageClassifier(app_config) + mgr = build_manager(app_config, clf) + state = AppState(app_config, clf, mgr, ServeScheduler(mgr)) + app = create_app(state, config_path=tmp_path / "cfg.json") + app.config["TESTING"] = True + resp = app.test_client().get("/hokku/ui") + assert resp.status_code == 200 + return resp.get_data(as_text=True) + + +def _inline_js(html: str) -> str: + blocks = re.findall(r"", html, re.S) + assert blocks, "no inline