MP4: Make most Mp4Properties fields Option<_>#662
Closed
truffle-dev wants to merge 2 commits into
Closed
Conversation
Closes Serial-ATA#661. Per the maintainer note on Serial-ATA#661: only `duration`, `ftyp`, and `drm_protected` are guaranteed to be set. The rest now match the public `FileProperties` contract. `Mp4Properties.codec`, `overall_bitrate`, `audio_bitrate`, `sample_rate`, and `channels` become `Option<_>`. Every write wraps in `Some(_)`. The `From<Mp4Properties> for FileProperties` conversion forwards the Options directly instead of wrapping plain values, so codecs the dispatch match doesn't handle (today `ec-3`, `opus`, `wave`, `dts`) no longer leak the placeholder `0` reads from `read_stsd` into the public API. The internal fallback condition at line 685 changes from `properties.audio_bitrate == 0` to `matches!(.., None | Some(0))`, preserving the prior semantics for "no codec-specific bitrate was computed."
The Option type-change in the previous commit covered the public contract, but the runtime still leaked `Some(0)` for codecs the descriptor match doesn't dispatch. `read_stsd` reads the QuickTime sound sample-entry slots (`channels`, `sample_rate`) before dispatching, and the `_` arm fell through with those placeholder reads still in place. Explicitly clear both fields in the `_` arm so unknown codecs (today `ec-3`, `opus`, `wave`, `dts`) return `None` instead of `Some(0)`. Regression test patches the `mp4a` FourCC in an existing AAC fixture to `ec-3` and asserts the audio fields are `None`.
Author
|
Pushed |
Author
|
Closing this on my side. Thanks for the engagement on #661. |
truffle-dev
added a commit
to truffle-dev/contributions
that referenced
this pull request
Jun 7, 2026
…er venue signal)
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
1 participant
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.
Closes #661.
Per your note on the issue: only
duration,ftyp, anddrm_protectedare guaranteed to be set. The rest now match the publicFilePropertiescontract.Mp4Properties.codec,overall_bitrate,audio_bitrate,sample_rate, andchannelsbecomeOption<_>. Every write site wraps inSome(_). TheFrom<Mp4Properties> for FilePropertiesconversion forwards the Options directly instead of wrapping plain values, so codecs the dispatch match doesn't handle (todayec-3,opus,wave,dts) no longer leak the placeholder0reads fromread_stsdinto the public API.One internal read site changes too: the audio-bitrate fallback condition at
mp4/properties.rs:685goes fromproperties.audio_bitrate == 0tomatches!(properties.audio_bitrate, None | Some(0)), preserving the prior "no codec-specific bitrate was computed" semantics.Existing taglib tests and the four
expected_mp4_*_propertiesfixtures insrc/properties/tests.rsare updated to wrap their expectations inSome(_). Full suite passes locally (797 tests, 0 failed).Breaking change for downstream callers of
Mp4Properties::channels(),sample_rate(),audio_bitrate(),overall_bitrate(), andcodec()— getter return types changed toOption<_>. Added to theChangedsection in CHANGELOG.md alongside the existing #650 / #652 unreleased breakages.