Let MP3 average its bitrate, and keep a constant rung for those who want it - #55
Merged
Conversation
…ant it Every MP3 Offstream wrote held each frame at exactly the rate the user picked. libmp3lame does that unless it is told otherwise, and it is the wrong default: a frame of fade-out gets the same allocation as a dense chorus, so bits are spent on silence that the loud passage two seconds later could have used. Every other lossy profile here already varies its rate — libopus and the native AAC encoder both do it unasked — so MP3 was the odd one out rather than the careful one. The reason it stayed that way is written down in the plan, and the reasoning was sound about the trade-off and wrong about the conclusion. Turning averaging on by itself would move the default down a notch on the most-used format, and there has to be a way back to a full-rate file. The objection to adding one was that the bitrate would stop being a plain kbps number, which is the LAMEPreset-shaped setting §5.1 rejected. But a preset enum bundles the rate and the mode into a single value, and that bundling is what was rejected. Two values — an int and a two-member BitrateMode — keep bitrateKbps a number the file can be hand-edited to and validation can range-check, while the page presents the pair as one ladder. The split costs the settings file its honesty nowhere and the UI nothing. The flag is profile data rather than a branch. EncodingProfile gains AverageBitrateArguments, which holds ["-abr", "1"] for MP3 and is empty everywhere else; Build appends whatever the profile declares and never asks what format it is looking at. Empty is the honest answer for the other lossy formats, not an omission — there is no switch to throw where the encoder already varies its rate, which is also why no constant rung is offered beside them. SupportsBitrateMode falls out of that list being non-empty, so the dropdown and the encoder cannot disagree about a format added later. BitrateMode.Average is the zero value deliberately. SettingsJsonContext's generator yields default(T) for an absent key, so every settings file written before this change resolves to averaging by way of the enum's member order rather than an initializer the generator ignores. There is a test on exactly that, because the failure would be silent and would land on every existing install. The dropdown is now one list of rungs carrying both fields rather than a list of numbers beside a second control, and it says which two rates Spotify itself streams at — 160 for free accounts, 320 for Premium — so recording no worse and no larger than what arrived is a matter of reading the list. The annotations describe the source, not the encode; a lossless stream is above both, which is what the hint says and what FLAC is for. Opus needed nothing. Its flags were compared and are already identical, and neither side sets -vbr or -compression_level, so the defaults match too. Two things measured and deliberately not quoted anywhere. ABR at a given rate produces a smaller file than CBR at the same rate, because the number is a target across the recording rather than a promise about each frame. And pink noise is the wrong source to size that difference on: a steady tone and one with a slow 6 dB swell came back within 0.1% of each other at ABR 320, since LAME allocates on spectral demand and noise is equally easy to code at any level. Neither number says anything about music, so the changelog and the plan describe the mechanism and give no figure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Picking FLAC and then MP3 again lost the constant-rate choice. LoadBitrates coerced the selection to what the chosen format could honour — correct for the dropdown, since offering a constant rung beside Opus is a control the encoder ignores — and then persisted the coerced value, so the detour wrote Average over the user's Constant and returning to MP3 found nothing to restore. The bitrate number survives that same detour, because it is carried through unchanged. One control giving two different answers to "does looking at another format cost me anything" is the part that makes this a bug rather than a simplification, and it is the same reasoning that makes the device list keep a disconnected device rather than quietly rewriting the setting to "system default". The mode is now held in _preferredBitrateMode, separately from the rung on screen. The display stays coerced; the file keeps what was chosen. An explicit selection updates the preference only on a format that offers both modes — everywhere else every rung reads Average because that is all the list holds, so reading the selection as an answer would discard a choice the user never revisited. Also amends the two remaining statements this change left half-true: §7's parity row and RecordingSettings' own remark both say the bitrate is a plain kbps number rather than the predecessor's LAMEPreset, which is still exactly right and now sits directly above a mode enum that looks like the thing being ruled out. Both say which half of the preset each value is. 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.
Every MP3 Offstream wrote held each frame at exactly the rate the user picked. libmp3lame does
that unless it is told otherwise, and it is the wrong default: a frame of fade-out gets the same
allocation as a dense chorus, so bits are spent on silence that the loud passage two seconds later
could have used. Every other lossy profile here already varies its rate — libopus and the native
AAC encoder both do it unasked — so MP3 was the odd one out rather than the careful one.
Why it stayed that way, and what that argument was actually right about
The plan already recorded a decision to leave MP3 at constant rate. That reasoning was sound about
the trade-off and wrong about the conclusion, and it is amended rather than deleted, because it is
what shaped the fix.
Turning averaging on by itself would move the default down a notch on the most-used format, so
there has to be a way back to a full-rate file. The objection to adding one was that the bitrate
would stop being a plain kbps number — the
LAMEPreset-shaped setting §5.1 rejected on purpose.But a preset enum bundles the rate and the mode into a single value, and that bundling is what
was rejected. Two values — an
intand a two-memberBitrateMode— keepbitrateKbpsa numberthe file can be hand-edited to and validation can range-check, while the page presents the pair as
one ladder. The split costs the settings file nothing and the UI nothing.
The flag is profile data, not a branch
EncodingProfilegainsAverageBitrateArguments, holding["-abr", "1"]for MP3 and emptyeverywhere else.
Buildappends whatever the profile declares and never asks what format it islooking at, per §5.1's "data, not code". Empty is the honest answer for the other lossy formats
rather than an omission — there is no switch to throw where the encoder already varies its rate,
which is also why no constant rung is offered beside them.
SupportsBitrateModefalls out of thatlist being non-empty, so the dropdown and the encoder cannot disagree about a format added later.
BitrateMode.Averageis the zero value deliberately.SettingsJsonContext's generator yieldsdefault(T)for an absent key, so every settings file written before this resolves to averagingby way of the enum's member order rather than an initializer the generator ignores.
OmittedBitrateMode_ReadsAsAveragedpins it, because the failure would be silent and would landon every existing install.
Opus: compared, already identical, no change
Flag for flag —
-c:a libopus -b:a {rate}kon both sides, and neither sets-vbror-compression_level, so the defaults match too. libopus is VBR out of the box. Nothing to do, andsaying so explicitly since the request named it.
The dropdown
One list of rungs carrying both fields, rather than a list of numbers beside a second control. It
marks the two rates Spotify itself streams at — 160 for free accounts, 320 for Premium — so
recording no worse and no larger than what arrived is a matter of reading the list. Those
annotations describe the source, not the encode; a lossless stream is above both, which is what
the hint says and what FLAC is for.
Two deliberate deviations from the dropdown as it was shown to me, both easy to overrule:
settings file holds one. Same superset stance already taken on AAC.
Insaneis LAME presetvocabulary,
RecordingSettingsdocuments rejecting exactly that vocabulary, and it tells a usernothing about what the setting does.
Second commit: the choice survives a look at another format
Coercing the rung for display is right — a constant rung beside Opus is a control the encoder
ignores. Persisting the coerced value was not: picking FLAC and then MP3 again wrote
Averageover the user's
Constant, while the bitrate number survived that identical detour. One controlanswering "did I lose anything" two ways is what makes it a bug. The mode is now held apart from
the rung on screen, the same way the device list keeps a disconnected device.
Measurement, and why no number appears anywhere
ABR at a given rate produces a smaller file than CBR at the same rate — the number is a target
across the recording rather than a promise about each frame. Sizing that difference was attempted
on synthetic sources and abandoned: a steady pink-noise tone and one with a slow 6 dB swell came
back within 0.1% of each other at ABR 320, because LAME allocates on spectral demand and noise
is equally easy to code at any level. Neither says anything about music, so the changelog, the
README and the plan describe the mechanism and quote no figure.
Verification
.\build.ps1 -Test— 1,307 passing, 0 warnings (1,025 Core + 282 UI; 11 new)..\build.ps1 -VerifyFormat— clean.The MP3 cover-art integration tests perform real ffmpeg encodes and now do so with
-abr 1in theargv, so the flag is exercised end to end rather than only in the golden vectors. New coverage:
golden argv for both modes;
-abrabsent from every profile that declares nothing, in eithermode;
SupportsBitrateModeis MP3 alone; the omitted-key upgrade path; the format round-trip; anda rate changed on a modeless format leaving the stored mode alone.
Documentation via the
document-changeskill:CHANGELOG.md(twoChangedentries, plus acorrection to an unreleased
Addedentry that said the running size estimate applies to"constant-bitrate formats"),
README.md, anddocs/MODERNIZATION-PLAN.md— the §5.1 MP3 row, the§7 parity row, and the encoding-profile finding, which said "MP3 stays constant-bitrate" and now
records the reversal.
🤖 Generated with Claude Code