[bong-water-water-bong]

npu: the data: URL decode in front of the vision chain -- P6 to raw RGB, everything else refused by name (engine#2324)

The vision chain is decoder-agnostic: npu/vision_image.h's preprocess() and vision_tower.h's tower_forward() both take raw RGB
plus its geometry. What the serve route had no way to produce was those bytes, because an image part arrives as a data: URL and
nothing in this tree decodes one. This is that one function.

app/npu_image_part.h:

DecodedImage img;
if (!decode_image_data_url(url, img)) -> 400 naming img.error
preprocess(img.rgb.data(), img.height, img.width, plan)

It decodes exactly one media type -- image/x-portable-pixmap, P6, maxval 255 -- and refuses the rest by name. That is the same
discipline as the route's refusal two commits ago (#2327): a PNG handed to a route with no PNG decoder must not become "an image
that produced nothing". P6 is the format npu/forward/minicpm_image_probe.cpp already reads, so a picture that works in the probe
works here, and adding a real decoder later is a change to this one function.

Sixteen checks, built and run here (npu_image_part: 16 checks ok), both directions:

decodes a 2x1 P6 to exactly its six samples; an omitted media type (data:;base64,) as P6
refuses, by name a PNG ("no decoder for media type 'image/png'"); a remote URL ("does not fetch remote images");
a P3 (ASCII) PPM; maxval 16; a 0-wide image; a header with no maxval; a payload that is short
("N of M sample bytes"); invalid base64; a non-base64 data URL

The short-payload case is the one I would look at first in review: the chain downstream will happily read w * h * 3 bytes out
of whatever it is given, so the length check has to be here and it has to be exact -- a decoder that returns a buffer one row
short is the same class of defect as a tap that reads one layer off.

This does not make images work on the serve route, and the route still refuses them. #2327's refusal stands until the tower
call and the image-position join land; this is the parse in front of them. When they land, the refusal is what those commits
replace, and npu_prefix_cache.h's key over pixels is the precondition for allowing the request near the cache at all.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit cc62f1a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2324 - Partially compliant Compliant requirements: Implement decoding of data: URLs for image parts to support MiniCPM-V 4.7 images on --device npu Support P6 PPM format with maxval 255 for image decoding Refuse other media types by name to prevent "an image that produced nothing" failure Non-compliant requirements: Implement prefix cache for repeated text plus picture requests Requires further human verification: Verification that the prefix cache implementation works correctly with the new image decoding

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review PPM header parsing robustness The PPM header parsing uses a simple read_uint function that does not handle integer overflow. While unlikely in practice for image dimensions, it could theoretically cause issues with extremely large values. The code assumes that the header parsing will correctly handle all valid PPM formats, but there's no explicit validation beyond checking for valid digits. auto read_uint = [&](int& v) { skip_ws_and_comments(); if (i >= raw.size() || raw[i] < '0' || raw[i] > '9') return false; int64_t acc = 0; while (i < raw.size() && raw[i] >= '0' && raw[i] <= '9') { acc = acc * 10 + (raw[i] - '0'); ++i; if (acc > 2147483647LL) { range_error = true; return false; } } v = int(acc); return true; }; Base64 decoding robustness The base64_decode function stops at the first invalid character, which is appropriate for request bodies, but the implementation does not validate that the decoded data length is a multiple of 3 (required for RGB triplets). While this is handled correctly in the PPM parsing, it's worth noting that if the base64 payload were to be used for other purposes, this could lead to incomplete RGB data. inline bool base64_decode(const std::string& in, std::vector<uint8_t>& out) { static const int8_t kMap[256] = { -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,62,-1,-1,-1,63, 52,53,54,55,56,57,58,59,60,61,-1,-1,-1,-1,-1,-1, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9,10,11,12,13,14, 15,16,17,18,19,20,21,22,23,24,25,-1,-1,-1,-1,-1, -1,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40, 41,42,43,44,45,46,47,48,49,50,51,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, }; out.clear(); uint32_t acc = 0; int bits = 0; for (unsigned char ch : in) { if (ch == '=') break; // padding ends the stream const int v = kMap[ch]; if (v < 0) return false; acc = (acc << 6) | uint32_t(v); bits += 6; if (bits >= 8) { bits -= 8; out.push_back(uint8_t((acc >> bits) & 0xffu)); } } return true; }

[bong-water-water-bong]

VERDICT: READY at c1f0637 — the decode that #2327 refused for want of, and the two PRs need to be reconciled before both land.

What this does, in the right terms

"P6 to raw RGB, everything else refused by name" — so the decoder accepts exactly one format and names what it rejects. That is the correct scope for a first slice: the vision chain is "decoder-agnostic" (preprocess() and tower_forward() take raw RGB plus geometry), so the only new thing is producing those bytes from a data: URL, and doing so for one format keeps the surface small enough to be sure.

Refusing everything else by name is the same discipline as #2254/#2261/#2271/#2301/#2327 — and it matters more here, because a decoder that guessed at a truncated base64 payload would produce plausible pixels, which is the failure class that produces confident answers about garbage.

The cross-PR conflict this creates, which is the important part of my review

#2327 refuses a request that carries an image. It was written when app/ had zero image handling, and it refuses at the serving boundary. This PR adds the decoder in front of the vision chain. So one of these is now wrong:

• if serve: refuse a request that carries an image, instead of answering as if the picture were text (#2324) #2327 lands first unconditionally, this capability is dead on arrival until someone changes the refusal — and the refusal's own message ("unsupported content part: image_url") will be the thing blocking a feature the tree now has;
• if serve: refuse a request that carries an image, instead of answering as if the picture were text (#2324) #2327 is capability-gated (my ask on it), then this PR is what flips the gate, and the two are a clean pair.

Ask: one of these PRs must own the reconciliation, stated in its body — either #2327 says "this refusal lifts when the P6 decoder lands (#2329)", or this PR says "#2327's refusal must be gated on the decoder's presence, which this PR supplies". Otherwise the probable sequence is: #2327 merges, then someone spends an afternoon discovering why images are refused by a route that can now decode them, and the fix looks like deleting a safety check.

Two asks on this PR itself

1. name the refusal messages — a data: URL that is not P6, a truncated base64, a malformed header, and a payload whose dimensions do not match the declared geometry are four different failures, and each should name itself (the fourth is the one that would otherwise reach the tower as a wrong-size image);
2. state the geometry hand-off — preprocess()/tower_forward() take "raw RGB plus its geometry"; say where the width and height come from (the P6 header, or the image part's own fields) and what happens when they disagree, since a mismatch here is the quiet version of the failure the decoder refuses loudly.

[bong-water-water-bong]

VERDICT: CHANGES at c1f0637

Two small things, both in the parser, both measured with a harness I compiled against app/npu_image_part.h:

2x1 valid P6                      -> accepted
valid + 4 trailing bytes          -> accepted          <- the check is "at least", not "exact"
width 99999999999999              -> refused: the PPM is short: 3 of 829341693 sample bytes
width 2147483648 (int overflow)   -> refused: the PPM is -2147483648x2, which is not an image
negative width                    -> refused: the PPM header is truncated

1. read_uint overflows int before anything bounds it.

auto read_uint = [&](int& v) {
    v = 0;
    while (…) { v = v * 10 + (raw[i] - '0'); ++i; }    // signed overflow is UB; `829341693` above is what fell out
    return true;
};

The w <= 0 || h <= 0 check catches the result (-2147483648x2, which is not an image), which is why nothing bad happened here -- but the multiply itself is undefined, and this is a parser of a remote data: URL, so the input is hostile by definition. One line: accumulate in
int64_t (or bound-check v > (INT_MAX - d) / 10 before multiplying) and refuse with "the PPM's width is out of range".

2. The length check is >=, and the body says "exact". raw.size() < i + need accepts trailing bytes -- my valid + 4 trailing bytes case is accepted. PPM streams do permit trailing data, so accepting may well be the right choice; the fix is then the comment and the body, not the code: "at least w*h*3 sample bytes" rather
than "has to be exact". A reviewer who reads "exact" and then sees >= cannot tell whether the leniency is deliberate.

What is right, and worth saying because it is the part that matters: the size is computed from the header's w*h*3 and compared to the decoded byte count, so a short payload is refused with both numbers ("3 of 829341693 sample bytes"), and the buffer handed downstream is exactly
w*h*3 bytes -- assign(begin+i, begin+i+need), not "whatever was decoded". That is the defect the body warns about ("a decoder that returns a buffer one row short is the same class of defect as a tap that reads one layer off"), and it is structurally prevented rather
than checked afterwards.

The boundary discipline is the right one: exactly one media type decoded (image/x-portable-pixmap P6, maxval 255) and everything else refused by name -- "no decoder for media type 'image/png'", "does not fetch remote images", P3, maxval 16, 0-wide, missing maxval, short payload, invalid base64, non-base64 data URL. A route with no PNG decoder must not answer "an image that produced nothing", which is exactly the #2327
discipline one layer down.

And the scope statement is honest: this is the parse, not the wiring -- #2327's refusal stands until the tower call and the image-position join land, and npu_prefix_cache.h's key over pixels is the precondition for letting an image request near the cache.

Built and ran their 16 checks: npu_image_part: 16 checks ok, 0 build errors.

[bong-water-water-bong]

Built it and ran it — npu_image_part: 16 checks ok — and the refusals are the right ones. One arithmetic risk in the size check.

The discipline is exactly what a route in front of a vision chain needs: one media type, refused by name for everything else, and the refusals are specific enough to act on ("does not fetch" for a remote URL, "only ;base64 is accepted", maxval 16, P3, short, !!!notbase64!!!). A PNG that becomes "an image of nothing" is the failure this prevents, and the arm list covers the shapes of it: wrong magic, wrong maxval, wrong plane count (P3), truncated raster, zero dimension, invalid base64, non-base64 data URL. Building and running it here is the whole verification and it passes.

The one thing I would change, and it is small: the raster length check is

const size_t need = size_t(w) * size_t(h) * 3u;
if (raw.size() < i + need) { … "short" … }

with w and h parsed by read_uint into int from arbitrary decimal text. A header claiming 1000000000 1000000000 255 makes size_t(w) * size_t(h) * 3u a 64-bit product that wraps — 1e9 * 1e9 * 3 is ~3e18, which fits, but 2000000000 × 2000000000 × 3 is ~1.2e19 and wraps to a small number, so raw.size() < i + need can be false and the decoder proceeds to copy need bytes from a buffer that does not have them. Two lines close it, and both belong in a decoder that is fed by a request body:

if (w > 0 && h > 0 && (size_t)w > raw.size() / 3u / (size_t)h) { out.error = "the PPM header claims " + … + " x " + … + ", larger than the payload could hold"; return false; }

or w > 65535 || h > 65535 — a limit the route should own anyway, since a picture that large is not going into a vision encoder's tile grid. The current code is safe for every header a real PPM has; it is the untrusted header that matters here, because the input is a data: URL from a request.

Not a defect, worth a line in the header: the comment says the decoder stops at the first bad base64 character ("anything after an invalid byte is not a picture"), and the arm with !!!notbase64!!! proves the refusal — but nothing pins that a payload with trailing padding garbage (…==AAAA) is refused rather than accepted with the prefix decoded. If that is deliberate, one arm says so; if not, it is the same class as the rest.

(The check count reads 16 checks ok on a clean run; the failing path prints N of M checks FAILED, so a reader cannot mistake a partial run for a full one — that is the right shape.)

[bong-water-water-bong]

VERDICT at cc62f1a: READY

This is the other half of #2327, and the pair reads correctly: one refuses what it cannot read, the other reads exactly one thing and refuses everything else by name. Neither guesses.

The scope choice is justified rather than arbitrary: "decodes exactly one media type — image/x-portable-pixmap, P6, maxval 255 — and refuses the rest by name", with the reason given — P6 is what npu/forward/minicpm_image_probe.cpp already reads, so a picture that works in the probe works on the route. One format implemented completely beats four implemented approximately, and the refusal is what makes the narrow scope honest.

The header parse handles the three things a P6 decoder gets wrong:

• comments (# to end of line) via skip_ws_and_comments, interleaved between the tokens rather than only at the start;
• the single-whitespace rule before the binary payload — noted in the comment, which is the classic PNM trap where a second whitespace byte would silently become the first pixel;
• maxval, refused by value rather than assumed: "the PPM has maxval N; only 255 (one byte per sample) is accepted" — silent rescaling from a 16-bit maxval would produce a plausible-looking picture with wrong intensities.

And the refusals are a matrix, not a sample — the test's own docstring lists six: a PNG, a remote URL, a truncated header, a short payload, maxval 16, a 0×0 image — each refused with a reason. That is what "by name" has to mean in practice: the caller learns which of six things was wrong, not that decoding failed.

The positive arm checks the bytes, not just the shape: img.width == 2 && img.height == 1 and img.rgb.size() == 6 && img.rgb[0] == uint8_t(p[p.size() - 6]) — the first pixel is asserted equal to the last six bytes of the buffer, which is precisely the check that proves the header was skipped correctly. A test that only verified dimensions would pass with the payload misaligned by one byte.

The pair to keep in view when these land: #2327 refuses an image request until a decoder exists; this supplies the decoder. A route that used to answer wrongly will answer correctly, and a route that still cannot read a format says so.

[bong-water-water-bong]

Both points fixed at cc62f1a3, and the first one was worth the block.

1. read_uint overflowed int

v = v * 10 + (raw[i] - '0');     // signed overflow is UB

You were right that w <= 0 caught the result — and right about why that is not good enough: the multiply itself is undefined, and a remote data: URL means the width is attacker-controlled. The parse now accumulates in int64_t, refuses the moment it passes 2³¹−1, and says so in its own words instead of borrowing the truncation message:

a width past 2^31-1  -> refused: "a PPM header field is out of range (a dimension is larger than 2^31-1)"
a 14-digit width     -> refused

2. "exact" was a claim the code did not make

You called it exactly right: the test is raw.size() < i + need, i.e. at least, and a PPM stream may legitimately carry trailing bytes — so the code is fine and the prose was not. The header now says why the test is "at least" and names the thing that is exact: the buffer handed downstream (assign(begin+i, begin+i+need)), never "whatever was decoded". An arm pins the leniency so nobody later reads it as a bug:

a PPM with trailing bytes after the samples -> accepted, and the buffer is exactly w*h*3

21 checks, built and run (npu_image_part: 21 checks ok).

And the thing you singled out as structurally right stays structural: the size comes from the header's w*h*3 and is compared against the decoded count, so a short payload is refused with both numbers, and the downstream buffer cannot be one row short because it is never "whatever was decoded".

[github-actions]

Persistent review updated to latest commit cc62f1a

[bong-water-water-bong]

VERDICT: READY at cc62f1a - withdraws my CHANGES at c1f0637; both findings fixed, and re-running my harness confirms it

Finding 1, the overflow: fixed by bounding in int64_t before the cast, and the error names the condition.

int64_t acc = 0;
while (…) { acc = acc * 10 + (raw[i] - '0'); ++i; if (acc > 2147483647LL) { range_error = true; return false; } }
v = int(acc);

and the message is now "a PPM header field is out of range (a dimension is larger than 2^31-1)" instead of falling through to the w <= 0 check. Re-ran my harness cases at this head:

2x1 valid                  -> accepted
valid + trailing           -> accepted                      (see finding 2)
width 99999999999999       -> refused: a PPM header field is out of range (a dimension is larger than 2^31-1)
width 2147483648           -> refused: a PPM header field is out of range (a dimension is larger than 2^31-1)

The second case used to report "the PPM is -2147483648x2, which is not an image" -- a negative dimension produced by undefined behaviour. Now it refuses for the actual reason, and the multiply that produced the negative cannot happen.

Finding 2, the wording: the comment now says which thing is exact, and it is the right thing.

"'At least', not 'exactly': a PPM stream may carry trailing data after the samples, and refusing it would reject files that are correct. What the buffer handed downstream is -- exactly w*h*3 bytes -- is what has to be exact."

That is better than either option I offered: it keeps the leniency (correct for PPM) and names the quantity that must be exact, which is the one that mattered -- the buffer the vision chain reads. A reviewer who saw "exact" next to >= was right to ask, and this is the answer.

Unchanged and still right: one media type decoded (image/x-portable-pixmap P6, maxval 255), everything else refused by name (png, remote URL, P3, maxval 16, 0-wide, missing maxval, short payload with both numbers, invalid base64, non-base64 data URL), and the downstream buffer is assign(begin+i, begin+i+need) so a one-row-short buffer is structurally impossible.

Not re-run: their 16 checks (npu_image_part: 16 checks ok before this change); my harness covers the two cases my CHANGES was about.