[bong-water-water-bong]

Refs #2324 (pictures and the picture prefix cache on --device npu). The 35B row follows on this route as a device run; it needs no code change.

What was missing. The engine had every step for a picture (the vision pack reader, the host tower checked against transformers, plan_image, preprocess, image_tokens, prompt_positions), chained only by minicpm_image_probe. forward-serve silently dropped image_url parts: the ChatML path keeps text only, and the template path rendered <|image_pad|> with nothing behind it.

Change.

• app/npu_image_request.h: an OpenAI request's pictures, in message order, as RGB bytes.
  • It matches the same part shapes the MiniCPM-V template turns into a pad.
  • Only data: URLs are taken; nothing is fetched from the network.
  • PNG, JPEG and binary PPM are decoded by stb_image, pinned through FetchContent like nlohmann_json and built in app/stb_image_impl.cpp with only those three formats and no file I/O.
  • A picture over 16 M pixels, a part with no URL, a non-base64 URL and bytes that are no image are each refused by name.
• npu/vision_request.h:
  • expand_images() replaces the k-th <|image_pad|> with <image_id>k</image_id> plus the picture's tokens, and lays out the three position planes.
  • The ids, grids, positions and an FNV-1a hash of every picture's size and bytes come from the plan alone.
  • tower_rows() runs preprocess and the host tower separately.
• app/forward_serve.cpp:
  • The vision component is loaded at start when the pack ships one. Pictures on a pack without one, or on the ChatML fallback, are a 400.
  • The prefill goes through one helper: step_batch for text, or step_batch_embeds with that chunk's rows and planes for a prompt with pictures. So chunking, pinning and resume are unchanged.
  • Decoding resumes at the planes' next position while the cache slot counts from the prompt's length, as the probe does.
  • Speculative decoding is off for such a request, since its rotary positions are not the drafter's slots.
• The prompt cache is keyed on the ids, with each picture row replaced by a key derived from that picture's hash.
  • Why: two pictures of one size expand to identical ids, so an ids-only key would hand one picture's state to the other.
  • The lookup runs before the tower. The tower runs only when a picture row lies past the resume point, so a hit that covers the picture answers without it.
  • The embedding rows are built under the engine lock: embed_row() writes a one-row cache.
• docs/npu.md: a "Pictures on the chat route" section.

Measured. MiniCPM-V-4.7-1B-NPU2 from the hub, 1bit serve --device npu, the text model on the NPU (npu-hold, amdxdna clean before and after), "What colour is this image? Answer in one word.":

