fix(audio): prioritize highest quality audio codec over numerical bitrate#3809
fix(audio): prioritize highest quality audio codec over numerical bitrate#3809kairosci wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughYTPlayerUtils.findFormat now prefers a ranked itag list (scanned from a startItag derived from AudioQuality and network metered state) to pick an exact audio format; if none match, it falls back to the existing bitrate/target-based selection. ChangesAudio format selection strategy
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
kairosci
force-pushed
the
feat/fix-highest-audio-quality
branch
5 times, most recently
from
54d4658 to
7ac88b8
Compare
kairosci
force-pushed
the
feat/fix-highest-audio-quality
branch
from
7ac88b8 to
933a3f1
Compare
|
I tried the PR 3809 build, and VERY_HIGH ("Highest possible") uses itag 140 on every track on this album (https://music.youtube.com/playlist?list=OLAK5uy_mqKanBXpCEbjgm8W3MU8ZCtPQzYn_LaVs) but one (https://music.youtube.com/watch?v=Ikmdr_TNrbQ) which uses itag 251. With HIGH it's completely opposite, it uses itag 251 on every track on this album (https://music.youtube.com/playlist?list=OLAK5uy_mqKanBXpCEbjgm8W3MU8ZCtPQzYn_LaVs) but one (https://music.youtube.com/watch?v=Ikmdr_TNrbQ) which uses itag 140. So I'm struggling to see any logic here... I'd still sort them all by itag and forget the bitrate sort completely so the logic would be predictable and clean. Something like this: VERY_HIGH: start the search from 774 and select the highest available itag from the list |
…ection Replace the bitrate-sorting approach with a fixed itag priority ranking (774, 141, 251, 140, 250, 249, 139, 600, 599) that selects the highest available codec from the appropriate starting point per quality level. - VERY_HIGH: start from 774 - HIGH: start from 251 - LOW: start from 250 - AUTO (metered): start from 250, else 774 This ensures predictable codec-aware selection unaffected by VBR bitrate fluctuations.
|
@Bec-de-Xorbin thx for test; I believe I fixed, could you pls retest? |
Works as expected, thank you! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.kt (1)
437-449:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReintroduce a non-ranked fallback when no ranked itag matches.
If
selectedItagis null, Line 449 returns null immediately, even whenaudioCapableFormatsis non-empty. That can unnecessarily fail selection for valid streams whose itags are not initagRanking(and this propagates to playback/download format choice).Suggested patch
- val format = selectedItag?.let { tag -> audioCapableFormats.first { it.itag == tag } } + val format = selectedItag?.let { tag -> audioCapableFormats.first { it.itag == tag } } - if (format != null) { + if (format != null) { Timber.tag(logTag).d("Selected format: itag=${format.itag}, mimeType=${format.mimeType}, bitrate=${format.bitrate}, audioQuality label: ${format.audioQuality}") - } else { - Timber.tag(logTag).d("No suitable audio format found") + return format } - return format + // Fallback: preserve resiliency when ranking does not cover available itags. + val fallback = when (audioQuality) { + AudioQuality.VERY_HIGH -> audioCapableFormats.maxByOrNull { it.bitrate } + AudioQuality.HIGH -> audioCapableFormats.maxByOrNull { it.bitrate } + AudioQuality.LOW -> audioCapableFormats.minByOrNull { it.bitrate } + AudioQuality.AUTO -> { + if (connectivityManager.isActiveNetworkMetered) { + audioCapableFormats.minByOrNull { it.bitrate } + } else { + audioCapableFormats.maxByOrNull { it.bitrate } + } + } + } + if (fallback != null) { + Timber.tag(logTag).d("Selected fallback format: itag=${fallback.itag}, bitrate=${fallback.bitrate}") + } else { + Timber.tag(logTag).d("No suitable audio format found") + } + return fallback🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.kt` around lines 437 - 449, The code currently returns null if selectedItag is not found in itagRanking, causing missed valid formats; modify the selection logic around selectedItag and format so that when selectedItag is null but audioCapableFormats is non-empty you fall back to a non-ranked choice (e.g., pick audioCapableFormats.maxByOrNull { it.bitrate } or the first element) instead of returning null—update the block that computes format (and the following Timber logging) to use this fallback so a valid Format is returned when available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.kt`:
- Around line 437-449: The code currently returns null if selectedItag is not
found in itagRanking, causing missed valid formats; modify the selection logic
around selectedItag and format so that when selectedItag is null but
audioCapableFormats is non-empty you fall back to a non-ranked choice (e.g.,
pick audioCapableFormats.maxByOrNull { it.bitrate } or the first element)
instead of returning null—update the block that computes format (and the
following Timber logging) to use this fallback so a valid Format is returned
when available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6673eb88-bcfe-4e89-83fd-3c0cfceaa71c
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/utils/YTPlayerUtils.kt
2 participants
Problem
"High" or "Highest possible" audio quality selection didn't always choose the true highest quality audio codec (e.g. itag 774 for premium users), and sometimes downgraded due to slight VBR bitrate fluctuations crossing the hardcoded target bitrate limits.
Cause
itagRankinglist completely omitted the recently introduceditag 774(Opus 256kbps), falling back to lower quality codecs.<= 256000). Since YouTube streams can have slightly variable bitrates (like 256008), this caused valid high-quality formats to be incorrectly filtered out, forcing a fallback to 160kbps (itag 251) on "High" settings.Solution
itag 774(Opus 256kbps) as the highest priority format in theitagRankinglist.targetBitratecalculation (e.g.,260000.0instead of256000.0) to safely catch VBR streams that slightly exceed the nominal bitrate, preventing unintended quality downgrades.Testing
assembleFossDebug.itag 774is correctly prioritized and selected when using "Highest possible" and "High" qualities.Related Issues
Summary by CodeRabbit