Repository navigation
fix(cli): cloud export re-encodes media whose bytes differ from its extension - #7033
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head f5253694
The 'fails without the fix:' line is present, and the .mp3/.wav/image cases are right. One case the PR says it handles, audio-only MP4 named .mp4, still fails the export ↓ inline.
Scope
Read all 3 files plus mediaKind, choosePreset, probeVideo, ffmpegReencode and segmentArgs on the head. No prior reviews. CI was still running at review time (CI, CodeQL, Comments, preview-regression and Windows pending; regression, Player perf and captures green).
— Genesis (Lane B)
| case ".mp4": | ||
| case ".m4v": | ||
| // An audio-only M4A/M4B brand is not a video the API stores as video/mp4. | ||
| return at(4, "ftyp") && !at(8, "M4A ") && !at(8, "M4B ") && !at(8, "M4P "); |
There was a problem hiding this comment.
blocker: An M4A-brand file named .mp4/.m4v now goes to the re-encode path, but its kind still comes from the extension. mediaKind returns "video", choosePreset returns "video", and segmentArgs maps -map 0:V:0. With no video stream, ffmpeg exits 1 with Stream map '0:V:0' matches no streams (reproduced locally on an AAC-only mp4 with the preset's arguments). So the export still fails, now in ffmpeg instead of at /complete, for a case the PR body says it fixes.
The most common audio-only MP4 is also not caught: ffmpeg and most tools write the isom brand, not M4A , so it still matches here and is uploaded as video/mp4 as before.
Fix: when an ftyp file under a video extension has no video stream (probe it), or carries an M4A/M4B/M4P brand, re-encode it with the audio preset. Add a test that runs the .mp4-named M4A through offloadHeavyMedia with the reencoder and checks it chooses audio.
There was a problem hiding this comment.
Fixed at c094d43. A video-named file is now asked whether it holds a video stream (ffprobe, through the re-encoder); none means it is treated as audio and re-encoded with the audio preset, whatever its ftyp brand. The new test runs an audio-only .mp4 through offloadHeavyMedia and checks the re-encoder is asked for audio; it fails on the previous head.
Fallow audit reportQuality gate failed Found 2 findings. Health (2)
Generated by fallow.
|
There was a problem hiding this comment.
Verdict: REQUEST CHANGES: 1 resolved, 0 open, 1 new · f525369 → 5588fa2
- New blocker (CI): required check
Test (cli 2/2)fails atpackages/cli/src/commands/cloud/render.test.ts:156("plans the media as HeyGen asset uploads…"). Itsclip.mp4fixture isBuffer.alloc(6 MiB, 1)with noftypbox, so the new sniff re-encodes it and--dry-runexits 1. Fix: give the fixture an honestftypheader, asoffload.test.tsnow does. - The optional
Fallow auditcheck is also red because of this diff:planFileCRAP score is 79.4 (limit 30) andbytesMatchExtensioncyclomatic complexity is 21 (limit 20). It doesn't block merge. - Your update says the live upload check passed, but the PR body still has "Not run against the live asset API" unchecked. Update the body to match.
What
Cloud export now looks at a media file's first bytes before uploading it as-is. A file whose bytes are not the container its extension claims (an AAC
.m4arenamed.mp3, an audio-only MP4 named.mp4, a PNG named.jpg, and so on) goes through the existing re-encode path instead of being sent with the extension's content type.Why
The asset API checks the stored bytes, not the name. A 6.8 MB AAC file named
song.mp3was uploaded asaudio/mpeg, the upload and the PUT both succeeded, and/completeanswered 400Stored file type not supported: audio/x-m4a. That failed the whole cloud export. Several people hit it the same day.Related work
None open on this path.
How
planFileinpackages/cli/src/cloud/offload.tsis the one place that decides "as-is or re-encode". It now also requiresbytesMatchExtension(new,offloadSniff.ts) to agree: ID3 or an MPEG frame sync for.mp3,RIFF/WAVEfor.wav,RIFF/WEBP, the PNG, JPEG and EBML signatures, and anftypbox for.mp4/.m4v(not an M4A/M4B/M4P brand) or.mov. Anything that does not match is re-encoded by the preset the file's kind already takes; a file the sniffer does not recognise is re-encoded rather than trusted, which costs size, not correctness.Test plan
Fails without the fix: the new test "re-encodes an AAC file named .mp3 instead of uploading it as audio/mpeg" fails on main's
offload.ts(1 failed, 28 passed) and passes with the change. A unit test pins the sniff table.src/cloudandapi.test.ts, 12 files, 152 tests passed; oxfmt clean