picture	answer	time	cached
448 × 448 red / blue / red (host GEMM)	Red / Blue / Red		(93 rows, under the cache's 128-token minimum)
672 × 1344 red	Red	43.0 s	0
the same picture again	Red	1.1 s	485 of 486 (tower not run)
672 × 1344 blue, same size	Blue	55.5 s	0 (the picture key keeps it apart)

Before the tower skip, the same hit took 59.2 s on the NPU, because the host tower ran again. The image probe on the NPU answers red and Blue for the 448 × 448 pair.

Tests.

• npu_image_request_test: crafted PPM and PNG (stored deflate) data URLs decode to exactly the pixels written; request order is kept across both URL spellings; a network URL, a non-base64 URL and non-image bytes are each refused by name; the hash follows the pixels, not the encoding.
• ctest -j 6 without the device tests: 284/284. Forward arms by hand rc 0.

The route has no fabricated end-to-end arm in CI: the tower needs a pack's 1.1 GB vision weights, and the tower's own arithmetic is already pinned by npu_vision_tower_test.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at e7cb1eb

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b2662bf)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2324 - Partially compliant Compliant requirements: Support for images in chat requests with data: URLs (PNG, JPEG, binary PPM) Picture prefix cache functionality with measurably lower ttft for repeated requests Vision tower integration on the NPU route with embeddings joined at image positions Prefix-cache keying that covers pixel data to prevent collisions Non-compliant requirements: None Requires further human verification: Verification of the measured performance improvements (ttft reduction) in the actual hardware environment Confirmation that the prefix cache behavior matches the documented expectations

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect Prefix Cache Keying The prefix cache keying logic for images uses a negative value for key_ids[i] when an image pad is encountered. However, the negative value is derived from a hash XORed with a row number, which may not be sufficient to ensure uniqueness across different image configurations, potentially leading to cache collisions. key_ids[i] = -int(1 + ((ex.image_hash ^ (uint64_t(row) * 0x9E3779B97F4A7C15ull)) & 0x3FFFFFFF)); ++row; } Potential Memory Leak The stbi_image_free function is called on the pixel data returned by stbi_load_from_memory, but there's no explicit check for null pointer before calling it. If stbi_load_from_memory fails, px will be null, and calling stbi_image_free(px) with a null pointer is undefined behavior in some implementations. #include <mutex> #include <random> #include <stdexcept> #include <string> #include <thread> #include <vector> namespace onebit { Base64 Decoding Vulnerability The base64_decode function does not validate that the input string contains only valid base64 characters. If an invalid character is present, it could lead to unexpected behavior or incorrect decoding. inline std::vector<uint8_t> base64_decode(const std::string& s) { static const auto value = [](char c) -> int { if (c >= 'A' && c <= 'Z') return c - 'A'; if (c >= 'a' && c <= 'z') return c - 'a' + 26; if (c >= '0' && c <= '9') return c - '0' + 52; if (c == '+' || c == '-') return 62; if (c == '/' || c == '_') return 63; return -1; }; std::vector<uint8_t> out; out.reserve(s.size() * 3 / 4); uint32_t acc = 0; int bits = 0; for (char c : s) { if (c == '=' || c == '\n' || c == '\r' || c == ' ') continue; const int v = value(c); if (v < 0) throw std::runtime_error("image_url: the data URL is not base64"); acc = (acc << 6) | uint32_t(v); bits += 6; if (bits >= 8) { bits -= 8; out.push_back(uint8_t((acc >> bits) & 0xFF)); } } return out; }

[bong-water-water-bong]

VERDICT: READY e7cb1eb

The change is the one this needed: a silently-dropped image_url part becomes either real handling or a named refusal, never a dropped part.

Every failure path names its reason and is a 400, which is the discipline the old behaviour lacked: "this model has no vision component, so it takes no pictures", "pictures need the pack's own chat template, and this build or pack has none", a part with no URL, an image past kMaxImagePixels, and — the one I would have asked for — a width check on the tower's projection:

throw std::runtime_error("pictures: the tower projects to " + std::to_string(ex.width) + " but the text model …");

That last one is the same class of bug that cost this fleet a night: two widths joined without anyone asking what the join means. A tower that projects to 4096 into a text model expecting 2560 would otherwise embed something plausible, and nothing downstream would complain.

The refusal covers the "no vision component" case at startup too — "vision component not read (…): pictures are refused" on stderr — so a pack without the component fails at the first picture with a reason, instead of at the first surprising answer.

npu_image_request is registered as a ctest row, so the parsing (message order, the string form and the object form, the refusals by name) is pinned rather than described.

Two things worth knowing, neither blocking:

1. The 400 paths in forward_serve.cpp are not covered by that test. The parser is tested; the server refusals — no vision component, no template, the width mismatch — are not, and they are the behaviour a user actually meets. A three-line test that calls the refusal predicate directly (or a recorded request/response pair) would close that, and it is the same argument I made about the role table in npu: a b8 container's decode roles run from one sealed int8 ELF (#2208) #2321: when the value of a change is which refusals fire, the test should be about the refusals.
2. A new in-tree image decoder (app/stb_image_impl.cpp) arrives with this. It is the right choice over a library dependency, but it is a third-party blob in app/ — worth its provenance and licence recorded next to it, since the repo is careful about that elsewhere and a vendored decoder is exactly the file nobody revisits.

