fix(audio): custom volume normalization to resolve muffled audio#3807
Open
kairosci wants to merge 6 commits into
Open
fix(audio): custom volume normalization to resolve muffled audio#3807kairosci wants to merge 6 commits into
kairosci wants to merge 6 commits into
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAudio loudness normalization is migrated from Android's LoudnessEnhancer to a new VolumeNormalizationAudioProcessor. MusicService instantiates and wires the processor, applies cached gain immediately, computes per-track target gain asynchronously, and simplifies audio-effect session tracking and renderer wiring. ChangesLoudness Normalization Processor Replacement
sequenceDiagram
participant MusicService
participant VolumeNormalizationAudioProcessor
participant DefaultAudioSink
MusicService->>VolumeNormalizationAudioProcessor: configure(format)
MusicService->>VolumeNormalizationAudioProcessor: setTargetGain(gainMb)
DefaultAudioSink->>VolumeNormalizationAudioProcessor: queueInput(buffer)
alt enabled && targetGainMb != 0
VolumeNormalizationAudioProcessor->>DefaultAudioSink: output processed buffer (gain applied)
else disabled or zero gain
VolumeNormalizationAudioProcessor->>DefaultAudioSink: output passthrough buffer
end
DefaultAudioSink->>VolumeNormalizationAudioProcessor: queueEndOfStream()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt (2)
92-98: 💤 Low valueSimplify redundant buffer identity check.
The condition
outputBuffer === inputBuffercompares the class fieldoutputBufferwith the method parameterinputBuffer. This will rarely (if ever) be true and adds confusion. TheEMPTY_BUFFERcheck is sufficient.♻️ Proposed simplification
- if (outputBuffer === EMPTY_BUFFER || outputBuffer === inputBuffer) { - outputBuffer = ByteBuffer.allocateDirect(inputSize).order(ByteOrder.nativeOrder()) - } else if (outputBuffer.capacity() < inputSize) { + if (outputBuffer === EMPTY_BUFFER || outputBuffer.capacity() < inputSize) { outputBuffer = ByteBuffer.allocateDirect(inputSize).order(ByteOrder.nativeOrder()) } else { outputBuffer.clear() }🤖 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 92 - 98, Remove the redundant identity comparison between the field outputBuffer and the parameter inputBuffer in the buffer allocation logic inside VolumeNormalizationAudioProcessor (the block that currently tests outputBuffer === EMPTY_BUFFER || outputBuffer === inputBuffer). Change the first condition to only check outputBuffer === EMPTY_BUFFER (keep the subsequent capacity check and clear behavior unchanged) so the code reads: if (outputBuffer === EMPTY_BUFFER) { allocate... } else if (outputBuffer.capacity() < inputSize) {...} else { outputBuffer.clear() }.
24-37: ⚡ Quick winCross-thread visibility:
linearGainandenabledshould be@Volatile.
setTargetGain()is synchronized and called from the main thread, whilequeueInput()runs on the audio rendering thread and readslinearGainandenabledwithout synchronization. Without@Volatile, the audio thread may see stale values.♻️ Proposed fix to ensure cross-thread visibility
- var enabled = false + `@Volatile` + var enabled = false set(value) { if (field != value) { field = value Timber.tag(TAG).d("Normalization processor enabled: $value") } } - private var inputBuffer: ByteBuffer = EMPTY_BUFFER private var outputBuffer: ByteBuffer = EMPTY_BUFFER private var inputEnded = false private var targetGainMb: Int = 0 - private var linearGain: Double = 1.0 + `@Volatile` + private var linearGain: Double = 1.0🤖 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 24 - 37, The fields accessed across threads (enabled and linearGain) need the `@Volatile` modifier to guarantee visibility between the main thread (setTargetGain()) and the audio rendering thread (queueInput()); update the declarations of var enabled and var linearGain to be annotated with `@Volatile` so reads in queueInput() always see the latest values written by setTargetGain().
🤖 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 92-98: Remove the redundant identity comparison between the field
outputBuffer and the parameter inputBuffer in the buffer allocation logic inside
VolumeNormalizationAudioProcessor (the block that currently tests outputBuffer
=== EMPTY_BUFFER || outputBuffer === inputBuffer). Change the first condition to
only check outputBuffer === EMPTY_BUFFER (keep the subsequent capacity check and
clear behavior unchanged) so the code reads: if (outputBuffer === EMPTY_BUFFER)
{ allocate... } else if (outputBuffer.capacity() < inputSize) {...} else {
outputBuffer.clear() }.
- Around line 24-37: The fields accessed across threads (enabled and linearGain)
need the `@Volatile` modifier to guarantee visibility between the main thread
(setTargetGain()) and the audio rendering thread (queueInput()); update the
declarations of var enabled and var linearGain to be annotated with `@Volatile` so
reads in queueInput() always see the latest values written by setTargetGain().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79921901-695b-43e3-8141-f95305e0bb17
📒 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
… processor to fix muffled audio
kairosci
force-pushed
the
fix/audio-normalization
branch
from
d97e39a to
c56c8ba
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt (2)
33-34: 💤 Low valueThread visibility gap between
setTargetGainandqueueInput.
setTargetGainis@Synchronized, butqueueInputreadstargetGainMbandlinearGainwithout synchronization. Since ExoPlayer calls these methods from different threads (playback vs. audio rendering), there's no happens-before guarantee that writes insetTargetGainare visible toqueueInput.In practice, stale values for a few audio samples are likely inaudible, but for correctness consider marking
linearGainas@Volatile:♻️ Proposed fix
- private var linearGain: Double = 1.0 + `@Volatile` + private var linearGain: Double = 1.0Also applies to: 41-48, 96-98
🤖 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 33 - 34, set the shared fields used by different threads to be volatile so writes in setTargetGain are visible to queueInput: annotate linearGain (and targetGainMb) with Kotlin's `@Volatile` and keep setTargetGain synchronized; this ensures updates to linearGain and targetGainMb made in setTargetGain are seen by queueInput and other audio-rendering threads (refer to setTargetGain, queueInput, targetGainMb, linearGain).
11-39: 💤 Low valueUnused field
inputBufferat line 29.The
inputBufferfield is initialized but never read—it's only assigned to inreset(). The parameterinputBufferinqueueInputshadows this field, which adds confusion. Consider removing this unused field.♻️ Proposed fix
- private var inputBuffer: ByteBuffer = EMPTY_BUFFER private var outputBuffer: ByteBuffer = EMPTY_BUFFER private var inputEnded = falseAnd in
reset():override fun reset() { `@Suppress`("DEPRECATION") flush() - inputBuffer = EMPTY_BUFFER sampleRate = 0🤖 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 11 - 39, The private field inputBuffer is never read and is shadowed by the parameter in queueInput; remove the unused field declaration (private var inputBuffer) and update reset() and any other places that assigned to it so they operate on the local/parameter buffers (e.g., queueInput and reset methods should only manipulate the parameter/local buffers and outputBuffer), ensuring no remaining references to inputBuffer remain and that queueInput's parameter name is not shadowing any removed member.
🤖 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 33-34: set the shared fields used by different threads to be
volatile so writes in setTargetGain are visible to queueInput: annotate
linearGain (and targetGainMb) with Kotlin's `@Volatile` and keep setTargetGain
synchronized; this ensures updates to linearGain and targetGainMb made in
setTargetGain are seen by queueInput and other audio-rendering threads (refer to
setTargetGain, queueInput, targetGainMb, linearGain).
- Around line 11-39: The private field inputBuffer is never read and is shadowed
by the parameter in queueInput; remove the unused field declaration (private var
inputBuffer) and update reset() and any other places that assigned to it so they
operate on the local/parameter buffers (e.g., queueInput and reset methods
should only manipulate the parameter/local buffers and outputBuffer), ensuring
no remaining references to inputBuffer remain and that queueInput's parameter
name is not shadowing any removed member.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe454c35-efc6-44c7-84f4-77aeaa86afe0
📒 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/audio/VolumeNormalizationAudioProcessor.kt`:
- Around line 33-37: queueInput can observe targetGainMb and linearGain
mid-update because setTargetGain writes them in two steps; fix by introducing an
atomic snapshot of the gain state and use that single snapshot in queueInput and
setTargetGain. Concretely, create a small immutable holder (e.g., GainState with
targetGainMb and linearGain) and replace the two `@Volatile` fields with one
AtomicReference<GainState> (or a single `@Volatile` GainState), update both values
by swapping the GainState in setTargetGain, and read the snapshot once at the
start of queueInput (referencing VolumeNormalizationAudioProcessor, queueInput,
setTargetGain, targetGainMb, linearGain) so buffer processing always uses a
consistent pair of 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: 8a49d079-f505-420b-ae60-8b2830c16aa2
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt
…e replaceOutputBuffer helper
Contributor
Author
|
TESTED: There is a difference, and you can really feel. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/audio/VolumeNormalizationAudioProcessor.kt`:
- Around line 57-63: VolumeNormalizationAudioProcessor currently
advertises/passes through 24/32/float PCM which breaks downstream processors
that only accept C.ENCODING_PCM_16BIT; change
VolumeNormalizationAudioProcessor.configure() so it only outputs
C.ENCODING_PCM_16BIT (set bytesPerSample = 2) and either reject non-16-bit
inputs with AudioProcessor.UnhandledAudioFormatException or perform an explicit
conversion in the sample processing path (the code around lines handling
bytesPerSample and the sample processing loop in
VolumeNormalizationAudioProcessor.process()/configure(), referenced as
bytesPerSample switch and sample processing block) to convert 24/32/float inputs
to 16-bit PCM before returning the configured output format so
CustomEqualizerAudioProcessor and SilenceDetectorAudioProcessor receive
C.ENCODING_PCM_16BIT.
🪄 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: 7168a3eb-2fbe-463e-9643-f156f448cc5e
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@nyxiereal ready to review. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/audio/VolumeNormalizationAudioProcessor.kt`:
- Around line 102-125: In VolumeNormalizationAudioProcessor, the 24/32-bit
branches currently apply gain to full-width samples then clamp to 16-bit,
causing widespread clipping; instead downshift the raw sample into 16-bit range
first (use read24Bit(...) shr 8 for C.ENCODING_PCM_24BIT and
inputBuffer.getInt() shr 16 for C.ENCODING_PCM_32BIT), then apply gain to that
downshifted value (use gain.linearGain), coerceIn(-32768, 32767) and toShort for
output; when applyGain is false just use the downshifted value converted to
Short. Ensure you reference read24Bit, gain.linearGain, applyGain, and the two
C.ENCODING_PCM_* branches when making the change.
🪄 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: 4ecb02d5-b02c-4944-8605-836bac7149a2
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/audio/VolumeNormalizationAudioProcessor.kt
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.
Problem
Audio frequently sounds muffled.
Cause
The use of Android's native
LoudnessEnhancerfor volume normalization triggers the hardware DSP on many devices, applying aggressive dynamic range compression that squashes high frequencies and muffles the audio even when simply normalizing.Solution
LoudnessEnhancerwith a customVolumeNormalizationAudioProcessor.LoudnessEnhancersession management logic.Testing
assembleFossDebug.Related Issues
Summary by CodeRabbit
New Features
Bug Fixes