Mp4: Update chunk offsets when creating udta/meta atoms - #688
Closed
jfietz wants to merge 1 commit into
Closed
Conversation
`update_offsets` was only called from `save_to_existing`. The two paths that *create* the tag container - "No `udta` atom found, creating one" and "No `meta` atom found, creating one" - spliced their bytes into `moov` without updating `stco`/`co64`. For files where `moov` precedes `mdat` (i.e. anything muxed with faststart), this shifts the media data later in the file while every chunk offset keeps pointing at the old location, so the media stream is no longer decodable. Files with `mdat` before `moov` are unaffected, as are files that already have a `udta`/`meta` pair, which is why this went unnoticed. Both call sites now update the offsets before the splice, while the offset atoms are still at their original positions. Adds three regression tests: two over minimal synthetic MP4s covering both creation paths, and one over `faststart_no_udta.mp4`, a real H.264/AAC file that the current implementation corrupts. That asset was generated with ffmpeg's `testsrc`/`sine` sources and contains no copyrighted material. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Disclosure, per the note in CONTRIBUTING.md: I ran into this on my own media library and worked out the symptom myself, but I used AI for narrowing it down to these specific branches. PR and the tests were done with AI assistance, and I have reviewed and reproduced all of the results below. Happy to adjust the approach or trim the tests if you would prefer something smaller.
Fixes chunk offset handling when the tag container has to be created.
update_offsetswas only reachable fromsave_to_existing. The two branches inwrite_tothat create the container - "Noudtaatom found, creating one" and"No
metaatom found, creating one" - spliced their bytes intomoovwithouttouching
stco/co64. For files wheremoovprecedesmdat, that shifts themedia data while every chunk offset keeps pointing at the old location, leaving
the stream undecodable.
Both call sites now call
update_offsetsbefore the splice, while the offsetatoms are still at their pre-splice positions.
Adds three regression tests:
create_udta_updates_chunk_offsetsandcreate_meta_updates_chunk_offsets,over minimal hand-built MP4s, one per creation path.
create_udta_updates_chunk_offsets_in_real_file, over a newfaststart_no_udta.mp4asset - a real H.264/AAC file that the currentimplementation corrupts. Generated from ffmpeg's synthetic
testsrc/sinesources, so it carries no copyrighted content.
All three fail on
mainand pass with the fix.cargo test,cargo fmtandcargo clippyare clean.closes #686
Unrelated to this change, but noticed while running the checks in
CONTRIBUTING.md:
cargo clippy --all-targetsfails on current stable forreasons that predate this branch. CI does not catch it, since it runs
cargo clippy --all-featureswithout--all-targetsand so never lints theexamples. #689 fixes that separately.