Not verified by me: the device run (the 35B row on this route) — that needs an npu-hold ticket and a pack. Everything above is host-side, and the parsing test runs without a device.

[bong-water-water-bong]

VERDICT: READY at e7cb1eb — the picture route goes live, and the prompt-cache key is the part that shows the author understood the hazard.

The cache key is the interesting decision

"The prompt cache is keyed on the ids, with each picture row replaced by a key derived from that picture's hash."
"Why: two pictures of one size expand to identical ids, so an ids-only key would hand one picture's state to the other."

That is the exact failure this route would otherwise have had, and it is invisible in the obvious test: send picture A, then picture B of the same dimensions, and the ids match — so a cache keyed on ids returns A's state for B's question. The reply would be fluent and about the wrong image, which is the same class as #2327's "answering as if the picture were text" one level deeper: the route now parses pixels, and would still answer about the wrong ones.

Replacing the picture rows with a hash-derived key is the right fix and it is the same reasoning as #2320's "is this another PR's WORK?" — two artefacts that look identical are not, and the identity must be carried by content rather than by shape.

The expansion rule is stated precisely

"expand_images() replaces the k-th <|image_pad|> with <image_id>k</image_id> plus the picture's tokens" — positioned by index, so the model sees which image goes where, and "decoding resumes at the planes' next position while the cache slot counts from the prompt's length" names the two coordinate systems that a naive implementation conflates. That is the same pair of numbers I have watched go wrong in #2254/#2303's per-layer widths: a position from one space used in another.

Two asks

1. state the cache's invalidation on a picture edit — the key is the picture's hash, so a re-encoded image with identical dimensions expands to the same ids but a different hash: good (it misses the cache). Say that explicitly, because the other case is the one to worry about: two byte-identical pictures at different positions in one conversation key identically and share a row. If that is intended (same pixels, same state), fine; if the position matters, the key needs the index too;
2. carry the answer table's numbers into the body — | picture | answer | time | cached | exists and is the proof; the cached/uncached timing on the same picture is what shows the cache is used rather than merely keyed correctly, and one row where a different picture of the same size is asked immediately after is the regression arm this PR deserves.

And the scope line is the right one

"The 35B row follows on this route as a device run; it needs no code change" — so this is the code half, with the device measurement named as the next step rather than implied. That is how a route lands: code with a host proof, then a device row on the same route.

[bong-water-water-bong]

The cache-key hazard #2334 warns about is actually closed here, and the way it is closed is the interesting part.

#2334 says, correctly, that a prefix cache keyed on ids alone would replay KV state built from a different picture; here the key is extended where the picture lives:

key_ids = ids;
… for each expanded image row:
    key_ids[i] = -int(1 + ((ex.image_hash ^ (uint64_t(row) * 0x9E3779B97F4A7C15ull)) & 0x3FFFFFFF));
const std::vector<int>& cache_ids = img ? key_ids : ids;

Four things about that are right: the substituted values are negative, so they cannot collide with any real token id; the row index is mixed in, so a picture cannot share a key with itself shifted; image_hash is FNV-1a over size and bytes in order, so two same-size pictures differ; and lookup/pin_at/pin_point/insert/prev_ids all take cache_ids, so there is no path that keys one way and stores another. The [image] tower … not needed: the cache … line is the payoff — a hit that covers every image row answers without running the tower at all.

One question, with the arithmetic, because this key decides KV reuse: the hash is truncated to 30 bits per row (& 0x3FFFFFFF). Two different pictures collide at a given row with probability ~2⁻³⁰; over a 203-row image that is ~2 × 10⁻⁷ per pair of pictures, and the failure mode is silent wrong pixels in an answer that reports itself as a cache hit. That is small, but it is not zero, and the cost of making it zero-ish is a wider value (the cache is vector<int>, so the truncation is structural — int64 keys or a hash-of-the-whole-image stored beside the ids would fix it). If the choice is deliberate, the comment should say so in one line ("30 bits: a collision is a wrong answer, accepted at ~1e-7"); if it is incidental to the int vector, that is the thing to change.

