Skip to content

server : accept data: URLs for input_video and input_audio - #27735

Merged
ngxson merged 3 commits into
ggml-org:masterfrom
geckguy:fix/27724-media-data-url
Sep 2, 2026
Merged

server : accept data: URLs for input_video and input_audio#27735
ngxson merged 3 commits into
ggml-org:masterfrom
geckguy:fix/27724-media-data-url

Conversation

@geckguy

@geckguy geckguy commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Overview

input_video and input_audio content parts were previously not handled as data: URLs in handle_media(), so a standard data: URL like data:video/mp4;base64,... was decoded directly as raw base64. That decode produced garbage bytes, the media probe failed afterwards, and the request died with a misleading "Failed to load image or audio file" 400 error (#27724).

This PR extends data: URL parsing in handle_media() to accept data:image/, data:video/, and data:audio/ uniformly across image_url, input_audio, and input_video, and removes the now-redundant accept_base64_uri flag. Malformed data URLs and unsupported MIME types throw std::invalid_argument so they return 400 invalid_request_error instead of 500, matching the rest of the file.

Additional information

Tested locally on an Apple Silicon Metal build with ggml-org/SmolVLM2-500M-Video-Instruct-GGUF and a short MP4:

Kept the MIME check to an explicit image/video/audio allowlist rather than accepting arbitrary data:* prefixes.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES - used an AI assistant to help draft the patch. I reproduced the bug, ran the tests above on my own machine, reviewed every line of the diff, and take responsibility for the change.

input_video and input_audio passed accept_base64_uri=false to
handle_media(), so data: URLs got treated as raw base64 strings and
failed later with a confusing media probe error (ggml-org#27724).

pass true for these two content types the same way image_url already
does, and allow video/audio mime types in the data: url check instead
of image only. data URL validation now throws std::invalid_argument so
malformed input comes back as 400 instead of 500, matching the other
input validation in this file.
@ggml-gh-bot

ggml-gh-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

Hi @geckguy, thanks for your contribution!

Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:

  • PR Template not respected: Please respect the template when creating a new pull request. Make sure to fill out all required sections.

Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below.

@ggml-gh-bot ggml-gh-bot Bot added the draft PR will be changed to draft by github-actions bot label Aug 26, 2026
@github-actions
github-actions Bot marked this pull request as draft August 26, 2026 11:53
@github-actions github-actions Bot removed the draft PR will be changed to draft by github-actions bot label Aug 26, 2026
@geckguy geckguy closed this Aug 26, 2026
@geckguy geckguy reopened this Aug 26, 2026
@geckguy
geckguy marked this pull request as ready for review August 26, 2026 12:08
@geckguy

geckguy commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Hi @ngxson !

The gpu-vulkan-apple job failed on the Test step (build/clone passed), but this appears to be unrelated to my changes.

Could you please re-run the failed gpu-vulkan-apple job when you get a chance? I believe it's a transient/flaky failure

Thanks

@Flo5k5

Flo5k5 commented Aug 27, 2026

Copy link
Copy Markdown

Thanks for the PR! Confirming this resolves the exact repro from #27724 on our side (2× V100, Qwen3.8-27B + mmproj): the data:video/mp4;base64,... form was falling into the raw-base64 branch and producing garbage bytes that made the ffprobe probe fail silently. The dual approach (parity for audio/video + the actionable error for unhandled data: URLs) is exactly right — the fail-fast branch in particular would have saved us a confusing debugging session. Happy to run an end-to-end test on our setup if useful before merge.

@ngxson

ngxson commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Aug 27, 2026

Copy link
Copy Markdown
Automated code review

Reviewed the diff in tools/server/server-common.cpp plus the surrounding handle_media logic, the exception->HTTP-status mapping in tools/server/server.cpp, and the three call sites.

Blocking

None. The fix is correct and minimal: passing accept_base64_uri=true for input_audio/input_video mirrors image_url, and the std::invalid_argument switches are confirmed by tools/server/server.cpp:60-63 (invalid_argument -> 400, other -> 500). The url.substr(0, url.find(',')) is safe when no comma exists (find returns npos, substr(0, npos) yields the whole string).

Will slow the review

(point 1) The new else if (string_starts_with(url, "data:")) branch at server-common.cpp:1121 is currently unreachable. All three call sites (image_url, input_audio, input_video) now pass accept_base64_uri=true, so the first branch (accept_base64_uri && starts_with("data:")) always wins for any data: URL. Maintainers are likely to ask why dead defensive code was added. Either justify it as a guard for future callers, or drop it. Relatedly, since every caller now passes true, the accept_base64_uri parameter itself is vestigial - consider removing it and collapsing the two data: branches into one, which is simpler than the dual-branch structure this PR adds.

(point 2) The MIME allowlist is now shared across all three input kinds. image_url previously rejected data:video/... and data:audio/... and now accepts and decodes them through the image path. That is a real (if minor) behavior broadening for image_url. If the intent is parity, fine, but it is worth a sentence in the PR description; if the intent is per-kind validation, the MIME check should be gated on the call kind rather than a single global allowlist.

(point 3) No test covers the regression. tests/unit/test_vision_api.py only exercises data:image/png. A small test sending a data:video/... (and the data:text/html 400 case) would lock in the fix; the bug (#27724) is exactly the kind that a one-line test would have caught. Not mandatory, but low-effort here.

Nits

(point 4) The comment at server-common.cpp:1075 still says // try to decode base64 image now that the branch also handles video/audio. Minor wording update.

(point 5) Pre-existing, not introduced by this PR: data: URLs (attacker-controlled) are base64-decoded with no size cap, unlike the 10MB limit on remote downloads a few lines above. Worth a follow-up, but out of scope here.

This review was generated automatically by pi coding agent using zai-org/GLM-5.2. It may contain mistakes. Maintainers make the final call.

Comment thread tools/server/server-common.cpp Outdated
out_files.push_back(decoded_data);
}

} else if (string_starts_with(url, "data:")) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please confirm if bot's review is correct: this branch seems to be unreachable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the bot is right. that branch only runs if accept_base64_uri is false and the string starts with data:. after this PR all three callers pass true, so it never hits.

i added it as a safety net from the original issue, but it's redundant now. the data: parser above already rejects unknown mime types with the same 400.

happy to drop it.

Comment thread tools/server/server-common.cpp Outdated
std::string url = json_value(input_video, "data",
json_value(input_video, "url", std::string()));
handle_media(out_files, url, opt.media_path, false);
handle_media(out_files, url, opt.media_path, true);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all call sites now use accept_base64_uri=true, so =false case is dead code now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. image_url already passed true, and this PR flipped audio/video to true as well, so nothing currently calls handle_media with false.

I originally left the parameter in place to keep the diff minimal, but since the false case is dead code now, I'll push an update to drop the parameter and remove the extra branch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped both. handle_media no longer takes accept_base64_uri, and the extra data: branch is gone. all three call sites just call handle_media(out_files, url, opt.media_path).

@geckguy

geckguy commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Could somebody review this?

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Traced the decode path and the diagnosis is exact: base64_decode stops at the first non-base64 character, so data:video/mp4;base64,... silently yields three bytes of garbage instead of failing, and the error only surfaces later at the mtmd probe. Routing everything through the single data: branch is the right call, and the compat endpoints get it for free since they normalize into image_url.

Nit: the comment above that branch still says "try to decode base64 image" while it now handles video and audio too.

Nit: one line in the existing parametrize of test_vision_api.py locks this in without needing a video model:
("What is this:\n", "data:text/html;base64,aGVsbG8=", False, None),

@geckguy

geckguy commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Updated @ServeurpersoCom . Thanks for the review

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

I've started the CI and am testing it at home.

@ngxson
ngxson merged commit 9cffdcc into ggml-org:master Sep 2, 2026
27 of 31 checks passed
Lawlietr pushed a commit to Lawlietr/llama.cpp that referenced this pull request Sep 3, 2026
…27735)

* server : accept data: URLs for input_video and input_audio

input_video and input_audio passed accept_base64_uri=false to
handle_media(), so data: URLs got treated as raw base64 strings and
failed later with a confusing media probe error (ggml-org#27724).

pass true for these two content types the same way image_url already
does, and allow video/audio mime types in the data: url check instead
of image only. data URL validation now throws std::invalid_argument so
malformed input comes back as 400 instead of 500, matching the other
input validation in this file.

* server : simplify handle_media and drop unused accept_base64_uri flag

* server : update comment and add unit test for invalid data URI MIME
fewtarius pushed a commit to fewtarius/CachyLLama that referenced this pull request Sep 5, 2026
…27735)

* server : accept data: URLs for input_video and input_audio

input_video and input_audio passed accept_base64_uri=false to
handle_media(), so data: URLs got treated as raw base64 strings and
failed later with a confusing media probe error (ggml-org#27724).

pass true for these two content types the same way image_url already
does, and allow video/audio mime types in the data: url check instead
of image only. data URL validation now throws std::invalid_argument so
malformed input comes back as 400 instead of 500, matching the other
input validation in this file.

* server : simplify handle_media and drop unused accept_base64_uri flag

* server : update comment and add unit test for invalid data URI MIME
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants