fix(mp4): preserve hdlr atom when writing new tags to metadata containers with missing ilst#660
Open
axel10 wants to merge 4 commits into
Open
fix(mp4): preserve hdlr atom when writing new tags to metadata containers with missing ilst#660axel10 wants to merge 4 commits into
axel10 wants to merge 4 commits into
Conversation
added 4 commits
May 24, 2026 20:16
…les with empty `ilst`
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.
Description
This PR fixes a bug where writing new tags (
Mp4Ilst) to MP4/M4A files that contain ametabox but noilstbox (e.g., after tag removal or clean metadata copy) would overwrite and destroy thehdlr(Handler Reference) atom. This resulted in malformed M4A files that standard-conforming media players (like FFmpeg, Windows Media Foundation/Groove, CoreMedia, etc.) could no longer read metadata from.Root Cause
In
lofty/src/mp4/read/mod.rs,atom_treereturned(usize, Vec<AtomInfo>)where the first element was the index of the matched atom. If the atom (e.g.,ilst) was not found, it would return0. If it was found at index 0 (which is valid), it would also return0.In
lofty/src/mp4/ilst/write.rs,save_to_existingrelied on this index. Whenilstwas deleted/absent,atom_treereturned0for the index, whiletreestill had child atoms (specifically,hdlrat index 0). The writer then incorrectly assumedtree[0](thehdlratom) was the existingilstand replaced thehdlratom with the newilstatom.Solution
atom_tree: The returned index is now anOption<usize>. It returnsSome(index)if found, andNoneif not found, allowing callers to accurately distinguish whetherilstexists.save_to_existing: It now checks ifilst_idxisNone(viaif let Some(ilst_idx) = ilst_idx). If it isNone, it appendsilstto the end ofmetarather than overwritingtree[0]. If it isSome(idx), it replaces the existingilstatom in place as before. This also resolves a Clippy lint forunnecessary_unwrap.Regression Test
An integration test
hdlr_preservation_on_tag_recreationhas been added tolofty/tests/files/mp4.rs. It simulates the exact tag recreation sequence:m4a_codec_aac.m4a).remove_from_path.save_to_path.hdlratom.All library unit and integration tests pass successfully.