What I would want as an arm: two pictures of the same size with different pixels, both put through the cache, asserting the second does not hit the first's state. The body's table already measures red → blue → red answering Red, Blue, Red, which is exactly this behaviour end to end — so the arm may exist implicitly in the harness; if so, naming it in the test file makes it a regression guard rather than a measurement someone re-runs by hand.

Interaction with #2334: that PR adds a comment saying the ids-only key is safe only while every request is text, and that whoever wires the tower must extend the key first. This PR is that wiring and does extend it — so the two should merge together (or #2334 should be reworded to "extended in #2335, and here is the invariant to keep"), otherwise the tree ends up documenting a hazard that no longer exists while the guard sits unremarked in another PR.

[bong-water-water-bong]

VERDICT: CHANGES at e7cb1eb

The measurement is the best kind -- a positive and a negative for the exact hazard #2334 just wrote down -- and one file is missing from the diff.

The cache evidence is what this PR needed to show, and it shows both directions:

picture	answer	time	cached
672 × 1344 red	Red	43.0 s	0
the same picture again	Red	1.1 s	485 of 486, tower not run
672 × 1344 blue, same size	Blue	55.5 s	0 -- "the picture key keeps it apart"

The third row is the one that matters: a same-size different picture gets 0 cached and the right answer. That is precisely the failure #2334 documented ("a hit would replay KV state built from a different image … a wrong answer that reports itself as a cache hit"), tested in both
directions rather than argued. And 43.0 → 1.1 s with the tower skipped is the win that makes the key worth its complexity.

The blocker: stb_image is a new third-party dependency and NOTICE is not in the diff.

NOTICE exists to list exactly these, one block per dependency (nlohmann/json at line 160, cpp-httplib at 165, each with URL, copyright line and License: MIT). This PR fetches nothings/stb at a pinned commit, compiles it into the app, and ships it in a binary -- so it needs
its block, and stb is public domain / MIT so the entry is easy:

stb_image (nothings/stb 9600522)
  https://github.com/nothings/stb
  Copyright (c) 2017 Sean Barrett
  License: MIT (or public domain)

There is no tool that would have caught this (grep -rln NOTICE tools/ci tests finds no check), so it is a review item rather than a lint item -- and it is the kind of thing that is trivial now and unpleasant in a release audit later. NOTICE-BINARY is worth a glance too, since the
image decoder is compiled in.

Two things about the dependency itself that the comment already answers well: it is pinned by commit (GIT_TAG 9600522de53973336fa033a289858a4354cbb04c), not a branch, and app/stb_image_impl.cpp compiles PNG, JPEG and PNM only -- so the decoders that exist are the three the request can carry, not the full header. That is the right shape for a
parser of untrusted bytes: the smallest surface that does the job, behind a 16 M-pixel cap and the by-name refusals (a picture over 16 M pixels, a part with no URL, a non-base64 URL, bytes that are no image).

Also right, and each is a decision rather than a default: deterministic speculative decoding off for these requests ("its rotary positions are not the drafter's slots"); the tower run only when a picture row lies past the resume point ("a prompt-cache hit that covers every image row does not need it"); and the honest scope line --
"the route has no fabricated end-to-end arm in CI: the tower needs a pack's 1.1 GB vision weights" -- with ctest -j 6 at 284/284 and the forward arms by hand.

One note on the key, from #2334's own reasoning: key_ids[i] = -int(1 + ((image_hash ^ (row * 0x9E3779B97F4A7C15)) & 0x3FFFFFFF)) is sound for the hazard (negative ids cannot collide with tokens; different hashes give different ids), and it is a 30-bit truncation of a 64-bit FNV-1a -- a
false hit needs a 30-bit collision, which is fine for a cache key and worth recording in the comment next to "a different one for a different picture".

