feat(upload): accept MP4 so video-capable models can read a clip - #975
feat(upload): accept MP4 so video-capable models can read a clip#975octo-patch wants to merge 1 commit into
Conversation
Attachments are forwarded to the selected model as file parts, but the upload path accepted only JPEG, PNG and PDF, so a clip could not reach a model that reads video. - Detect MP4 from the ISO base media `ftyp` box, restricted to MP4 brands so QuickTime and M4A audio stay rejected rather than failing at the provider. - Add `video/mp4` to the declared-type gate and to the three client allowlists that guard the picker, the drop zone and the attachment menu. - Estimate a clip's tokens from a frame-sampled fixed cost instead of its byte length, which would otherwise price one clip above the whole attachment budget and evict every other file replayed to the model.
|
@octo-patch is attempting to deploy a commit to the morphic Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae1ac0d0a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'video/mp4': buffer => | ||
| buffer.subarray(4, 8).equals(Buffer.from('ftyp', 'ascii')) && | ||
| MP4_BRANDS.has(buffer.subarray(8, 12).toString('ascii')), |
There was a problem hiding this comment.
Valid MP4 files are not limited to these major brands: for example, CMAF clips can use cmfc, and an ISO-BMFF file can advertise MP4 through a compatible brand rather than its major brand. Because detection checks only bytes 8–12 against this small set, such files pass the browser's video/mp4 check but receive Unsupported file content from the upload route. Inspect the compatible-brand entries as well and accept the complete set of supported MP4 brands.
Useful? React with 👍 / 👎.
| // A clip is sampled into frames, so it costs a small multiple of a single | ||
| // image rather than what its bytes expand into. The upload size ceiling bounds | ||
| // how long a clip can be, which is what keeps a fixed estimate usable here. | ||
| export const VIDEO_ATTACHMENT_TOKENS = 5 * IMAGE_ATTACHMENT_TOKENS |
There was a problem hiding this comment.
For low-bitrate or highly compressible clips, the 5 MB upload ceiling does not meaningfully bound duration, while frame-sampled model input grows with duration. Returning 50,000 tokens for every video therefore lets capHistoricalAttachments and truncateMessages retain long clips whose actual input can greatly exceed their token budget or model context window, causing provider failures or unexpectedly costly requests. Derive the estimate from duration/frame sampling, or use a conservative bound when duration is unavailable.
Useful? React with 👍 / 👎.
|
Reviewed this with model routing in mind, and the finding changes the framing of the PR, so I would rather check the premise with you than hand you a fix list. Only one provider in this repo can carry a video part todayVerified against the versions in this tree. Each of these throws during prompt conversion, before any network call: Ollama is image-only upstream. The gateway provider does forward the part, but the AI Gateway models API still does not expose a video-input tag (vercel/ai#9417 is open), so capability cannot be read from metadata either. Worth noting for the "which model reads video" question: the open models that do read video (Qwen3-VL, GLM-4.6V) are not reachable through this stack either. vLLM serves them with a So the capability this PR makes reachable is reachable on What that means for the current shapeThe allowlist is global and nothing gates MP4 on the receiving model:
That is the shape of the incident #952 closed. Today the same clip is refused at upload with a 400 that names the problem. After this change it is accepted, stored, and then breaks the conversation with a generic error. QuestionGiven that video is reachable only on Google today: do you still want to land this, or should it be closed? If yes, the gate belongs on the receiving model's provider rather than on the upload boundary:
If no, closing this PR is all that is needed. An MP4 cannot be uploaded today, so nothing else has to change. Minor
The two P2s from codex above (compatible brands, duration-independent token estimate) I agree with. They apply in the "yes" branch. UI is not a blocker: video falls back to the same generic rendering PDF gets today, an extension chip in the composer, a link in the message, a generic icon in Library. Worth its own PR rather than this one. |
2 participants
Reason: Uploaded attachments are forwarded to the selected model, but the upload path accepts only JPEG, PNG and PDF, so a clip cannot reach a model that reads video.
Problem
Attachments become
fileparts and are handed to whichever model the chat is using, with no per-format branching on the way out. The formats that may enter, though, are fixed at the upload boundary: JPEG, PNG and PDF. A model that accepts video therefore cannot be given a clip — the file is refused before it is ever stored, so the capability is unreachable no matter which model is selected.MP4 was not deliberately excluded. It was simply absent from the allowlist, and the content check added in #952 rejected the ISO container as a consequence.
Changes
lib/storage/file-signature.tsdetects MP4 from the ISO base mediaftypbox: the box type at bytes 4..8, then the brand at bytes 8..12 checked against the MP4-compatible brands. The brand check is what keeps the shared container from over-matching — QuickTime (qt) and M4A audio ride in the same box, and accepting the box alone would move their failure from the upload to the provider request.video/mp4is ordered ahead of PDF because PDF is the one format matched by scanning a window rather than a fixed offset.video/mp4is added to the declared-type gate (shared with the signature module) and to the three client allowlists that guard the attachment menu, the standalone picker and the drag-and-drop zone. The 400 message that enumerates the readable formats is updated to match.lib/utils/attachment-tokens.tsestimates a clip from a frame-sampled fixed cost, the same reasoning already applied to images, instead of falling through to the byte-length branch. Byte length is the wrong model for video and not merely imprecise: a clip at the upload ceiling estimates over a million tokens, which exceeds the whole 200k attachment budget from feat(streaming): bound replayed attachments by weight, not only by count #951 and would evict every other attachment replayed to the model, and would trip context-window truncation on the turn that carries it. The estimate is deliberately above a single image's.The existing signature test asserted that the production ISO container is rejected; it now asserts the container is detected as video, which keeps the original regression covered — a clip named
image.jpgis still not sent to the model as a broken image. The route test's "unsupported content" fixture was that same container, so it is replaced with bytes that open no supported format.Scope
The 5MB upload ceiling is unchanged, so this admits short clips rather than arbitrary video. Raising that limit is a storage and cost decision, not part of making the format reachable, and it is left alone deliberately. Attachment previews already render a non-image as a labelled chip and a link, so a clip shows up as
MP4without new UI.Verification
bun run test— 542 passed, 3 skipped, 61 files. New coverage: MP4 detection, the production container now detected as video, a non-MP4 brand and a truncated brand both rejected, a clip stored under its detected type across the object write and the library row, and the fixed video token estimate.bun lint,bun typecheck,bun run format:check,bun run buildall pass.ftypboxes real encoders write (isomiso2avc1mp41,mp42mp41isomavc1,dashiso6avc1mp41), all detected asvideo/mp4, with a QuickTime and an M4A header rejected and JPEG/PNG detection unchanged.