fix(quality): let a real encoder bitrate reach the rung it was encoded for - #934
Open
m4bard wants to merge 1 commit into
Open
fix(quality): let a real encoder bitrate reach the rung it was encoded for#934m4bard wants to merge 1 commit into
m4bard wants to merge 1 commit into
Conversation
…d for A file encoded at a nominal bitrate almost never reports that figure exactly. A "128kbps" AAC file commonly reports around 127241 bps. NormalizeKbps rounds that to 127, and rung eligibility was `rung.BitrateKbps <= fileKbps`, so the 128 rung was excluded and the file fell to the tier below. If that lower tier sits under the profile's cutoff, the book reports quality-mismatch. The failure is systematic rather than occasional, because it happens to any lossy file whose reported bitrate lands even one kbps under its nominal tier, which is most of them. Eligibility now goes through MeetsRung, which allows a file to reach a rung it falls just short of. The tolerance is five percent with a floor of one kbps. That is far below the gap between adjacent rungs in any ordinary profile, where 64, 96, 128, 192, 256 and 320 sit at least a third apart, so it cannot promote a file across a real tier boundary. A test pins that: a 96kbps file against a profile of 128 and 64 still matches 64. Two tests, both failing on canary. One at the matcher, one at AudiobookStatusEvaluator so the user-visible status is covered too. Worth noting why the existing tests did not catch this. Every bitrate in QualityMatcherTests and AudiobookStatusEvaluatorTests is an exactly round tier value: 256000, 320000, 128. The suite only ever asks the question in the one form where the answer was already right. Co-Authored-By: Claude Opus 5 (1M context) <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.
Fixes one of the causes behind #549, where books report
quality-mismatchagainst a profile theyshould satisfy.
What happens
QualityMatcher.NormalizeKbpsrounds a reported bitrate to whole kbps, and rung eligibility wasA file encoded at a nominal 128kbps does not report 128000 bps. It reports something like 127241,
which rounds to 127, so the 128 rung is excluded and the file falls to the tier below. Where that
lower tier sits under the profile cutoff, the book reports a mismatch.
Most lossy files land at least a little under their nominal tier, so this is not an occasional
rounding artifact. It is closer to the normal case.
The change
Eligibility goes through
MeetsRung, which lets a file reach a rung it falls just short of. Thetolerance is five percent, with a floor of one kbps.
Adjacent rungs in an ordinary profile sit much further apart than that. 64, 96, 128, 192, 256 and 320
are at least a third apart, so the tolerance cannot carry a file across a real tier boundary. There
is a test for exactly that: a 96kbps file against a profile of 128 and 64 still matches 64.
Tests
Two, both failing before the change. One at
QualityMatcher, one atAudiobookStatusEvaluatorsothe user-visible status is covered and not just the internals.
It is worth saying why the existing tests did not catch this. Every bitrate in
QualityMatcherTestsand
AudiobookStatusEvaluatorTestsis an exactly round tier value: 256000, 320000, 128. The suiteonly ever asks the question in the one form where the answer was already right.
Scope, and what this does not fix
This is one cause. It is not the whole of #549.
Measured on a real library before and after, the share of books with a file reporting
quality-mismatchwent from roughly nine in ten to roughly two in three. Better, and obviously notfinished.
The rest looks like a different mechanism, which I have left alone here.
AudiobookStatusEvaluator.ComputeStatusfilters the file list by the profile'sPreferredFormatsbefore the matcher is consulted, and a book left with no candidate files returns
QualityMismatch.So a book whose only files are in a format the profile does not prefer reports a quality mismatch
whatever its bitrate:
On that library, every book that matched was AAC and every MP3-only book was mismatched regardless
of bitrate, against an m4b-only
PreferredFormats.I do not think that is mine to decide. There is a reading where a book in a format you did not ask
for is legitimately not what you wanted. But
quality-mismatchis the status the UI reads, sotreating a format preference as a quality shortfall reaches further than the label does, and
redefining that status does not belong in a pull request about rounding. I can open a separate issue
for it if that is useful.
Worked through with Claude Code at my direction. The mechanism and both tests were checked by
running them, including confirming they fail without the change. The before and after rates come
from a real library rather than from the test suite. I reviewed this before posting.