fix(relay): stop routing audio containers through the video validator - #5950
fix(relay): stop routing audio containers through the video validator#5950Chessing234 wants to merge 4 commits into
Conversation
`looks_like_iso_bmff` answers "is this an `ftyp` container", which is true of an MP4 video and of an M4A voice memo alike. Nothing could tell the two apart: `looks_like_mp4_iso_bmff` consults the compatible-brand list, and an Apple Voice Memo carries `isom` and `mp42` there, so it reads as MP4. Add `looks_like_audio_iso_bmff`, which reads the *major* brand — the one field that actually declares the container's content — against the Apple and Flash audio brands. Pure predicate, no caller yet. Signed-off-by: Taksh <takshkothari09@gmail.com>
`should_stream_as_video` sent every ISO-BMFF container down the streaming
video path. An M4A voice memo is ISO-BMFF, so an Apple Voice Memo reached
`validate_video_file`, where a normal (non-fast-start) recording fails
`check_moov_before_mdat` with
422 moov atom not at front of file (not fast-start)
That message describes an internal layout detail the user cannot act on, and
it is not even the real obstacle: the same validator goes on to require a
video track, which an audio-only file will never have. Rewriting the file to
put moov first would move the failure, not fix it (block#5752).
Route audio-branded containers to the generic path instead, which already
refuses audio explicitly and answers
415 disallowed content type: audio/m4a
That is what "Buzz has no audio pipeline yet" honestly looks like, and 415 is
the right status for it. Video routing is unchanged, including the
proprietary-brand case the existing test pins.
Signed-off-by: Taksh <takshkothari09@gmail.com>
themiguelamador
left a comment
There was a problem hiding this comment.
The major-brand shortcut is not a sound proxy for track contents. The MP4 Registration Authority defines M4A as an iTunes MPEG-4 audio brand that can contain audio, video, 3G text, and chapter tracks; M4B can contain those tracks too. As written, any valid video-bearing file with either major brand skips video validation and is rejected by the generic path as audio. That regresses an accepted video based only on its compatibility declaration.
I pushed a complete fix in 52cbcd293:
- keep all ISO-BMFF uploads on the bounded streaming path;
- classify the completed container from parsed track types before applying video-only fast-start and metadata requirements;
- return
415 audio/mp4for an actual audio-only,mdat-first Voice Memo; - preserve acceptance and validation of an
M4A-branded file that actually contains video; - remove the unregistered/over-broad audio-brand list and its public helper.
Verification:
cargo fmt --check— passcargo test -p buzz-media --lib— 119 passed- exact relay tests for M4A and proprietary-brand routing — pass
cargo clippy -p buzz-media -p buzz-relay --lib -- -D warnings— pass- broader relay media module — 28 infrastructure-free tests passed; 6 existing DB-backed tests could not run because the local Postgres instance has no
buzzrole
Authoritative brand registry: https://mp4ra.org/registered-types/brands
…t-start
Review is right that a major-brand shortcut is not a sound proxy for track
contents. MP4RA registers `M4A ` as an iTunes MPEG-4 audio brand that may
still carry audio, video, 3G text and chapter tracks, and `M4B ` likewise. A
brand is a compatibility declaration, not an inventory.
The validator already knew how to answer this correctly — the track loop
returns `DisallowedContentType("audio/mp4")` (a 415) when it finds audio and
no video. What went wrong is ordering: `check_moov_before_mdat` ran as the
very first statement, so a Voice Memo, which is never written fast-start,
failed there with "moov atom not at front of file" and never reached the
classification. That is a 422 about a requirement which does not apply to a
file with no video in it.
Classification now runs first, from the parsed track types, and the two
video-only requirements — fast-start and metadata-free — run after it. The
mp4 crate parses the whole file regardless of atom order, so an mdat-first
container reaches the track scan fine; the size guard still bounds the parse.
An `M4A `-branded file that really does contain video is unaffected: it
classifies as video, is validated as one, and still has to be fast-start.
Verified the regression fails on the old ordering: reinstating the leading
`check_moov_before_mdat` turns the new test's result into `Err(MoovNotAtFront)`.
- `cargo test -p buzz-media --lib` — 121 passed
- `cargo fmt --all -- --check`
Signed-off-by: Taksh <takshkothari09@gmail.com>
Review: routing audio-branded containers away from the streaming path rejects a genuine video on its compatibility declaration alone. Any valid video-bearing file with an `M4A `/`M4B ` major brand would have skipped video validation and then been refused by the generic path as audio — a regression against files that are accepted today. `should_stream_as_video` goes back to admitting every ISO-BMFF container. With the previous commit the validator classifies from parsed tracks, so an audio-only container still gets the honest `415 audio/mp4` and a video-bearing one is still validated as video. The routing layer no longer needs to guess. The brand list itself is gone along with its public helper: several of its entries (`M4P `, `M4R `, `F4A `, `F4B `, `mp4a`) are not registered ISO-BMFF brands at all, and the registered ones do not mean what the list claimed. - `cargo test -p buzz-media --lib` — 121 passed - relay media module — 29 passed; the same 6 DB-backed tests cannot run here, the local Postgres has no `buzz` role (pre-existing, unrelated to this diff) - `cargo clippy -p buzz-media -p buzz-relay --lib -- -D warnings` - `cargo fmt --all -- --check` Signed-off-by: Taksh <takshkothari09@gmail.com>
|
You're right and the brand shortcut is gone. The useful thing I found reimplementing it: the validator already answered this correctly. The track loop returns So the fix is the one you described, and it needed no new classification logic:
An I checked the regression actually regresses: reinstating the leading Verification:
|
Addresses the first "minimum safe behavior" bullet of #5752 — the misleading error. It does not add audio support.
should_stream_as_videorouted every ISO-BMFF container into the streaming video path. An M4A voice memo is ISO-BMFF, so an Apple Voice Memo reachedvalidate_video_fileand a normal (non-fast-start) recording died incheck_moov_before_mdatwith:That names an internal layout detail the user can do nothing about, and it isn't even the real obstacle — the same validator goes on to require a video track, which an audio-only file will never have. Rewriting the container to put
moovfirst would move the failure, not fix it.Audio-branded containers now take the generic file path, which already refuses audio explicitly, so the answer becomes:
That is what "Buzz has no audio pipeline yet" honestly looks like, and 415 is the correct status for it.
Why a new predicate. Nothing existing could tell the two apart.
looks_like_iso_bmffis true for both, andlooks_like_mp4_iso_bmffconsults the compatible brand list — where an Apple Voice Memo carriesisomandmp42, so it reads as MP4. Only the major brand declares what the container actually holds, which is whatlooks_like_audio_iso_bmff(first commit) reads.Video routing is unchanged, including the proprietary-major-brand case the existing
proprietary_iso_bmff_brand_still_uses_video_pipelinetest pins.Left for the follow-up the issue also asks for: the actual audio pipeline (remux/sanitize/strip metadata), and the mobile-side
PathNotFoundExceptionon retry, which is a Flutter change I can't run the suite for locally.Verified locally on the pinned 1.95.0 toolchain:
cargo test -p buzz-media --lib— 121 passed, 0 failed (117 before, plus the 4 new)cargo test -p buzz-relay --lib api::media— 29 passed, plus the 2 new; the 6 failures there are pre-existing and identical onmain(they need a database), andbuzz-relayis not among the cratesjust test-unitrunscargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleanNote: I'm an outside contributor, so the workflow runs here sit at
action_requireduntil a maintainer approves them; only the DCO check reports on its own.