[not-merge] fix(audio): apply normalization immediately from YouTube response, reset stale cross-track gain#3905
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesMusicService Normalization Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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}
|
@yarikeua @Bec-de-Xorbin It seems to work, can you test and confirm that it works? |
|
@kairosci I tested your version and now everything works without delays. !Update |
Thanks for the quick reaction! Unfortunately no, it still takes ~3 seconds for normalization to kick in. |
Hmm. I'm using Metrolist PR 3905 and when this (https://music.youtube.com/watch?v=i97OkCXwotE) ends and this (https://music.youtube.com/watch?v=dKWfA9LCCOg) begins it takes ~3 seconds to lower volume. |
|
@kairosci There's a slight delay only when changing the normalization settings, specifically the loudness level. Is this how it should be? |
@Bec-de-Xorbin Yes, you are right, when I tested with these tracks, the problem persisted. |
kairosci
force-pushed
the
fix-audio-normalization-timing
branch
from
17c1b46 to
36016fe
Compare
|
@yarikeua @Bec-de-Xorbin could you take another look? Thanks |
|
Yes, this. Not working at all. |
|
@yarikeua @Bec-de-Xorbin could you take another look? Thanks |
Same. Not working at all. |
|
do you have log about this? |
|
|
thanks |
|
@kairosci It works now, but just as it did originally, with a delay of 3-5 seconds. |
|
@yarikeua do you have log also about this? |
|
|
@kairosci Nothing has changed, only the progress bar is now staying at 0:00 while the track is playing. |
kairosci
force-pushed
the
fix-audio-normalization-timing
branch
from
6495f2f to
0502262
Compare
|
@kairosci After a clean install, the progress bar started working again. The delay is still there. Record_2026-06-06-01-11-40_d800bdcb138acf830d1e1a76bd31f651.mp4 |
kairosci
force-pushed
the
fix-audio-normalization-timing
branch
from
0502262 to
29d0896
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt (1)
185-191: 💤 Low valueRemove unused
read24Bitfunction.This function is now dead code—the 24-bit sample reading logic was inlined directly in the
ENCODING_PCM_24BITbranch at lines 104-107.🧹 Proposed removal
- private fun read24Bit(buffer: ByteBuffer): Int { - val b0 = buffer.get().toInt() and 0xFF - val b1 = buffer.get().toInt() and 0xFF - val b2 = buffer.get().toInt() - return (b2 shl 16) or (b1 shl 8) or b0 - }🤖 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/playback/audio/VolumeNormalizationAudioProcessor.kt` around lines 185 - 191, Remove the now-unused helper function read24Bit from VolumeNormalizationAudioProcessor: locate the private fun read24Bit(buffer: ByteBuffer) and delete it, and ensure no other references to read24Bit remain (the 24-bit logic has been inlined in the ENCODING_PCM_24BIT branch), leaving the class without dead code.
🤖 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.
Nitpick comments:
In
`@app/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt`:
- Around line 185-191: Remove the now-unused helper function read24Bit from
VolumeNormalizationAudioProcessor: locate the private fun read24Bit(buffer:
ByteBuffer) and delete it, and ensure no other references to read24Bit remain
(the 24-bit logic has been inlined in the ENCODING_PCM_24BIT branch), leaving
the class without dead code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be58b35b-af80-4a81-86f3-c7671c34b7e1
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktapp/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt
|
@kairosci same( Record_2026-06-06-01-39-08_d800bdcb138acf830d1e1a76bd31f651.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 2
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/playback/MusicService.kt (1)
2226-2228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset normalization when the
FormatEntityrow is missing.This branch still keeps the previous track's gain alive.
onMediaItemTransition()callssetupAudioNormalization()before the new row exists, so the next track can start with stale normalization and, if playback is served from the URL/cache path, keep it until some later async update happens.Suggested fix
isFormatNull -> { - Timber.tag(TAG).d("Loudness row not ready yet; keeping cached normalization state") + Timber.tag(TAG).d("Loudness row not ready yet; resetting normalization to neutral") + cachedNormalizationGainMb = null + cachedNormalizationEnabled = false + playerNormalizationProcessors.values.forEach { it.enabled = false } }🤖 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/playback/MusicService.kt` around lines 2226 - 2228, The isFormatNull branch in MusicService.kt should clear/reset the per-track normalization state instead of keeping the previous track's gain; update the isFormatNull handling inside setupAudioNormalization() (used by onMediaItemTransition()) to call the normalization reset path (e.g., invoke whatever method or logic resets normalization state and related fields that current/previous track normalization uses) so a missing FormatEntity row does not leave stale gain applied; ensure you reference and reset the same variables/flags used by the normal setup path so subsequent async updates can populate fresh values.
🤖 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.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2152-2156: The early return guarded by isCrossfading skips
applying the freshly computed track gain to the incoming player's normalization
processor; instead, always cache the new clampedGain and ensure it is applied to
the processor instance for the player being prepared (or to the secondary
ExoPlayer's processor) even during a crossfade: modify the block around
startCrossfade()/isCrossfading so you do not return before calling
playerNormalizationProcessors.values.forEach (or target the specific processor
for the incoming player) to call setTargetGain(clampedGain) and enable the
processor, or alternatively construct the secondary ExoPlayer with normalization
forced neutral until the track-specific loudness arrives.
- Around line 2143-2158: When measuredLufs is null (i.e. no loudness data),
immediately neutralize normalization: set cachedNormalizationGainMb to 0 and
cachedNormalizationEnabled to false, and if not isCrossfading update every
playerNormalizationProcessors entry by calling setTargetGain(0) and setting
enabled = false; if isCrossfading, still update the cached values but return
early as current code does. Locate this logic in MusicService.kt around the
block using targetLufs, measuredLufs, cachedNormalizationGainMb,
cachedNormalizationEnabled, isCrossfading and playerNormalizationProcessors, and
add an else branch handling the null measuredLufs case with the steps above (use
MIN_GAIN_MB/MAX_GAIN_MB only for the normal path where measuredLufs exists).
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2226-2228: The isFormatNull branch in MusicService.kt should
clear/reset the per-track normalization state instead of keeping the previous
track's gain; update the isFormatNull handling inside setupAudioNormalization()
(used by onMediaItemTransition()) to call the normalization reset path (e.g.,
invoke whatever method or logic resets normalization state and related fields
that current/previous track normalization uses) so a missing FormatEntity row
does not leave stale gain applied; ensure you reference and reset the same
variables/flags used by the normal setup path so subsequent async updates can
populate fresh values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 637f4ca8-4337-4195-a206-ceeeca4b30cf
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
|
@yarikeua could you take another look? Thanks again |
|
@yarikeua could you take another look? If it's still not working, could you send me the logs? Thanks again! |
@kairosci still the same( |
kairosci
force-pushed
the
fix-audio-normalization-timing
branch
from
e7fbada to
3ee06a1
Compare
|
@kairosci Good evening, what's the latest? I'm really annoyed by this delay, where the song just screams for the first few seconds before it gets back to normal. |
|
@yarikeua hi, I think I've fixed it; I'm testing it now, and I can't reproduce the problem. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
2273-2304: 💤 Low valueUnused
mediaIdparameter.The
mediaIdparameter is declared but never referenced in the function body. If it's intended for future logging or debugging, consider adding a comment; otherwise, remove it to reduce confusion.Proposed fix
private fun applyNormalizationFromLoudnessData( - mediaId: String, loudnessDb: Double?, perceptualLoudnessDb: Double?, )And update the call site accordingly:
-applyNormalizationFromLoudnessData(mediaId, loudnessDb, perceptualLoudnessDb) +applyNormalizationFromLoudnessData(loudnessDb, perceptualLoudnessDb)🤖 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/playback/MusicService.kt` around lines 2273 - 2304, The mediaId parameter in the applyNormalizationFromLoudnessData function is declared but never used in the function body. Remove the mediaId parameter from the function signature of applyNormalizationFromLoudnessData, and then locate and update all call sites where this function is invoked to remove the mediaId argument being passed to it.
🤖 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.
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2273-2304: The mediaId parameter in the
applyNormalizationFromLoudnessData function is declared but never used in the
function body. Remove the mediaId parameter from the function signature of
applyNormalizationFromLoudnessData, and then locate and update all call sites
where this function is invoked to remove the mediaId argument being passed to
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1413f06-daca-4b76-bdc3-f11df74c9456
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
|
@yarikeua click here to download; let me now if it works |
@kairosci Still the same for me( Record_2026-06-19-14-18-38_d800bdcb138acf830d1e1a76bd31f651.mp4 |
|
I don't understand why I can't reproduce the problem on the songs I listen to. Do you have the logs? |
@kairosci this is the full log |
Why can't you listen to the two songs I gave as an example? It's clearly noticeable because the first song is quieter and the second is louder. Should I open an issue so somebody else could take a look? |
kairosci
force-pushed
the
fix-audio-normalization-timing
branch
from
3ee06a1 to
ca96133
Compare
…eset stale cross-track gain
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 4088-4091: Resolve the empty catch blocks by handling the
exceptions explicitly instead of swallowing them in MusicService’s receiver
teardown paths, including the unregisterReceiver(screenStateReceiver) block and
the other similar catch near the widget/receiver cleanup section. Rename
intentionally ignored exceptions to the configured ignored name if they are
truly safe to ignore, or log/report the failure with enough context so detekt no
longer flags EmptyCatchBlock/SwallowedException and teardown problems are
visible.
- Around line 427-430: The cached normalization state is currently global, so a
previous track or resolver update can be applied to the next media item before
setupAudioNormalization() validates the new mediaId. Update the cached state in
MusicService to include the owning mediaId alongside cachedNormalizationGainMb
and cachedNormalizationEnabled, and make applyCachedAudioNormalizationNow() only
apply or restore normalization when that cached mediaId matches
player.currentMediaItem?.mediaId; otherwise neutralize the cache before any
transition path reuses it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5ade87c-3bdc-4618-9e79-5ca5e7d51b24
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 4088-4091: Resolve the empty catch blocks by handling the
exceptions explicitly instead of swallowing them in MusicService’s receiver
teardown paths, including the unregisterReceiver(screenStateReceiver) block and
the other similar catch near the widget/receiver cleanup section. Rename
intentionally ignored exceptions to the configured ignored name if they are
truly safe to ignore, or log/report the failure with enough context so detekt no
longer flags EmptyCatchBlock/SwallowedException and teardown problems are
visible.
- Around line 427-430: The cached normalization state is currently global, so a
previous track or resolver update can be applied to the next media item before
setupAudioNormalization() validates the new mediaId. Update the cached state in
MusicService to include the owning mediaId alongside cachedNormalizationGainMb
and cachedNormalizationEnabled, and make applyCachedAudioNormalizationNow() only
apply or restore normalization when that cached mediaId matches
player.currentMediaItem?.mediaId; otherwise neutralize the cache before any
transition path reuses it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5ade87c-3bdc-4618-9e79-5ca5e7d51b24
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
🛑 Comments failed to post (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (2)
427-430: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make cached normalization state media-scoped before applying it on transitions.
Line 2556 reapplies the last cached gain before
setupAudioNormalization()verifies the newmediaId, but Lines 427-430 only cache gain/enabled globally. If the cache still belongs to the previous track—or a resolver fetch for another item updates it at Lines 3796-3798—the first buffers of the new track can receive the wrong normalization. Store the owningmediaIdwith the cached state and haveapplyCachedAudioNormalizationNow()neutralize unless it matchesplayer.currentMediaItem?.mediaId. This is central to the PR’s stale cross-track gain objective.Also applies to: 2278-2289, 2556-2557, 3796-3798
🤖 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/playback/MusicService.kt` around lines 427 - 430, The cached normalization state is currently global, so a previous track or resolver update can be applied to the next media item before setupAudioNormalization() validates the new mediaId. Update the cached state in MusicService to include the owning mediaId alongside cachedNormalizationGainMb and cachedNormalizationEnabled, and make applyCachedAudioNormalizationNow() only apply or restore normalization when that cached mediaId matches player.currentMediaItem?.mediaId; otherwise neutralize the cache before any transition path reuses it.
4088-4091: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the detekt empty-catch warnings.
These catches currently swallow exceptions with no signal. Rename intentionally ignored exceptions to the configured ignored name, or log/report recoverable failures so widget/receiver teardown issues are not invisible. Based on static analysis hints, detekt flags these as
EmptyCatchBlock/SwallowedException.Proposed fix
- } catch (e: Exception) { + } catch (_: Exception) { }- } catch (e: Exception) { + } catch (e: Exception) { + Timber.tag(TAG).w(e, "Failed to update widgets") } finally {Also applies to: 4494-4495
🧰 Tools
🪛 detekt (1.23.8)
[warning] 4090-4091: Empty catch block detected. If the exception can be safely ignored, name the exception according to one of the exemptions as per the configuration of this rule.
(detekt.empty-blocks.EmptyCatchBlock)
[warning] 4090-4090: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 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/playback/MusicService.kt` around lines 4088 - 4091, Resolve the empty catch blocks by handling the exceptions explicitly instead of swallowing them in MusicService’s receiver teardown paths, including the unregisterReceiver(screenStateReceiver) block and the other similar catch near the widget/receiver cleanup section. Rename intentionally ignored exceptions to the configured ignored name if they are truly safe to ignore, or log/report the failure with enough context so detekt no longer flags EmptyCatchBlock/SwallowedException and teardown problems are visible.Source: Linters/SAST tools
kairosci
force-pushed
the
fix-audio-normalization-timing
branch
from
ca96133 to
e2f0913
Compare
kairosci
changed the title
…c setupAudioNormalization - Remove setupAudioNormalization() entirely (async DB-query path) - Remove loudnessSetupJob/loudnessSetupGeneration fields - applyNormalizationFromLoudnessData() is now the single entry point - Data source factory: applies gain immediately from YouTube response (time 0) - Preference collector: recomputes gain directly from reactive format flow - Track change: invalidates cache, data source factory sets correct gain before audio starts
|
@Bec-de-Xorbin I’m trying this with your song right now; it seems to be working. Could you have a look at it too? Thanks |
Not working. |
3 participants
Problem
After PR #3807, transitioning between tracks causes playback to start at full volume — normalization takes ~3 seconds to kick in, even with crossfade disabled.
Cause
Two root causes:
Stale cross-track state retained: In
setupAudioNormalization(), when the Room database has no loudness data yet for the new track (first playback), theformat == nullbranch did nothing — keeping the previous track's gain values on the processor. This caused wrong boost/cut to be applied to the new track.Slow async pipeline: Loudness data fetched from the YouTube response goes through Room→Flow→Combine→coroutine before being applied, adding latency even when the data is already available at stream fetch time.
Solution
Reset to neutral on missing data: The
format == nullbranch insetupAudioNormalization()now immediately resets the processor to neutral (enabled=false, gain=null) instead of keeping stale cross-track state. This prevents the previous track's gain from leaking into the new track.Bypass the Room pipeline: New
applyNormalizationFromLoudnessData()is called directly from the data source factory right after the YouTube response provides loudness data — applying gain to theVolumeNormalizationAudioProcessorsynchronously, before a single audio sample reaches the processor. The Room→Flow→Combine reactive path becomes a secondary convergence mechanism.Testing
assembleFossDebugbuilds successfullyRelated Issues
Summary by CodeRabbit
Bug Fixes
Performance & Stability