Conversation
It pins clang-format-19; the system clang-format is a different version and produces different output, which fails the CI format check.
ThumbHash (https://github.com/evanw/thumbhash, MIT) encodes a ~20-25 byte placeholder that a receiving client renders as a blurred preview while the real attachment downloads. It is a better fit than BlurHash on every axis we care about: natively binary rather than base83, alpha support, and it spends its bits like a codec (7x7 DCT on luminance, 3x3 per chroma axis, 5x5 on alpha) instead of splitting them evenly across R, G and B. Measured against 13 photos, scored by RMSE against the original downsampled to 32px: thumbhash 20.5 bytes / 29.25, raw 4x4 pixels 48 bytes / 30.87, BlurHash 4x3 28 bytes / 36.68. Smallest and best of everything tried. There is no upstream C or C++ implementation, so this is a port, with three deliberate departures: - The DCT basis goes through src/image/det_trig.hpp rather than std::cos, and every a*b+c is an explicit std::fma. Neither std::cos's accuracy nor whether the compiler contracts a multiply-add is fixed by the standard, and both change the emitted hash: V8's cos differs from glibc's by 1 ulp on ~3.5% of the arguments this DCT uses, which alters ~7% of hashes. Since a thumbhash is sent to other people, that would make it a weak fingerprint of the sending platform. Exact integer argument reduction also happens to be 27x more accurate than the reference formulation, which hands libm a thrice-rounded angle. Verified identical across 7 GCC and 2 Clang configurations spanning -O0..-O3, -march=native, -mfma and -ffp-contract both fast and off. - The basis tables are hoisted out of the innermost loops; the reference rebuilds them per (cx, cy) when encoding and per pixel when decoding. Accumulation order is untouched, so this is bit-identical, and about 3x faster each way: encode 0.11ms at 32x32, decode 0.085ms at 32x24. - Decoding is resolution-independent, so there is no upscale-and-blur step and no image library needed on the receive path. Keep the decode small and let the UI scale it: at 32px, upscaling 10x with any linear filter differs from a full-size decode by RMSE 0.87, which is imperceptible. The hash therefore differs from upstream encoders by at most 1 in an individual 4-bit AC coefficient. Decoders remain fully interoperable; nothing requires two encoders to agree. THUMBHASH_REFERENCE_COS switches the basis back to the upstream std::cos formulation, which is what the algorithm is defined as and what its published vectors were produced with; it is there to keep that legible and measurable, and must not be defined in production. Two tests guard the parts that would otherwise fail silently. The pinned hash vectors catch a build that reintroduces a platform dependency, rather than letting it leak. The det_trig case checks the hand-written Taylor coefficients against libm across the DCT's whole argument set, because a mistranscribed factorial still yields smooth, plausible output -- just with the wrong values in every hash. The API is shaped so the safe path is the obvious one. decode() takes explicit dimensions, because a hash carries no usable record of the source's shape: what it stores is the DCT component counts, which a decoder needs in order to know how many luminance AC coefficients precede the chroma terms. That parameter tracks the shape loosely, so upstream exposes it as an aspect ratio, but one side is always pinned at the per-channel maximum and the complete set of results is 7/n for n in 1..7 and reciprocals (13 values), or 5/n for n in 1..5 with alpha. It also saturates, so a 1000x100 banner reads as 7.0 rather than 10.0 and a 1x100 sliver is wrong by a factor of fourteen. It is therefore named component_aspect_ratio(), and the overload that guesses an output shape from it is decode_unsized(); both are documented as diagnostics for a hash that arrives with no metadata, not as part of a display path. valid() and expected_size() are for a carrier storing peer-supplied values. A hash's length is not merely bounded but fully determined by its own header -- only six lengths are reachable at all -- so a receive path can check exact structural validity for the cost of a few bit extractions, which is strictly stronger than a length cap that any blob of the right size would pass. Note for the libvips work: encode() takes RGBA at <=100x100, so the caller does the downscaling, and the scaler is now the weakest link for reproducibility -- two clients that downscale the same photo differently produce different hashes.
| image decode(std::span<const std::byte> hash, uint32_t width, uint32_t height) { | ||
| if (width < 1 || height < 1) | ||
| throw std::invalid_argument{"thumbhash: output dimensions must be non-zero"}; | ||
| auto hd = read_header(hash); | ||
| int w = int(width), h = int(height); | ||
|
|
There was a problem hiding this comment.
should we enforce a max width/height here?
Claude report:
decode() has no upper bound on output dimensions; the pixel index overflows and writes out of bounds
decode() validates only that width and height are non-zero, and the header documents no upper bound (thumbhash.hpp#L64-L69 says only "each at least 1").
Confirmed by running, against the PR's own source:
decode(hash, 32768, 16384) — exactly 2^29 px — built with -fsanitize=signed-integer-overflow -fno-sanitize-recover: runtime error: signed integer overflow: 2147483644 + 4 cannot be represented in type 'int', reported at line 337.
decode(hash, 32768, 20480) — 671 M px — no sanitizer: SIGSEGV (exit 139), from the out-of-bounds store at lines 367–370.
Three related hazards on the same unguarded path:
-
int w = int(width)at line298is a silent narrowing; forwidth > INT_MAXit yields a negativew, after which the loops do not run and the size computations wrap. In practice that path always ends instd::length_error/std::bad_allocrather than returning an image whose buffer contradicts itswidth/height(I checkeddecode(hash, 2^31, 2^31)anddecode(hash, 2^32-1, 1)— both throw). But the thrown type is not thestd::invalid_argumentthe docstring promises -
Where
size_tis 32 bits (any 32-bit Android/iOS ABI),size_t(w) * h * 4at line322wraps, soout.rgbais under-allocated while the loops still writew*h*4bytes — a heap overflow at the same pixel count but with no huge allocation needed first. I could not execute this (no 32-bit multilib available here), but the expression is plainly 32-bit-unsafe. -
det_trig.hpp#L109 computes
2 * i + 1in int, which overflows fori >= 2^30; i there is the output pixel coordinate, bounded only by the same missing check
failure scenario: a client calls thumbhash::decode(hash, att.width, att.height) with dimensions taken from peer-supplied attachment metadata — the docstring tells callers to "pass the attachment's real dimensions", and only advises, without enforcing, that they be scaled down. A peer advertising a 32768×20480 image crashes the receiving client.
Fix: reject oversized outputs up front and index with size_t:
if (width < 1 || height < 1)
throw std::invalid_argument{"thumbhash: output dimensions must be non-zero"};
if (size_t(width) * height > max_decode_pixels) // e.g. 4096*4096; generous for a placeholder
throw std::invalid_argument{"thumbhash: output too large"};and change for (int y = 0, i = 0; ...) to a size_t i. A documented max_output_pixels constant next to max_input_dimension would make the bound part of the contract. The same cap also removes the 32-bit wrap and the det_trig overflow.
There was a problem hiding this comment.
There's no strict max here, and no value that fails, but astronomical values will be slow. The better approach here is to just document that the intention here is to be quite small -- e.g. 100x100 -- and then if you need bigger, use libvips to scale that up with a non-pixelated scaling which should work quite well for these blurred images.
There was a problem hiding this comment.
Correcting my earlier reply on this thread: I said there was no value that fails, only values that are slow. That was wrong — there was a genuine out-of-bounds write.
The output index was an int incremented by 4 per pixel, so it wrapped past 2^29 output pixels and the stores then landed outside the buffer:
decode(hash, 32768, 16384) UBSan: signed integer overflow, 2147483644 + 4, thumbhash.cpp:337
decode(hash, 32768, 20480) Segmentation fault (exit 139)
Both now run to completion and return a correct buffer.
I have still not added a policy cap, since I do not think one belongs here — a large decode is legitimately just slow. What changed is that the arithmetic now survives one:
- the pixel index is
size_trather thanint - the byte count is computed in
uint64_tand rejected withstd::invalid_argumentif it would not fit asize_t, which is what would otherwise under-allocate on a 32-bit target and overflow at a much lower pixel count - dimensions are bounded to int32 so the
intloop counters, anddet_trig's2*i+1, cannot overflow either
and the decode docs now say what this is for: decode at 100x100 or less and scale the result up with libvips, which handles a blurred image like this very well. Decoding large gains nothing — the hash holds at most 7 cycles across the image, so a 32px decode already captures essentially all of it.
Thanks for pushing on this one; my "no value fails" was an assumption I had not tested.
| TEST_CASE("thumbhash is bit-reproducible", "[image][thumbhash]") { | ||
| struct { | ||
| int w, h; | ||
| bool alpha; | ||
| std::string_view expected; | ||
| } const vectors[] = { | ||
| {32, 32, false, "1b67067f262062763f9a885289d879678789670777909a09"sv}, | ||
| {64, 48, false, "1b67067da62062763f9a885289d87976767007a999"sv}, | ||
| {48, 64, false, "1b67067d2620623f9a2895d879769878767007a999"sv}, | ||
| {32, 32, true, "9d3782250a2617b138d871efbd777007b99808688888808968"sv}, | ||
| {64, 48, true, "9d3782248c3717a138d871ef7d0777908b8980868808988806"sv}, | ||
| {100, 100, false, "1b67067f261061763f9a885189e879678789670777909a09"sv}, | ||
| {17, 5, true, "9d378219883506b037d883777007b98828788888808a78"sv}, | ||
| {1, 1, false, "d5102ad70708f808888888808f8088f80888808ff8088800"sv}, | ||
| }; | ||
| for (const auto& v : vectors) { | ||
| auto hash = thumbhash::encode(test_image(v.w, v.h, v.alpha), v.w, v.h); | ||
| INFO(v.w << "x" << v.h << (v.alpha ? " rgba" : " rgb")); | ||
| CHECK(oxenc::to_hex(hash) == v.expected); | ||
| } | ||
| } |
There was a problem hiding this comment.
Claude report, this seems like a good thing.
Nothing tests the decoder; eight injected decoder bugs all pass the full suite
The pinned vectors pin the encoder only. The round-trip test checks buffer sizes and the alpha flag but never inspects a decoded pixel; the resolution-independence test compares the decoder against itself; and the average-colour test has a ±24/255 tolerance between two quantities that move together (both average_rgba and the decoded mean read the same DC terms).
Fix: pin decoded output the way encoded output is pinned. For each entry in the existing vectors[] table, decode at a small fixed size (e.g. 8×6) and compare a hex dump of the RGBA bytes. That is one extra TEST_CASE reusing the same table, and it kills all eight mutants above. Better still, generate the expected bytes with upstream's JS decoder, which would pin interoperability rather than just regression.
| } | ||
|
|
||
| // Number of AC coefficients a channel contributes: the triangular set the format keeps, minus | ||
| // the DC term. Must stay in step with the loop in `decode_at`'s decode_channel. |
There was a problem hiding this comment.
Comment points at a function that does not exist (decode_at)
| double b = std::fma(-(2.0 / 3.0), hd.p_dc, hd.l_dc); | ||
| double r = (std::fma(3.0, hd.l_dc, -b) + hd.q_dc) / 2; | ||
| double g = r - hd.q_dc; | ||
| auto to8 = [](double v) { return std::byte(uint8_t(std::max(0.0, 255 * std::min(1.0, v)))); }; | ||
| return {to8(r), to8(g), to8(b), to8(hd.a_dc)}; |
There was a problem hiding this comment.
DRY: the LPQA→RGB conversion and the to8 clamp are written twice
src/image/thumbhash.cpp#L287-L291 and #L361-L366 are character-for-character the same three-line colour conversion plus the same to8 lambda. The repo's CLAUDE.md asks for exactly this ("when logic is duplicated across two or more call sites, extract a shared helper… proactively when writing new code"). A std::array<std::byte, 4> lpqa_to_rgba8(double l, double p, double q, double a) in the anonymous namespace serves both and guarantees average_rgba can never drift from the per-pixel path.
| shape read_shape(std::span<const std::byte> hash) { | ||
| shape s{}; | ||
| uint32_t h16 = byte_at(hash, 3) | (uint32_t(byte_at(hash, 4)) << 8); | ||
| s.has_alpha = (byte_at(hash, 2) & 0x80) != 0; | ||
| bool landscape = (h16 >> 15) != 0; | ||
| s.lx = std::max(3, landscape ? (s.has_alpha ? 5 : 7) : int(h16 & 7)); | ||
| s.ly = std::max(3, landscape ? int(h16 & 7) : (s.has_alpha ? 5 : 7)); | ||
| s.ac_start = s.has_alpha ? 6 : 5; | ||
| return s; | ||
| } |
There was a problem hiding this comment.
Claude report, but I think it got this one wrong. At least I don't see how this case can happen?
valid() accepts a header no encoder can emit, and the rest of the API then calls it malformed
src/image/thumbhash.cpp#L108-L117, #L165-L178, #L273-L283
read_shape clamps the stored 3-bit component count with std::max(3, ...), so a stored count of 0 is silently treated as 3 and expected_size/valid accept it. component_aspect_ratio does not clamp, and throws when the resulting ly is zero. The encoder never emits 0 — it clamps to std::max(1, ...) at L208-L209 — so 0 is unambiguously malformed, yet valid() passes it.
Verified: take a hash from a 100×43 encode (landscape, stored count 3, 17 bytes) and clear the low three bits of byte 3.
valid = 1, expected_size = 17, actual size = 17
component_aspect_ratio -> throws "thumbhash: invalid component count"
decode_unsized -> throws "thumbhash: invalid component count"
decode(16, 16) -> succeeds
The portrait variant of the same corruption fails differently again: it does not throw, it returns a ratio of 0, and decode_unsized then produces a 1×32 image.
This undercuts what valid() is sold as at thumbhash.hpp#L86-L94 ("what a receive path should test before storing a value a remote peer supplied"): a caller that stored the value on the strength of valid() and later calls the documented no-metadata path, decode_unsized, gets an exception from a value it was told was well-formed. Both functions do document that they throw, so this is a consistency defect rather than a memory-safety one — but the fix is one line, and skipping it leaves a structural check with a hole in it.
Fix: reject the impossible count in expected_size, so the trust-boundary check rejects what the rest of the API rejects:
// in read_shape, keep the raw value: s.stored = int(h16 & 7);
auto s = read_shape(hash);
if (s.stored == 0) // no conforming encoder emits 0
return std::nullopt;
While there, consider having component_aspect_ratio derive lx/ly from read_shape rather than re-deriving them from the raw bytes: that removes the third copy of this bit-unpacking and makes the clamp asymmetry impossible to reintroduce.
There was a problem hiding this comment.
Reproduced — it is reachable, just not from our own encoder.
The stored component count is a 3-bit field a peer controls, and valid() is documented as the check to run before storing a peer-supplied value, so "no conforming encoder emits this" is exactly the case it exists to screen. Taking a real 100x43 hash and clearing the low three bits of byte 3:
valid() = 1 <- accepted
expected_size() = 17, actual = 17
component_aspect_ratio() THREW: thumbhash: invalid component count
decode_unsized(32) THREW: thumbhash: invalid component count
The decoder's max(3, ...) clamp waves the 0 through, while component_aspect_ratio reads the field unclamped and rejects it — so a carrier that stored the value on the strength of valid() then gets an exception out of the documented no-metadata path. expected_size() now rejects a stored count of 0, so the trust boundary agrees with the rest of the API.
I did not take the second half of the suggestion, though: having component_aspect_ratio derive lx/ly from read_shape would change results rather than just deduplicate. read_shape clamps to max(3, ...) because the DCT needs three components per axis, whereas component_aspect_ratio deliberately reports the unclamped stored value, which is what upstream thumbHashToApproximateAspectRatio does. Adopting it would make a 1000x100 banner report 7/3 instead of 7/1, collapsing the reachable set from 13 ratios to 9 and breaking the pinned {100, 10, 7.0} case. The asymmetry is intentional and is now commented as such.
| // Only eight lengths are reachable at all. | ||
| CHECK(lengths == std::set<size_t>{17, 19, 21, 23, 24, 25}); |
There was a problem hiding this comment.
nitpick
Test comment says "eight lengths" for a set of six
Review fixes for session-foundation#172. decode() indexed its output with an `int` that wrapped at 2^29 output pixels, after which the stores went outside the buffer: decode(hash, 32768, 16384) is a signed overflow under UBSan, and decode(hash, 32768, 20480) segfaults. The index is now size_t, the byte count is computed in uint64_t and rejected if it would not fit a size_t (which is what would otherwise under-allocate on a 32-bit target and overflow the same way at a much lower pixel count), and the dimensions are bounded to int32 so the `int` loop counters and det_trig's 2*i+1 cannot overflow either. No policy cap: a big decode is merely slow, as intended -- the header now says to decode at 100x100 or less and scale the result up with libvips, which suits a blurred image well. valid() accepted a stored component count of 0. No conforming encoder emits one -- both this encoder and upstream clamp to max(1, ...) -- but it is a bit-field a peer controls, and while the decoder's max(3, ...) clamp waves it through, component_aspect_ratio reads the field unclamped and rejects it. A carrier that stored a value on the strength of valid() could then get an exception out of the documented no-metadata path. expected_size() now rejects it, so the trust boundary agrees with the rest of the API. Nothing tested the decoder. The pinned vectors cover the encoder; the round-trip test checks buffer sizes but no pixels, resolution-independence compares the decoder against itself, and the average-colour test compares two quantities that both derive from the same DC terms. A decoder producing consistently wrong pixels passed the entire suite. Its output is now pinned at 8x6 for the same eight vectors. Also: extract lpqa_to_rgba8, which decode() and average_rgba() had character-for-character in common, so the flat-colour placeholder cannot drift from the image it stands in for; fix a comment naming decode_at, which was renamed to decode; and correct a test comment saying "eight lengths" about a set of six. The encoder is untouched: its pinned vectors are unchanged across -O0..-O3, -march=native, -mfma and -ffp-contract fast/off.
ThumbHash (https://github.com/evanw/thumbhash, MIT) encodes a ~20-25 byte placeholder that a receiving client renders as a blurred preview while the real attachment downloads. It is a better fit than BlurHash on every axis we care about: natively binary rather than base83, alpha support, and it spends its bits like a codec (7x7 DCT on luminance, 3x3 per chroma axis, 5x5 on alpha) instead of splitting them evenly across R, G and B.
Measured against 13 photos, scored by RMSE against the original downsampled to 32px: thumbhash 20.5 bytes / 29.25, raw 4x4 pixels 48 bytes / 30.87, BlurHash 4x3 28 bytes / 36.68. Smallest and best of everything tried.
There is no upstream C or C++ implementation, so this is a port, with three deliberate departures:
The DCT basis goes through src/image/det_trig.hpp rather than std::cos, and every a*b+c is an explicit std::fma. Neither std::cos's accuracy nor whether the compiler contracts a multiply-add is fixed by the standard, and both change the emitted hash: V8's cos differs from glibc's by 1 ulp on ~3.5% of the arguments this DCT uses, which alters ~7% of hashes. Since a thumbhash is sent to other people, that would make it a weak fingerprint of the sending platform. Exact integer argument reduction also happens to be 27x more accurate than the reference formulation, which hands libm a thrice-rounded angle. Verified identical across 7 GCC and 2 Clang configurations spanning -O0..-O3, -march=native, -mfma and -ffp-contract both fast and off.
The basis tables are hoisted out of the innermost loops; the reference rebuilds them per (cx, cy) when encoding and per pixel when decoding. Accumulation order is untouched, so this is bit-identical, and about 3x faster each way: encode 0.11ms at 32x32, decode 0.085ms at 32x24.
Decoding is resolution-independent, so there is no upscale-and-blur step and no image library needed on the receive path. Keep the decode small and let the UI scale it: at 32px, upscaling 10x with any linear filter differs from a full-size decode by RMSE 0.87, which is imperceptible.
The hash therefore differs from upstream encoders by at most 1 in an individual 4-bit AC coefficient. Decoders remain fully interoperable; nothing requires two encoders to agree. THUMBHASH_REFERENCE_COS switches the basis back to the upstream std::cos formulation, which is what the algorithm is defined as and what its published vectors were produced with; it is there to keep that legible and measurable, and must not be defined in production.
Two tests guard the parts that would otherwise fail silently. The pinned hash vectors catch a build that reintroduces a platform dependency, rather than letting it leak. The det_trig case checks the hand-written Taylor coefficients against libm across the DCT's whole argument set, because a mistranscribed factorial still yields smooth, plausible output -- just with the wrong values in every hash.
The API is shaped so the safe path is the obvious one. decode() takes explicit dimensions, because a hash carries no usable record of the source's shape: what it stores is the DCT component counts, which a decoder needs in order to know how many luminance AC coefficients precede the chroma terms. That parameter tracks the shape loosely, so upstream exposes it as an aspect ratio, but one side is always pinned at the per-channel maximum and the complete set of results is 7/n for n in 1..7 and reciprocals (13 values), or 5/n for n in 1..5 with alpha. It also saturates, so a 1000x100 banner reads as 7.0 rather than 10.0 and a 1x100 sliver is wrong by a factor of fourteen. It is therefore named component_aspect_ratio(), and the overload that guesses an output shape from it is decode_unsized(); both are documented as diagnostics for a hash that arrives with no metadata, not as part of a display path.
valid() and expected_size() are for a carrier storing peer-supplied values. A hash's length is not merely bounded but fully determined by its own header -- only six lengths are reachable at all -- so a receive path can check exact structural validity for the cost of a few bit extractions, which is strictly stronger than a length cap that any blob of the right size would pass.
Note for the libvips work: encode() takes RGBA at <=100x100, so the caller does the downscaling, and the scaler is now the weakest link for reproducibility -- two clients that downscale the same photo differently produce different hashes.