From cf272c4edceca2c76a2b8a2c317fdefcdbbaabd8 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 15 Oct 2026 08:32:54 +0000 Subject: [PATCH] playback: apply the gain only from the item flush Review follow-ups for the apply slice. - Drop the media item transition application. It never supplied a gain the flush had not already supplied, and as a second writer it raced the playback thread: it applied the incoming item's gain while the outgoing item's buffers were still queued, scaling the previous track's tail by the next track's gain (reproduced at +12 dB). The flush is the only writer now. This reverses the earlier decision to keep it as a fallback. That was made on the premise that it was insurance against a flush that could not be resolved. The flush covers every transition, and a period-less flush only happens while the sink is being reset, so the premise does not hold. - Ignore a flush that carries no period rather than reading it as "this item has no gain". Resetting to unity there would have been wrong if a bare flush ever arrived mid-playback; the test that pinned that behaviour now pins the opposite. - Guard the pass-through branch against an empty input, which is the pipeline's own shared buffer and cannot be put into itself. - The controller is written from the playback thread now, so its state updates and the recompute that follows them are synchronized rather than two independent volatile writes. - Add ReplayGainSinkTest, which drives the real DefaultAudioSink and asserts it hands the processor the item being configured. That is the media3 behaviour this whole feature rests on, and it was previously asserted only by comments. --- .../subtracks/playback/Media3PlayerHandle.kt | 4 -- .../com/subtracks/playback/PlaybackService.kt | 1 - .../playback/ReplayGainController.kt | 14 ++-- .../subtracks/playback/ReplayGainProcessor.kt | 10 ++- .../playback/ReplayGainControllerTest.kt | 34 +++------- .../playback/ReplayGainProcessorTest.kt | 16 +++-- .../subtracks/playback/ReplayGainSinkTest.kt | 66 +++++++++++++++++++ 7 files changed, 100 insertions(+), 45 deletions(-) create mode 100644 app/src/test/kotlin/com/subtracks/playback/ReplayGainSinkTest.kt diff --git a/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt b/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt index 37460c29..abc7b0b5 100644 --- a/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt +++ b/app/src/main/kotlin/com/subtracks/playback/Media3PlayerHandle.kt @@ -266,10 +266,6 @@ private fun Bundle.putGain( private fun Bundle.gain(key: String): Float? = if (containsKey(key)) getFloat(key) else null -internal fun ReplayGainController.applyItem(mediaItem: MediaItem?) { - setTrack(mediaItem.replayGain()) -} - internal fun queueMediaItem( item: QueueItem, local: String?, diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt index d808a4e8..9c937461 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt @@ -140,7 +140,6 @@ class PlaybackService : MediaSessionService() { reason: Int, ) { observeCurrentSong(mediaItem?.mediaId) - replayGain.applyItem(mediaItem) } }, ) diff --git a/app/src/main/kotlin/com/subtracks/playback/ReplayGainController.kt b/app/src/main/kotlin/com/subtracks/playback/ReplayGainController.kt index 6d254a38..cd14f58c 100644 --- a/app/src/main/kotlin/com/subtracks/playback/ReplayGainController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/ReplayGainController.kt @@ -1,23 +1,23 @@ package com.subtracks.playback +import androidx.media3.common.MediaItem import com.subtracks.data.prefs.ReplayGainMode internal class ReplayGainController( private val apply: (Float) -> Unit, ) { - // Written from the playback thread by ReplayGainProcessor.onFlush and from the app thread by - // the media item transition fallback. - @Volatile private var track = ReplayGain() - - @Volatile private var settings = ReplayGainSettings(ReplayGainMode.Track, 0f) - fun setTrack(value: ReplayGain?) { - track = value ?: ReplayGain() + // Called from the playback thread, by ReplayGainProcessor.onFlush. + @Synchronized + fun applyItem(item: MediaItem) { + track = item.replayGain() applyGain() } + // Called from the app thread, when the preferences change. + @Synchronized fun setSettings(value: ReplayGainSettings) { settings = value applyGain() diff --git a/app/src/main/kotlin/com/subtracks/playback/ReplayGainProcessor.kt b/app/src/main/kotlin/com/subtracks/playback/ReplayGainProcessor.kt index 111a1930..519a1930 100644 --- a/app/src/main/kotlin/com/subtracks/playback/ReplayGainProcessor.kt +++ b/app/src/main/kotlin/com/subtracks/playback/ReplayGainProcessor.kt @@ -13,7 +13,7 @@ import kotlin.math.roundToInt @OptIn(UnstableApi::class) internal class ReplayGainProcessor( - private val onItem: (MediaItem?) -> Unit = {}, + private val onItem: (MediaItem) -> Unit = {}, ) : BaseAudioProcessor() { @Volatile private var linearGain = 1f @@ -30,7 +30,9 @@ internal class ReplayGainProcessor( // Player.Listener.onMediaItemTransition, which the app thread receives afterwards. // See androidx/media#418. override fun onFlush(streamMetadata: StreamMetadata) { - onItem(streamMetadata.currentItem()) + // A flush with no period comes from a sink reset rather than an item boundary, so leave the + // gain alone rather than resetting it to unity. + streamMetadata.currentItem()?.let(onItem) } override fun queueInput(inputBuffer: ByteBuffer) { @@ -48,7 +50,9 @@ internal class ReplayGainProcessor( } else -> { - output.put(inputBuffer) + // An empty input would be the pipeline's own shared buffer, which cannot be put into + // itself. + if (inputBuffer.hasRemaining()) output.put(inputBuffer) } } inputBuffer.position(inputBuffer.limit()) diff --git a/app/src/test/kotlin/com/subtracks/playback/ReplayGainControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/ReplayGainControllerTest.kt index b569c2a3..817bea3b 100644 --- a/app/src/test/kotlin/com/subtracks/playback/ReplayGainControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/ReplayGainControllerTest.kt @@ -12,16 +12,16 @@ class ReplayGainControllerTest { private val controller = ReplayGainController { applied += it } @Test - fun appliesTheGainForTheCurrentTrack() { + fun appliesTheItemsGain() { controller.setSettings(ReplayGainSettings(ReplayGainMode.Track, 0f)) - controller.setTrack(ReplayGain(trackGain = -6.0206f)) + controller.applyItem(item("a", trackGain = -6.0206f)) assertEquals(0.5f, applied.last(), TOLERANCE) } @Test fun recomputesWhenTheSettingsChange() { - controller.setTrack(ReplayGain(trackGain = -6.0206f)) + controller.applyItem(item("a", trackGain = -6.0206f)) controller.setSettings(ReplayGainSettings(ReplayGainMode.Track, 6.0206f)) assertEquals(1f, applied.last(), TOLERANCE) @@ -29,7 +29,7 @@ class ReplayGainControllerTest { @Test fun switchingOffRestoresUnityGain() { - controller.setTrack(ReplayGain(trackGain = -6.0206f)) + controller.applyItem(item("a", trackGain = -6.0206f)) controller.setSettings(ReplayGainSettings(ReplayGainMode.Track, 0f)) assertEquals(0.5f, applied.last(), TOLERANCE) @@ -39,9 +39,12 @@ class ReplayGainControllerTest { } @Test - fun aTrackWithoutGainPlaysAtUnity() { + fun anItemWithoutGainPlaysAtUnity() { controller.setSettings(ReplayGainSettings(ReplayGainMode.Track, 0f)) - controller.setTrack(null) + controller.applyItem(item("a", trackGain = -6.0206f)) + assertEquals(0.5f, applied.last(), TOLERANCE) + + controller.applyItem(item("b", trackGain = null)) assertEquals(1f, applied.last(), TOLERANCE) } @@ -56,25 +59,6 @@ class ReplayGainControllerTest { assertEquals(0.5f, applied.last(), TOLERANCE) } - @Test - fun anItemWithoutGainPlaysAtUnity() { - controller.setSettings(ReplayGainSettings(ReplayGainMode.Track, 0f)) - - controller.applyItem(item("a", trackGain = null)) - - assertEquals(1f, applied.last(), TOLERANCE) - } - - @Test - fun noItemPlaysAtUnity() { - controller.setSettings(ReplayGainSettings(ReplayGainMode.Track, 0f)) - controller.applyItem(item("a", trackGain = 6.0206f)) - - controller.applyItem(null) - - assertEquals(1f, applied.last(), TOLERANCE) - } - private fun item( id: String, trackGain: Float?, diff --git a/app/src/test/kotlin/com/subtracks/playback/ReplayGainProcessorTest.kt b/app/src/test/kotlin/com/subtracks/playback/ReplayGainProcessorTest.kt index e752ed82..f5b761e4 100644 --- a/app/src/test/kotlin/com/subtracks/playback/ReplayGainProcessorTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/ReplayGainProcessorTest.kt @@ -93,7 +93,7 @@ class ReplayGainProcessorTest { @Test fun aFlushReportsTheItemBeingFlushed() { - val reported = mutableListOf() + val reported = mutableListOf() val item = item("a", trackGain = -6.0206f) ReplayGainProcessor { reported += it }.flush(streamMetadata(item)) @@ -102,12 +102,18 @@ class ReplayGainProcessorTest { } @Test - fun aFlushWithoutAPeriodReportsNoItem() { - val reported = mutableListOf() + fun aFlushWithoutAPeriodLeavesTheGainAlone() { + lateinit var processor: ReplayGainProcessor + val controller = ReplayGainController { processor.setLinearGain(it) } + processor = ReplayGainProcessor { controller.applyItem(it) } + controller.setSettings(ReplayGainSettings(ReplayGainMode.Track, 0f)) + processor.configure(AudioFormat(SAMPLE_RATE, CHANNELS, C.ENCODING_PCM_16BIT)) - ReplayGainProcessor { reported += it }.flush(StreamMetadata.DEFAULT) + processor.flush(streamMetadata(item("a", trackGain = -6.0206f))) + processor.flush(StreamMetadata.DEFAULT) + processor.queueInput(sixteenBit(1000)) - assertEquals(listOf(null), reported) + assertEquals(listOf(500), readShorts(processor.getOutput())) } @Test diff --git a/app/src/test/kotlin/com/subtracks/playback/ReplayGainSinkTest.kt b/app/src/test/kotlin/com/subtracks/playback/ReplayGainSinkTest.kt new file mode 100644 index 00000000..469e29d7 --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/playback/ReplayGainSinkTest.kt @@ -0,0 +1,66 @@ +package com.subtracks.playback + +import android.content.Context +import androidx.annotation.OptIn +import androidx.media3.common.C +import androidx.media3.common.Format +import androidx.media3.common.MediaItem +import androidx.media3.common.MimeTypes +import androidx.media3.common.util.UnstableApi +import androidx.media3.exoplayer.audio.AudioSink +import androidx.media3.exoplayer.source.MediaSource.MediaPeriodId +import androidx.media3.exoplayer.source.SinglePeriodTimeline +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import java.nio.ByteBuffer +import java.nio.ByteOrder + +// Pins the media3 behaviour the whole feature rests on: DefaultAudioSink hands the processor the +// timeline and the period being configured, on every configuration, which is what lets the gain be +// set before a new item's buffers. See androidx/media#418. +@OptIn(UnstableApi::class) +@RunWith(AndroidJUnit4::class) +class ReplayGainSinkTest { + @Test + fun theSinkHandsTheItemBeingConfiguredToTheProcessor() { + val context = ApplicationProvider.getApplicationContext() + val reported = mutableListOf() + val processor = ReplayGainProcessor { reported += it } + val sink = ReplayGainRenderersFactory(context, processor).buildAudioSink(context, false, false) + + val item = MediaItem.Builder().setMediaId("b").build() + val timeline = SinglePeriodTimeline(DURATION_US, false, false, false, null, item) + sink.configure( + AudioSink.AudioSinkConfig + .Builder(pcmFormat()) + .setTimeline(timeline) + .setMediaPeriodId(MediaPeriodId(timeline.getUidOfPeriod(0))) + .build(), + ) + sink.handleBuffer(sixteenBit(1000), 0, 1) + + assertEquals(listOf(item), reported) + } + + private fun pcmFormat() = + Format + .Builder() + .setSampleMimeType(MimeTypes.AUDIO_RAW) + .setPcmEncoding(C.ENCODING_PCM_16BIT) + .setChannelCount(2) + .setSampleRate(44_100) + .build() + + private fun sixteenBit(vararg samples: Int): ByteBuffer = + ByteBuffer.allocate(samples.size * 2).order(ByteOrder.nativeOrder()).apply { + samples.forEach { putShort(it.toShort()) } + flip() + } + + private companion object { + const val DURATION_US = 1_000_000L + } +}