fix(player): fall back to ffmpeg for MPEG-2/2.5 low-rate MP3 - #452
Open
cesarho wants to merge 1 commit into
Open
fix(player): fall back to ffmpeg for MPEG-2/2.5 low-rate MP3#452cesarho wants to merge 1 commit into
cesarho wants to merge 1 commit into
Conversation
go-mp3 (github.com/hajimehoshi/go-mp3, the pure-Go decoder beep's mp3 package wraps) parses MPEG-2/2.5 Layer III streams — sample rates below the MPEG-1 floor of 32000Hz, e.g. 22050Hz — without returning an error, but produces audibly garbled/distorted PCM. This is the actual cause of "bad output sound" on low-bitrate Icecast/Shoutcast radio streams that use these rates (common for bandwidth-constrained stations), independent of the resample-headroom clipping fixed in the previous commit. Confirmed by ear against a real 22050Hz stream (RTHK Radio 2, stm2.rthk.hk/radio2): decoding the identical captured bytes with ffmpeg sounded correct; decoding with go-mp3 did not, with or without resampling involved at all (tested decoding straight to the source's native 22050Hz, no resample step in the chain). ffmpeg's mp3float decoder handles this stream correctly. Adds isLowRateMP3() and routes any HTTP/local .mp3 stream detected at this rate range to the existing ffmpeg fallback path (previously only used when go-mp3 returned an outright error) instead of trusting its error-free-but-wrong output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L7ty32fpxBC45NiVTFPAGA
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change detects low-rate MPEG-2/2.5 MP3 streams, rejects unreliable native decoding, and routes these streams through the existing ffmpeg fallback. Tests cover detection and decoded output. ChangesLow-rate MP3 fallback
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Low-rate MP3 streams now use FFmpeg instead of the unreliable native decoder, correcting distorted playback while preserving normal MP3 handling. No current merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant buildPipeline
participant goMP3Decoder
participant localFFmpegStreamer
buildPipeline->>goMP3Decoder: Decode low-rate MP3
goMP3Decoder-->>buildPipeline: Return sample rate below 32000Hz
buildPipeline->>localFFmpegStreamer: Use fallback decoder
localFFmpegStreamer-->>buildPipeline: Produce samples
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5 tasks
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.
Summary
go-mp3(github.com/hajimehoshi/go-mp3, the pure-Go decoderbeep'smp3package wraps — used for all native.mp3HTTP/file decoding in cliamp) parses MPEG-2/2.5 Layer III streams — sample rates below the MPEG-1 floor of 32000Hz (22050/24000/16000Hz for MPEG-2, 11025/12000/8000Hz for MPEG-2.5) — without returning an error, but produces audibly garbled/distorted PCM. This rate range is common on low-bitrate Icecast/Shoutcast radio, which frequently drops to it for bandwidth-constrained stations.Root-caused this against a real report of "bad output sound" on
stm2.rthk.hk/radio2(RTHK Radio 2, a 22050Hz/32kbps MPEG-2 stream). Methodically isolated it by ear:go-mp3straight to its native 22050Hz — no resampling at all in the chain — still garbled. This ruled out resampling, EQ, volume, and the audio device entirely.ffmpeg(mp3float) — clean.go-mp3).So the defect is specifically
go-mp3producing wrong output for this MPEG version/rate combination, silently — no decode error to hang a fallback on.Fix
This isn't a new code path or a new dependency:
ffmpegis already a hard runtime dependency of cliamp, andneedsFFmpeg()already routes.m4a/.aac/.aacp/.m4b/.alac/.wma/.opus/.webmand all HLS through it, plus there's already an existing fallback to ffmpeg when a native decoder (go-mp3/wav/flac/vorbis) returns an outright error. This PR extends that same, already-exercised fallback to trigger in one more well-defined case.player/decode.go: addsisLowRateMP3(ext, sampleRate)— true for.mp3with a sample rate below 32000Hz.player/pipeline.go: after a successful native decode, ifisLowRateMP3is true, discards thego-mp3decoder's output and routes through the existing ffmpeg fallback path (previously only reachable via a decode error) instead of trusting output known to be wrong for this stream class.Testing
go build ./...,go vet ./..., fullgo test ./...— all clean across the whole repo.player/decode_test.go:TestIsLowRateMP3— table test covering MPEG-1/2/2.5 rate boundaries and the.mp3-only scope.TestBuildPipelineRoutesLowRateMP3ToFFmpeg— builds a real pipeline against a small synthesized MPEG-2 22050Hz MP3 fixture (player/testdata/mpeg2_22050.mp3, a 1-second sine tone generated withffmpeg— not copyrighted stream audio) and asserts the resulting decoder is*localFFmpegStreamer, not the nativego-mp3path, plus a basic sanity check that it actually produces samples.Test plan
go build ./...go vet ./...go test ./...stm2.rthk.hk/radio2stream, confirmed by ear🤖 Generated with Claude Code
Summary by CodeRabbit