coordinator: extend audio/video media support alongside images - #2659
coordinator: extend audio/video media support alongside images#2659dmitripikus wants to merge 16 commits into
Conversation
Each multimodal entry now carries the kind of media it holds (image, audio, or video). Also adds ModalityAudio and ModalityVideo constants next to ModalityImage. All entries created today are still images, and nothing reads the new field yet, so behavior is unchanged. This is the first step for later work that will let the coordinator handle audio and video too. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
The step now recognizes audio_url, video_url, and input_audio content parts alongside image_url. Audio and video URLs are downloaded through the same SSRF guard and size cap that images use, and inlined into the request as data URIs. Inline input_audio items get format-to-MIME validation and a size check on their base64 payload. Data URIs are checked against a per-modality MIME allowlist, so audio bytes in an image slot (and vice versa) are rejected. Audio and video are validated and inlined but do not enter MultimodalEntries yet. The encoder path (encode, render, decode) is unchanged and keeps working for image-only requests exactly as before. Adding audio and video to MultimodalEntries is a follow-up step that depends on the encoder wire contract being agreed with the render and encoder teams. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
The helpers that build and read multimodal feature maps used to hardcode the image key everywhere. This change reworks them so each entry's own modality decides which key it goes under, and the reader groups response data by modality too. Nothing external changes today: every entry is still an image and the maps still have only an image key, so encoder, render, and prefill see identical wire bodies. The purpose is to prepare the code so a later step that adds audio and video entries can drop in without another sweep through utils, render, and encode. Also: extractMultimodalEntries now walks every modality key present in the response instead of only image. A response with audio hashes and no matching placeholders is now rejected instead of silently ignored; one existing test was updated to match. On today's image-only data path, output is byte-identical. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Adds three new settings for the download size cap: one for images, one for audio, one for video. Each is optional. When set, it applies only to that kind of media, so an operator can keep image downloads small while allowing much larger audio and video without changing the image setting. The audio limit also applies to inline audio sent inside the request. The built-in list of allowed file types for each kind of media can now be changed through the config file. Three new settings accept a list of MIME types for images, audio, and video. If a setting is not given, the built-in list is used. Bad values are rejected at startup: a zero or negative size, a size too large to represent, a non-list value where a list is expected, or a non-string item in a list all cause the step to fail to load instead of quietly ignoring the setting. The example YAML file shows the new settings with sensible defaults, and the description of the step is updated to mention audio and video. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Audio and video now flow through the encoder the same way images do. Each media part (image_url, audio_url, video_url, or input_audio) becomes one entry in the multimodal list. For each entry, the encode step sends one sub-request to the encoder carrying the original content part in its native shape and features keyed by the entry's kind. The request-body walker in the encode step now returns parts grouped by kind. A small helper computes each entry's position within its own kind so the fanout can pick the right content part to send. The same helper is used by the decode step, so audio and video parts get their cache tag alongside images. For mixed-media requests (image plus audio plus video), entries stay in the order the walker saw them. For a single kind the new position lines up with the old flat index, so image-only tests still pass. The step description and per-kind download-size settings in the YAML now flag that a deployment whose encoder pod does not accept the audio and video part types will error on those requests, and that the audio and video caps should stay tight until the security team has reviewed the decoder path. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Some comments and config lines pointed to missing docs. Other comments talked about what the code used to do rather than what it does now. Rewrite them so a reader with no outside context can follow the intent. No behavior change. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
replace_media_urls silently ignores a media part that is missing its inner map, has a non-string url, or has empty input_audio data. Two later steps line up media parts with entries by counting position. Neither skipped the broken parts, so a broken one could take the slot that belonged to a good one — the wrong content in the encode request, or the wrong hash tag in decode. Use one shared check for "is this part usable" in all three places. Add tests that put a broken part next to a good one and verify the good one still gets the right entry. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
The check that rejects an oversized input_audio payload used a base64 length formula that was a few characters short of the real base64 length of a full-cap payload, so a payload exactly at the cap was rejected as too large. Use the standard padded-base64 length formula 4 * ceil(n/3), and add a test that a payload of exactly cap bytes is accepted. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
buildSingleMediaContent has a safety fallback for when the entry and part index do not line up. That fallback always returned an image_url shape, so if the mismatch happened on an audio or video entry, the encode sub-request carried audio hashes but image content — the encoder rejects it with a confusing image/audio mismatch error. Pick the fallback part type from the entry's modality (audio → audio_url, video → video_url, image → image_url) so the sub-request stays self-consistent and the encoder can reject it with a clear message. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
For audio_url and video_url downloads, check the origin's Content-Type against the per-modality allowlist. A server that returns text/html or another unexpected type is now rejected instead of being inlined into an audio_url or video_url slot as-is. Image downloads keep their existing permissive behavior so this does not change traffic that already works. Data URIs stay checked as before for all modalities. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
- The media step returned the same MIME sets the defaults use, so any
change to a returned set would leak back into the defaults. Copy the
sets when returning them.
- The render step used two different names ("images" and "mm_entries")
for the same count in its logs. Use "mm_entries" everywhere.
Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
The encode and decode steps found each entry's position within its media type by counting all earlier entries every time. Replace with a running counter that fills each position in one pass. Drop the helper that did the counting since nothing else uses it. Also add a small safety check in the media step: skip a result that was never filled in, so a future change that leaves one behind cannot crash the request. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
roytman
left a comment
There was a problem hiding this comment.
Solid design and strong config-validation/test coverage for the ingestion side (per-modality size caps, MIME allowlists, the audio-hashes-without-placeholders behavior change is directly tested).
Unicode symbols: 20 occurrences across new comments, mostly em dashes used as clause separators, plus two arrows:
decode.go:139,148 encode.go:244,278,279,284,285 render.go:299 replace_media_urls.go:284,424 replace_media_urls_test.go:1528,1676,1728,1746 utils.go:117,156 utils_test.go:388,655,698 coordinator.yaml:38,111,152
Suggest a straight-ASCII sweep (commas/periods/parens instead of em dashes, "->" or spelled out instead of →/↔) per project convention.
| if err := step.Execute(context.Background(), reqCtx); err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(reqCtx.MultimodalEntries) != 1 { |
There was a problem hiding this comment.
These comments say "MultimodalEntries stays empty," but the test checks that if len(reqCtx.MultimodalEntries) != 1 {
| // Section 5: all three media parts feed into MultimodalEntries. | ||
| // Entries appear in walker order: URL refs first (image, audio, video), | ||
| // then any inline refs (none here). | ||
| if len(reqCtx.MultimodalEntries) != 3 { |
There was a problem hiding this comment.
the comments say "The image feeds into
// MultimodalEntries; audio and video are inlined but stay out." here we see if len(reqCtx.MultimodalEntries) != 3 {
| // Today entries carry Modality=image and the response has only [image] | ||
| // keys, so this collapses to the single-key check that was here before. | ||
| totalHashes, totalPlaceholders, totalKwargs := 0, 0, 0 | ||
| for _, s := range renderResp.Features.MMHashes { |
There was a problem hiding this comment.
nit: for _, s := range renderResp.Features.MMHashes (and the two loops after it) shadow the method's s *RenderStep receiver, which is used both above (s.postRender) and below (s.checkPlaceholderLimit) in the same function. Not a bug since s isn't read inside these loops, but worth a different loop variable name to avoid confusion.
| hashIdx++ | ||
| localIdx := modCounter[modality] | ||
| modCounter[modality]++ | ||
| if h, ok := hashByModLocalIdx[modality][localIdx]; ok { |
There was a problem hiding this comment.
if h, ok := hashByModLocalIdx[modality][localIdx]; ok { partMap["uuid"] = h } silently leaves the part untagged on a miss: no log, no error. Given the PR's stated "fail loudly" philosophy elsewhere (render.go's per-modality mismatch errors), a decode-stage pairing miss here is exactly the kind of silent divergence that should at least log at logged.DEBUG or count a metric, since it'd otherwise be invisible in production.
|
encode.go:94 still reads "vision encoder" although audio now takes the same inline path, so maybe consider changing the comment and adding a TODO to re-measure the inline-versus-fanout tradeoff for video, where kwargs_data is far larger than for images. Also, the generate-path tests are still image-only (render_test.go only gains Modality: ModalityImage on two fixtures), so consider adding a TestRenderStep_GenerateFormat_MultiModality cloning render_test.go:481 with the audio/video features from utils_test.go |
Replace non-ASCII punctuation in comments and YAML across this branch's changes: em dash -> comma or period arrows -> -> and <-> ellipsis -> ... approx -> ~= Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
|
Regarding @roytman 's comment about Unicode characters, |
The media step split parts into two buckets (URL-based and input_audio) and appended entries in bucket order, not request order. Encode and decode walk the request in true order, so a request mixing audio_url and input_audio silently paired each audio entry with the wrong part. Collect all media parts into one walker-order slice tagged by kind and process them in that single order. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Replace "(matches prior behavior)" in the inline-ref skip comment with a description of what the code does, so the reader does not need to know the code's history. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Three docstrings in replace_media_urls_test.go claimed audio and video parts do not enter MultimodalEntries; the bodies check the opposite. Rewrite them to match. Also drop a "today" from a nearby docstring. Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Summary
Closes #2648.
Adds full audio and video support to the coordinator alongside images. Covers every part of the tracking issue: type foundation, request handling, pipeline plumbing, per-kind config, and encoder path.
What each part of the branch does:
Media kind on each item. Every downloaded media item now carries a label saying what kind it is (image, audio, or video). New names for audio and video sit next to the existing name for image so the rest of the coordinator can tell them apart.
Downloading audio and video. The step that downloads media links used to accept only image links and drop the rest. It now also accepts audio links, video links, and audio sent directly inside the request. Each kind has its own list of allowed file types and its own size limit.
Pipeline stages handle every kind. The steps that pass media between stages of the pipeline no longer assume everything is an image. They now handle audio and video too. Requests that only carry images behave exactly as before — every existing test still passes.
Per-kind operator settings. New settings let an operator set a different download size limit for each kind and a different list of allowed file types for each kind. Images can stay tight while audio and video are allowed to be much larger. Bad settings (zero, negative, or too large) are rejected at startup instead of being silently ignored.
Sending audio and video to the encoder. Audio and video now go through the encoder the same way images do. Each item in the request is paired with the matching content when the encoder is called.
Design decisions
The following decisions were made:
{type: "audio_url", audio_url: {...}},{type: "input_audio", input_audio: {data, format}}, etc.); features undermm_hashes[<modality>]. Encoder is expected to return the same shape with new modality keys.Features.MM*[<modality>]per-modality; mismatched responses fail loudly with clear per-modality error messages, never silently.MultimodalEntry.Indexis walker discovery order; per-modality local index computed on the fly. For image-only requests this collapses to today's flat indexing so every existing test passes unchanged.audio/wavonly) when warranted.max_concurrent_downloadsbounds peak memory the same way for all media. An operator who wants to accept 200 MB videos must raise the video cap explicitly, so the peak-memory implications are a conscious choice.Config changes
New YAML params in the
replace-media-urlsstep (all optional):max_image_download_size,max_audio_download_size,max_video_download_size— per-modality overrides of the globalmax_download_sizeallowed_image_content_types,allowed_audio_content_types,allowed_video_content_types— override the built-in MIME allowlist per modalityOut of scope
http(s)://URL (rather than an inlinedata:URI), the coordinator downloads the bytes and inlines them into the request body as a data URI using whateverContent-Typeheader the origin server reported. The per-modality file-type allowlist is only enforced ondata:URIs supplied directly by the client — it does NOT fire on theContent-Typereturned by the origin server on an HTTP download. In practice: a compromised or misconfigured origin can returnContent-Type: text/html(or anything else) under an image URL, and the coordinator will faithfully inline it asdata:text/html;base64,…inside theimage_urlslot. The model server usually rejects that shape, but the coordinator's own defense-in-depth check does not fire. This gap existed for image downloads before this PR and it remains for audio and video downloads after. Closing it uniformly requires reconciling one existing test that relies on the fallbackapplication/octet-streamContent-Type passing through, so it is deliberately kept out of scope here and tracked as a separate follow-up rather than mixed into this change.