Per-picture dithering and crop overrides - #35
Merged
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
_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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
_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) <noreply@anthropic.com>
…n 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) <noreply@anthropic.com>
…view crop GET/PATCH /hokku/api/image/<name>/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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
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 <select>, hidden and holding one filename, so the shared
function needs no special case.
The details modal gains two independent overrides, matching the two record
fields. The dither dropdown has an "Automatic" entry the global pipelines
cannot have - there is nothing above them to defer to. The letterbox control
starts in automatic and shows the global value until you take it over; the
minimum fill this picture needs, which the details table already computed, is
now printed where it is actionable.
Compare presets renders a handful of candidates for this one picture and lets
you click the winner. Along with the presets it sweeps the palette LUT, which
is the point: all three built-in presets share one LUT, so a picture that comes
out the wrong colour cannot be fixed by choosing a different preset - the fix
lives in a knob most people will never open. Tiles render strictly one at a
time; the decode dominates the cost and the server serialises it anyway, so
firing them in parallel would only hold several decoded images at once.
The modal had no max-height at all, so the advanced knobs would have run off
the bottom of the viewport with the Apply button unreachable. It now caps and
scrolls, and widens while the knobs are open.
Adds test_ui_template.py, which runs `node --check` over the page's inline
script. Roughly 3000 lines of JavaScript had no check of any kind: a syntax
error there ships silently, since the page still loads and only the script
dies. It also pins the mount points and asserts the hand-written copies have
not come back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dithering.md section 6 gains the override step and, more importantly, why it is applied by the image manager rather than by the classifier: the classifier observes content and is rebuilt on every config reload, while overrides are user intent living in the manager's DB. Overlaying on the finished decision is also what keeps the face keep-out and crop-anchor boxes by construction. Section 12 documents the editor component and its four mounts, and replaces a claim that was simply false: it said the config parser "validates every field and raises on any missing key", when the parser it named merged missing fields onto the defaults and ignored unknown ones. That is now true of the parser it describes. manual.md section 1.1 had promised "you can override per-image in Config" for a feature that did not exist. It now documents the one that does, leading with Compare presets, since that is the fastest route for the case that prompted this - a picture that converts to the wrong colours - and mentions the two surprising lifetimes (survives a re-upload of the same name, lost on delete). Version bumped to the dev formula: 4.0.1.dev10 / 4.0.1~dev.10-1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Generating the editor from one component accidentally gave the B&W pipeline a "show face CLAHE keep-out zones" preview overlay it never had. That looked like drift being repaired; it was not. The classifier's dispatch returns face bounding boxes on the face branch alone - both the B&W and the default branch return an empty tuple - so CLAHE keep-out only ever runs for a picture routed to the face pipeline. Offering the control anywhere else advertises a stage that cannot occur for those images. It is now an explicit option, on for the face pipeline and for the per-picture editor, off for B&W and for the default pipeline. The default pipeline had the control before and equally could not use it, so this removes it there too. The per-picture editor keeps it because an override replaces the choice of pipeline, not the detection: a face-routed picture keeps its keep-out boxes even when pinned to a different dither. The control hides itself when the picture has no detected faces. Nothing about this affected rendering - the overlay is a canvas drawn over the preview, and keep-out is an L* operation that cannot introduce colour, still less through the B&W pipeline's two-ink LUT. It was misleading, not harmful. Pins the underlying invariant in test_image_classifier: a B&W photo yields no keep-out boxes even when a face is detected in it. If that dispatch ever changes, the UI rule has to change with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A fresh install showed "Custom (your edits)" in all three pipeline dropdowns. Nothing was wrong with the config - the UI picks the selected entry by matching the pipeline config against the preset catalog, and the shipped defaults were deliberately kept out of that catalog, so nothing ever matched. It read as though someone had already been editing the settings. The three defaults are now the first three catalog entries, and are the same objects as DEFAULT_IMAGE_CONFIG / DEFAULT_BW_IMAGE_CONFIG / DEFAULT_FACE_IMAGE_CONFIG rather than copies, so they cannot drift out of agreement with what the server actually dispatches to. Keeping them out was a considered decision, on the grounds that the face tuning is deliberately gentle and makes a poor general-purpose starting point. That is still true, so its description says so outright - but it is a reason to describe the setting accurately, not to hide one the server ships with. The three hand-picked entries stay as alternatives, and are not redundant: atkinson_hue_aware differs from default_general in serpentine scan and in doing saturation and DRC in CIELAB rather than OKLAB. The manual now spells that out, since two entries both labelled Atkinson is otherwise a fair thing to be confused by. FALLBACK_PRESET now points at default_general. It has no production callers left - the strict parser removed the last one - and is only a canonical starting point for tests. Tested at both levels, because they catch different things: that each default IS a catalog entry and serialises byte-identically to it (key order included, since the browser compares JSON strings), and that the /api/config payload the browser actually receives resolves each pipeline to the right preset key. The second found a mistake in the first draft of the first, so both stay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The catalog lists the three shipped defaults first so they lead the dropdown, but that order never reached the browser: jsonify sorts object keys, so the presets arrived alphabetically and the list opened on "Atkinson (hue-aware)" with the defaults scattered through it. The payload now carries dither_preset_order and the dropdown iterates that, falling back to whatever key order it gets if the field is missing. Caught on the deployed test server, not in the suite. The test that was supposed to cover this asserted the order of the Python dict, which was never the thing at risk - the wire format was. There is now a payload-level test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes half of #18 — the per-picture dithering half. Tagging/collections is untouched.
Some pictures convert badly and no global setting fixes them without spoiling everything else. The reporter's case was a Simpsons print that comes out blue; they tried every preset and none helped, which is unsurprising — all three built-in presets share
lut_name="hue_aware", so the axis that actually fixes a wrong-coloured picture was reachable only through the advanced knobs.What this adds
Two independent nullable fields on
ImageRecord, set from the image's Details panel:image_config— pin this picture to its own pipelinecrop_to_fill_threshold— pin this picture's letterbox/crop behaviourPlus Compare presets, which renders several candidates for that one picture into a clickable grid and sweeps the palette LUT alongside the named presets — the fastest route to the reported problem.
Setting an override re-renders exactly that picture; every other cached render stays valid. Overrides survive Clear Cache & Re-convert and a re-upload of the same filename, and are dropped when the image is deleted.
How it fits together
Overrides are applied by the image manager, in
_decision_for_record(), not byImageClassifier. The classifier observes content and is rebuilt on every config reload, while overrides are user intent living in the manager's DB — the wrong lifetime. Overlaying on the finished decision also keepsclahe_keepout_bboxesandface_crop_bboxesby construction: an override replaces the choice of pipeline, never the observations. Both fields feedScreenImageConfig.cache_slug(), so invalidation needs no new machinery.The dither editor became one component mounted four times (the three Config pipelines plus the per-picture one). It had been written out three times by hand and the copies had already drifted — the B&W panel was missing an overlay the other two had.
Fixes found along the way
reload()builds the replacement manager (which readsimage_manager.json), then shut down the outgoing one, whoseshutdown()flushed its stale snapshot back over that file — and a multi-threaded manager's render callbacks kept writing after the swap. It only cost recomputable fields before; it would have cost the new per-picture settings. Split intoshutdown()(flush then freeze) andretire()(freeze without flushing).ImageConfigis now parsed strictly everywhere — every field required, enums checked, unknown keys rejected. Config-shape changes move onto theAppConfigmigration chain (v10), which completes older blobs once. Previously a misspelled knob was silently ignored on every load and a badlut_namesurfaced much later as a failed conversion.render_preview_pngwas called positionally so the threshold took its0.0default — every preview letterboxed, while the face-box overlay on the next line was computed for a cropped canvas. Previews are also now bounded in concurrency; they are decode-bound and were unlimited.waitresswas missing fromrequirements.txt, so recreating the venv as AGENTS.md documents produced an environment where the server would not start.Testing
994 passing, ruff and pyright clean. New coverage includes the DB-wipe salvage path, the strict parser's rejection cases, the routes, and
node --checkover the page's inline script — ~3000 lines that had no check of any kind, where a syntax error ships silently because the page still loads.Deployed to
hokku-test.localand exercised end to end:GET/PATCHbehaviour, a malformed payload rejected with the record untouched, a crop override re-converting one picture out of 17, and the dropdown resolving to the named defaults.Worth a reviewer's eye
/hokku/api/dither/previewstill applies keep-out from cached boxes regardless of which pipeline's config it is given, so a preview can show keep-out where production would not. Pre-existing; flagged rather than folded in.Docs updated:
dithering.md§4/§6/§12 andmanual.md§1.1/§1.3, including two claims that were simply untrue — a parser described as raising on missing keys when it silently filled defaults, and a manual promising per-image overrides in Config that did not exist.🤖 Generated with Claude Code