Repository navigation
feat(images): smart attention crop for thumbnails + lossless profile - #36
Merged
Merged
Conversation
Two encode-quality changes to the image pipeline. **Smart attention crop.** Square-cropped renditions now use `pyvips.Interesting.ATTENTION` instead of `CENTRE`, so a portrait's subject survives the 300x300 thumbnail instead of being cut off by a geometric crop. `RenditionSpec` gains a `crop_mode` field; `crop: bool` is deliberately left alone rather than replaced, because it is read outside this module as "is this a square spec?" by `UploadRecord._filtered_renditions` and `_filter_renditions` to drop width specs wider than the source. Changing its type would have silently broken both. Cost is negligible and, unlike a naive expectation, independent of source size: `thumbnail_image` shrinks before running the saliency pass, so it always operates on a ~300px image. Measured 0-3ms across a 1MP JPEG, a 12MP HEIC and a 24.5MP HEIC, against a 50ms budget. **`lossless` profile.** A fourth `optimization` value for diagrams, screenshots, logos and flat-colour graphics where lossy artifacts around sharp edges are unacceptable. `_get_optimization_params` returns an `_EncodeParams` dataclass rather than a widening 3-tuple, because lossless is not a parameter tweak: `smart_subsample` is a chroma-subsampling control and is meaningless without lossy chroma, so it is not passed, and `Q` stops meaning quality and becomes a compression-effort knob. `max_dim` is capped at 4096 -- lossless implies wanting resolution preserved, but libwebp refuses any dimension above 16383, so an uncapped profile would fail the encode with an opaque error on large scans. Video's own `optimization` literal is untouched. Renditions stay lossy regardless of the primary asset's profile: they are CDN accelerators where transfer size is the whole point. `IMAGE_PIPELINE_VERSION` -> 3, since cropped output bytes change and a re-upload must not dedup onto a centre-cropped record. Documentation corrected against measurement rather than expectation. The "3-10x larger" rule of thumb badly understates `lossless` on photographs: a 4284x5712 HEIC (2.66 MB in) produces a 9.3 MB object in ~6s, versus 0.35 MB in 0.8s at `balanced` -- 26x the size, 7x the time, and larger than its own input, because re-encoding a lossy source losslessly grows it. Benchmarking this on a 1920x1280 test image reports ~755ms / 322 KB and looks harmless, which is the trap. On its intended content it wins decisively: a flat-colour 1920x1080 graphic is 16.7x smaller and 5.4x faster than `balanced`. CLAUDE.md also notes that a freely-selectable, unbounded-output profile is what makes the existing "no per-owner quotas" backlog gap most acute. No security validation changes. All five `write_to_buffer` call sites still pass `strip=True` (both the lossless and lossy branches), the magic-byte allow-list and SVG rejection are untouched, and placeholder extraction still sits below the `MAX_IMAGE_PIXELS` guard. Verified: 496 passed / 14 skipped; ruff check, ruff format --check and mypy clean. Both behavioural tests confirmed non-vacuous by breaking the code they cover -- forcing `crop_mode="centre"` fails the attention test, and dropping `lossless=True` fails the pixel-identity test.
`optimization=lossless` had no cost guard, and it is by far the most expensive profile: a 4284x5712 photo took ~6s and produced a 9.3MB object -- larger than its own 2.66MB input -- on the synchronous request path, freely selectable by any `upload:image` token. The obvious guards are both wrong, and measurement says why. Cost tracks content entropy, not pixel count: a 14.7MP flat-colour screenshot encodes in 262ms to 5KB, while a *smaller* 12.2MP photograph takes 5563ms and produces 6.1MB. A dimension cap would therefore block the intended use (large screenshots) and permit the abusive one (photographs). Routing lossless to the worker is equally misdirected -- the expensive case is misuse, and async would penalise the correct case with a `processing` state and a poll cycle, force the response shape to vary by parameter, and require image dedup to learn about in-flight rows. So the guard probes entropy instead: lossless-encode a 512px thumbnail and measure bytes-per-pixel. Screenshots score 0.012-0.018, photographs 0.66-1.27 -- a ~70x separation, so the 0.25 threshold is nowhere near delicate. The probe costs ~32ms and turns a ~5.5s encode into a ~250ms rejection. Verified against real content: 1080p/4K/5K screenshots still accepted at 50/117/204ms, all three real photos rejected in 33-540ms. Two non-obvious details, both learned the hard way: - The lossless branch now calls `copy_memory()` before probing. The probe and the encode are two independent pyvips sinks and would otherwise each force a full decode -- the same trap already documented on `_extract_placeholders`. Bounded by max_dim=4096, so ~50MB for this branch only. - The probe is skipped below 1MP. WebP's ~30-byte container overhead dominates bytes-per-pixel at small sizes, so an 8x8 image scores ~0.5 and would be rejected whatever it contains. This is not hypothetical: it broke `tiny.png` in two of session 3's existing tests before the floor was added. Sub-megapixel images also cannot produce an expensive encode, so nothing is lost -- a 1MP photograph measured 375ms and 0.4MB. `LOSSLESS_MAX_OUTPUT_BYTES` (8MB) is the backstop for content the probe misjudges: the CPU is already spent, but an unbounded object never reaches storage. Rejection is `LosslessUnsuitableError`, a subclass of `ImageValidationError` so every existing handler (including the bulk-upload path) still catches it. It is surfaced as a *specific* 400 rather than the usual generic one: the message is a fixed constant naming the caller's own parameter and the alternative to use, so it leaks nothing internal and the error-sanitization rule still holds -- a generic "Invalid or unsupported image" would leave the caller no way to act. Behaviour change worth noting: a small photograph uploaded with `optimization=lossless` now returns 400 where it previously succeeded. That is deliberate -- the rule is "lossless is for graphics" and applying it by size would make it incoherent -- but it is a contract change, not just a guard. Verified: 501 passed / 14 skipped; ruff check, ruff format --check and mypy clean.
Owner
Author
Added: entropy-probe guard for
|
| Content | Pixels | Time | Output |
|---|---|---|---|
| 5K screenshot | 14.7 MP | 262 ms | 5 KB |
| Photo | 12.2 MP | 5563 ms | 6.1 MB |
A larger image is 21x faster and 1200x smaller. So a dimension cap blocks the intended use and permits the abusive one. Routing to the worker is equally misdirected — the expensive case is misuse, and async would penalise the correct case with a processing state and a poll cycle, force the response shape to vary by parameter, and require image dedup to learn about in-flight rows.
The guard probes entropy instead: lossless-encode a 512px thumbnail, measure bytes-per-pixel.
| Content | bytes/px @512 |
|---|---|
| Screenshots | 0.012 – 0.018 |
| Photographs | 0.66 – 1.27 |
~70x separation, so the 0.25 threshold is nowhere near delicate. The probe costs ~32 ms and turns a ~5.5 s encode into a ~250 ms rejection.
Verified against real content:
1080p screenshot ACCEPTED 50ms
4K screenshot ACCEPTED 117ms
5K screenshot ACCEPTED 204ms
IMG_6034 (24.5MP) REJECTED 540ms (was 5978ms / 9.27MB)
IMG_6342 (12.2MP) REJECTED 250ms (was 5532ms / 6.14MB)
prof-pic (1.0MP) REJECTED 33ms
Two details learned the hard way
- The lossless branch calls
copy_memory()before probing. The probe and the encode are two independent pyvips sinks and would each otherwise force a full decode — the same trap documented on_extract_placeholdersin feat(images): add LQIP placeholders (dominant_color + blur_data_url) #34. - The probe is skipped below 1 MP. WebP's ~30-byte container overhead dominates bytes-per-pixel at small sizes, so an 8×8 image scores ~0.5 and would be rejected whatever it contains. Not hypothetical — it broke
tiny.pngin two of this PR's existing tests before the floor was added.
LOSSLESS_MAX_OUTPUT_BYTES (8 MB) is the backstop for content the probe misjudges.
Contract change worth a decision
A small photograph with optimization=lossless now returns 400 where it previously succeeded (prof-pic, 1 MP, was 375 ms / 0.4 MB). Deliberate — the rule is "lossless is for graphics" and applying it by size would make it incoherent — but it is a contract change, not just a guard. Raising _LOSSLESS_GUARD_MIN_PIXELS would exempt small photos if you'd rather.
Rejection is LosslessUnsuitableError, a subclass of ImageValidationError so every existing handler still catches it, surfaced as a specific 400 naming the caller's parameter and the alternative — a generic "Invalid or unsupported image" would leave them no way to act.
501 passed, 14 skipped; ruff + format + mypy clean.
…tographic The previous guard thresholded the probe's bytes-per-pixel ratio directly, which rejected *any* photographic content and needed an arbitrary 1MP floor to stop WebP's container overhead from rejecting tiny images too. Both were symptoms of measuring the wrong thing. The ratio only says "this is photographic", and that is not by itself a problem. Measured on crops of one photo: 0.25MP encodes losslessly in 126ms to 0.27MB, 1MP in 389ms to 0.99MB, 12.2MP in 5363ms to 6.14MB. The small ones are entirely affordable and were explicitly asked for by the caller, so rejecting them made the guard do something its own purpose -- preventing expensive encodes -- did not justify. The probe's bytes-per-pixel turns out to be near-constant for a given image regardless of size (1.265-1.297 across that whole 0.25MP-12MP range), so it is a pure content signal. Multiplying it by the real pixel count projects the output size, which tracks the actual result within ~1.5x -- enough to decide before paying for the encode. Gate on that against LOSSLESS_MAX_OUTPUT_BYTES, which is also what the post-encode backstop already checked, so the two halves of the guard now express one idea instead of two. This removes the small-image exemption entirely rather than tuning it: an 8x8 image scores ~0.5 bytes/px (indistinguishable from a photo, which is how the previous version rejected `tiny.png`), but 0.5 x 64 pixels projects to 30 bytes, so it passes on the projection with no special case. Verified against real content: 1080p/4K/5K screenshots still accepted at 41/108/214ms; a 24.5MP photo rejected in 492ms (projected 14.4MB) and a 12.2MP one in 227ms (15.4MB); and prof-pic.jpg -- 1MP, photographic, 413ms, 0.40MB -- is now correctly accepted where the previous guard rejected it. `LOSSLESS_MAX_PROBE_BYTES_PER_PIXEL` is gone; `LOSSLESS_MAX_OUTPUT_BYTES` is the single knob. The 400 detail no longer claims the image "is photographic", since the rejection is about projected cost; it now says the encode would produce an oversized object and names optimization=quality. The two route tests added in 7422934 asserted the old behaviour on a 1.1MP input, which is now (correctly) accepted -- resized to 6.8MP and re-worded. Verified: 504 passed / 14 skipped; ruff check, ruff format --check and mypy clean.
|
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.
Two encode-quality changes to the image pipeline.
Smart attention crop
Square-cropped renditions now use
pyvips.Interesting.ATTENTIONinstead ofCENTRE, so a portrait's subject survives the 300×300 thumbnail instead of being cut off geometrically.RenditionSpecgains acrop_modefield.crop: boolwas deliberately left alone rather than replaced — it is read outside this module as "is this a square spec?" byUploadRecord._filtered_renditionsand_filter_renditions, to drop width specs wider than the source. Changing its type would have silently broken both (a truthy string makes every width spec look like a crop spec, so the filter stops firing and responses start advertising renditions that were never encoded).Cost is negligible and independent of source size —
thumbnail_imageshrinks before running the saliency pass, so it always operates on a ~300px image:Against a 50 ms budget.
losslessprofileA fourth
optimizationvalue for diagrams, screenshots, logos and flat-colour graphics._get_optimization_paramsnow returns an_EncodeParamsdataclass rather than a widening 3-tuple, because lossless is not a parameter tweak:smart_subsampleis a chroma-subsampling control and is meaningless without lossy chroma, so it is not passed.Qstops meaning quality and becomes a compression-effort knob.max_dimis capped at 4096. Lossless implies wanting resolution preserved, but libwebp refuses any dimension above 16383, so an uncapped profile would fail the encode with an opaque error on large scans.Video's own
optimizationliteral is untouched. Renditions stay lossy regardless of the primary asset's profile — they are CDN accelerators where transfer size is the whole point.IMAGE_PIPELINE_VERSION→ 3, since cropped output bytes change and a re-upload must not dedup onto a centre-cropped record.Docs corrected against measurement
The "3–10× larger" rule of thumb badly understates
losslesson photographs:balancedlossless26× the size, 7× the time, and larger than its own input — re-encoding a lossy source losslessly grows it. Six seconds also matters for client and CDN read timeouts.
Benchmarking this on a 1920×1280 test image reports ~755 ms / 322 KB and looks harmless, which is the trap. On its intended content it wins decisively: a flat-colour 1920×1080 graphic is 16.7× smaller and 5.4× faster than
balanced.CLAUDE.mdalso records that a freely-selectable, unbounded-output profile is what makes the existing "no per-owner quotas" backlog gap most acute.No security validation changes
Checked explicitly rather than assumed:
write_to_buffercall sites still passstrip=True— including both the new lossless branch and the lossy one, so EXIF/GPS/ICC stripping is not bypassed by the new profile.MAX_IMAGE_PIXELSguard, so the decompression-bomb ordering fixed in feat(images): add LQIP placeholders (dominant_color + blur_data_url) #34 is intact.optimizationis aLiteral, so an unknown value is a 422 before anything reaches libvips.Verification
496 passed, 14 skipped·ruff check·ruff format --check·mypy app— all clean viadocker compose run --rm --build test.Both behavioural tests confirmed non-vacuous by breaking the code they cover: forcing
crop_mode="centre"fails the attention test, and droppinglossless=Truefails the pixel-identity test.