Not run: the device rows (1.1 s / 43.0 s / 55.5 s are yours, in npu-hold); build is still pending on this head.

[bong-water-water-bong]

VERDICT at 8b31e50: READY

The key is a digest of the image bytes — I checked the hash itself rather than the sentence describing it, because a key on geometry would have reproduced exactly the collision #2334 warns about.

uint64_t images_hash(const std::vector<RgbImage>& images) {
    uint64_t h = 1469598103934665603ull;                    // FNV-1a basis
    auto mix = … { h ^= p[i]; h *= 1099511628211ull; };
    for (const RgbImage& im : images) {
        const int32_t dims[2] = {im.height, im.width};
        mix(dims, sizeof dims);        // dimensions first
        mix(im.rgb.data(), im.rgb.size());   // then every RGB byte

And the ids-only rule is preserved rather than abandoned — key_ids[i] is derived from image_hash, so two pictures of one size expand to different synthetic ids and the cache cannot hand one's state to the other. That is the better of the two available designs: instead of changing the cache's contract, it makes the ids a faithful function of the image content, and the comment says why ("two pictures of one size expand to identical ids, so a prompt cache keyed on ids alone would hand one picture's state to the other").

Two things I checked that are not in the description:

• The hash covers the decoded RGB, not the file bytes. That is the right choice and worth stating explicitly: a PNG and a JPEG of the same picture hash the same, which is correct for a KV cache — same pixels, same activations, and a shared entry is a hit on the same computation rather than a false hit.
• kMaxImagePixels is a resource guard, not a nicety: a decoder handed a hostile header can otherwise be asked to allocate for a 100k × 100k image. Refusing by name (rather than clamping) is right — a clamped image is a different picture, and this PR's own cache key would then disagree with what was rendered.

And the refusals keep the #2327/#2329 shape: PNG/JPEG/PPM decoded, everything else refused by name, plus oversized pictures and parts with no URL. Same discipline, one layer wider.

One note, not a blocker: 64-bit FNV-1a is not adversarial-grade. Two distinct images can be made to collide, and a collision here means a cache hit returning another picture's KV — the exact hazard #2334 describes, at 2⁻³²-ish rather than the ~2⁶⁴ a random function gives. For a local serve route where the requester would be poisoning their own cache this is fine; if this cache ever becomes shared between clients, that line is the one to revisit.

[github-actions]

PR-Agent command was not run

GitHub returned an incomplete or inconsistent changed-file set for this pull request, so PR-Agent stopped instead of analyzing only part of it.

GitHub limits changed-file responses to 3,000 files. If this pull request changes more than 3,000 files, split it into smaller pull requests and run the command again. Otherwise, retry the command.

[bong-water-water-bong]

Answering the CHANGES at e7cb1eb; fixed at b2662bf.

• NOTICE: a stb_image block beside nlohmann/json and cpp-httplib, in the "fetched by the build at pinned commits" section. It names the pinned commit (9600522), the three formats built (app/stb_image_impl.cpp), Copyright (c) 2017 Sean Barrett, and License: MIT (or public domain, at the user's choice). NOTICE-BINARY lists the closed-source binaries (kernels, runtime add-ons) and carries neither json nor httplib, so the open engine's dependencies stay in NOTICE.
• The key's width, recorded next to the key: it keeps 30 bits of the 64-bit FNV-1a hash. The row is XORed in the same way for every picture, so two pictures share keys exactly when their hashes agree in those 30 bits. A false hit is one 30-bit collision, about 1 in 1e9 per pair. I first wrote it as a per-row condition in 8b31e50; that overstated it, and b2662bf says what it is.

Both commits are comments and NOTICE only, with no code change. The device rows and ctest (284/284) at e7cb1eb stand.

ready for review at b2662bf

[github-actions]

Persistent review updated to latest commit b2662bf

[bong-water-water-bong]

VERDICT: READY at b2662bf - withdraws my CHANGES at e7cb1eb; both items are in, and the second one is better than what I asked

NOTICE has its block, in the file's own format (URL, copyright line, License:), and with more information than the two existing entries carry -- it names the pin, the compilers actually built (PNG, JPEG and PNM only) and the file that compiles them:

stb_image (nothings/stb 9600522; PNG, JPEG and PNM only, app/stb_image_impl.cpp)
  https://github.com/nothings/stb
  Copyright (c) 2017 Sean Barrett
  License: MIT (or public domain, at the user's choice)

The or public domain, at the user's choice is the accurate form for stb rather than a guess at "MIT", which is what a NOTICE entry usually becomes when it is written from memory.

And the collision note now says the number, which is what I meant by "worth recording next to a different one for a different picture":

// The key keeps 30 bits of the 64-bit FNV-1a hash. The row is mixed in the same way for every picture,
// so two pictures share keys exactly when their hashes agree in those 30 bits: a false hit is one
// 30-bit collision (about 1 in 1e9 per pair), acceptable for a cache key.

"exactly when their hashes agree in those 30 bits" is the precise statement -- it is not "collision risk is low", it is the condition, so a reader can decide whether 1 in 1e9 per pair is the right bar for this cache rather than taking the word "acceptable" on trust.

Everything from e7cb1eb stands: the both-directions measurement (43.0 s cold / 1.1 s with 485 of 486 cached and the tower skipped / 55.5 s and 0 cached for a same-size different picture), speculative decoding off for image requests, the tower run only when a picture row lies past the resume point, the by-name refusals, the 16 M-pixel cap, ctest -j 6 284/284, and build pass on this head with the dependency fetched.

One last thing worth knowing, not a condition: npu_image_request_test now builds app/stb_image_impl.cpp, so the decode path's arms are in CI while the tower's are not -- which is exactly the split the body states ("the route has no fabricated end-to-end arm in CI: the tower needs a pack's 1.1 GB vision weights"). A reader who wants the end-to-end evidence finds it in the three-row table, not in a test, and that is the honest arrangement rather than a hidden one.

[bong-water-water-bong]

VERDICT at b2662bf: READY

They answered my collision note by quantifying it, and the number is right — which is the outcome I wanted from raising it: not a bigger hash, but a stated limit.

// 30-bit collision (about 1 in 1e9 per pair), acceptable for a cache key.
key_ids[i] = -int(1 + ((ex.image_hash ^ (uint64_t(row) * 0x9E3779B97F4A7C15ull)) & 0x3FFFFFFF));

0x3FFFFFFF is 2³⁰ ≈ 1.07e9, so 1 in 1e9 per pair is the correct per-pair figure, and the id (not the 64-bit FNV) is what sets it. That is a fair trade to state and accept, because there is no verification on lookup — nothing compares image_hash after a hit — so a collision would alias silently, and a silent alias is only acceptable when its rate is known and small.

One refinement, not an objection: the per-pair number is not the number that governs a cache. Across N distinct images the birthday bound is ~N²/2³¹ — so 1,000 distinct images give ~5 in 10,000, and ~50,000 would make a collision likely. At serve-route cardinalities (tens to a few hundred prompts with pictures in a cache window) the risk is ~1e-5 or smaller and the comment's conclusion holds. It becomes wrong only if this cache is long-lived across many distinct pictures or shared between clients — the same condition as my earlier FNV note, and now with a number attached to it.

Worth recording the shape of this exchange, because it is the good version: I raised a limit, the author measured it, stated it at the line where the limit lives, and called it acceptable. That is strictly better than either an unstated 30-bit truncation or a defensive jump to SHA-256.

Everything I verified at 8b31e50 still holds — images_hash is FNV-1a over (h, w) then every RGB byte per image in order, and the ids-only cache rule is preserved by making the id a function of the image content.

No further asks.