From 646a6352a2c6fb170cee2ae2d8e8d1a24eb7aed8 Mon Sep 17 00:00:00 2001 From: ivanbuper Date: Tue, 19 Nov 2024 06:59:27 -0800 Subject: [PATCH] Remove bypass of SonicAudioProcessor in SpeedChangingAudioProcessor SpeedChangingAudioProcessor is redundantly bypassing SonicAudioProcessor when the speed is set to 1f. SpeedChangingAudioProcessor should not make assumptions about the underlying audio processing and moreover should not be bypassing any AudioProcessor based on parameter values. Default parameter values are a valid state for an active AudioProcessor. Sonic already handles the "default case" state by just copying the input buffer onto the output buffer. This CL also simplifies SonicAudioProcessor, which would mark itself as inactive when configured with a valid set of default parameters. The API contract for `isActive()` makes no mention about parameter state, which makes changes in `isActive()` after applying new valid parameters quite unintuitive. PiperOrigin-RevId: 698000500 --- RELEASENOTES.md | 2 + .../common/audio/SonicAudioProcessor.java | 34 +++++++-- .../audio/SpeedChangingAudioProcessor.java | 70 +++++++------------ .../SynchronizedSonicAudioProcessor.java | 4 +- .../common/audio/SonicAudioProcessorTest.java | 10 ++- 5 files changed, 67 insertions(+), 53 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index f3f1e861a3..358276cef2 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -28,6 +28,8 @@ * Extractors: * DataSource: * Audio: + * Do not bypass `SonicAudioProcessor` when `SpeedChangingAudioProcessor` + is configured with default parameters. * Video: * Text: * Metadata: diff --git a/libraries/common/src/main/java/androidx/media3/common/audio/SonicAudioProcessor.java b/libraries/common/src/main/java/androidx/media3/common/audio/SonicAudioProcessor.java index 030b1e3dff..a26522505d 100644 --- a/libraries/common/src/main/java/androidx/media3/common/audio/SonicAudioProcessor.java +++ b/libraries/common/src/main/java/androidx/media3/common/audio/SonicAudioProcessor.java @@ -15,8 +15,11 @@ */ package androidx.media3.common.audio; +import static androidx.media3.common.util.Assertions.checkArgument; import static androidx.media3.common.util.Assertions.checkNotNull; +import static java.lang.Math.abs; +import androidx.annotation.FloatRange; import androidx.annotation.Nullable; import androidx.media3.common.C; import androidx.media3.common.Format; @@ -44,6 +47,8 @@ public class SonicAudioProcessor implements AudioProcessor { */ private static final int MIN_BYTES_FOR_DURATION_SCALING_CALCULATION = 1024; + private final boolean shouldBeActiveWithDefaultParameters; + private int pendingOutputSampleRate; private float speed; private float pitch; @@ -64,6 +69,17 @@ public class SonicAudioProcessor implements AudioProcessor { /** Creates a new Sonic audio processor. */ public SonicAudioProcessor() { + this(/* keepActiveWithDefaultParameters= */ false); + } + + /** + * Creates a new instance of {@link SonicAudioProcessor}. + * + *

If {@code keepActiveWithDefaultParameters} is set to {@code true}, then {@link #isActive()} + * returns {@code true} when parameters have been configured to default values that result in + * no-op processing. + */ + /* package */ SonicAudioProcessor(boolean keepActiveWithDefaultParameters) { speed = 1f; pitch = 1f; pendingInputAudioFormat = AudioFormat.NOT_SET; @@ -74,6 +90,7 @@ public class SonicAudioProcessor implements AudioProcessor { shortBuffer = buffer.asShortBuffer(); outputBuffer = EMPTY_BUFFER; pendingOutputSampleRate = SAMPLE_RATE_NO_CHANGE; + shouldBeActiveWithDefaultParameters = keepActiveWithDefaultParameters; } /** @@ -83,7 +100,8 @@ public class SonicAudioProcessor implements AudioProcessor { * * @param speed The target factor by which playback should be sped up. */ - public final void setSpeed(float speed) { + public final void setSpeed(@FloatRange(from = 0f, fromInclusive = false) float speed) { + checkArgument(speed > 0f); if (this.speed != speed) { this.speed = speed; pendingSonicRecreation = true; @@ -97,7 +115,8 @@ public class SonicAudioProcessor implements AudioProcessor { * * @param pitch The target pitch. */ - public final void setPitch(float pitch) { + public final void setPitch(@FloatRange(from = 0f, fromInclusive = false) float pitch) { + checkArgument(pitch > 0f); if (this.pitch != pitch) { this.pitch = pitch; pendingSonicRecreation = true; @@ -113,6 +132,7 @@ public class SonicAudioProcessor implements AudioProcessor { * @see #configure(AudioFormat) */ public final void setOutputSampleRateHz(int sampleRateHz) { + checkArgument(sampleRateHz == SAMPLE_RATE_NO_CHANGE || sampleRateHz > 0); pendingOutputSampleRate = sampleRateHz; } @@ -196,9 +216,13 @@ public class SonicAudioProcessor implements AudioProcessor { @Override public final boolean isActive() { return pendingOutputAudioFormat.sampleRate != Format.NO_VALUE - && (Math.abs(speed - 1f) >= CLOSE_THRESHOLD - || Math.abs(pitch - 1f) >= CLOSE_THRESHOLD - || pendingOutputAudioFormat.sampleRate != pendingInputAudioFormat.sampleRate); + && (shouldBeActiveWithDefaultParameters || !areParametersSetToDefaultValues()); + } + + private boolean areParametersSetToDefaultValues() { + return abs(speed - 1f) < CLOSE_THRESHOLD + && abs(pitch - 1f) < CLOSE_THRESHOLD + && pendingOutputAudioFormat.sampleRate == pendingInputAudioFormat.sampleRate; } @Override diff --git a/libraries/common/src/main/java/androidx/media3/common/audio/SpeedChangingAudioProcessor.java b/libraries/common/src/main/java/androidx/media3/common/audio/SpeedChangingAudioProcessor.java index be20fa8024..e2e1eb8e71 100644 --- a/libraries/common/src/main/java/androidx/media3/common/audio/SpeedChangingAudioProcessor.java +++ b/libraries/common/src/main/java/androidx/media3/common/audio/SpeedChangingAudioProcessor.java @@ -24,6 +24,7 @@ import static androidx.media3.common.util.Util.sampleCountToDurationUs; import static java.lang.Math.min; import static java.lang.Math.round; +import android.annotation.SuppressLint; import androidx.annotation.GuardedBy; import androidx.annotation.IntRange; import androidx.media3.common.C; @@ -99,7 +100,8 @@ public final class SpeedChangingAudioProcessor extends BaseAudioProcessor { public SpeedChangingAudioProcessor(SpeedProvider speedProvider) { this.speedProvider = speedProvider; lock = new Object(); - sonicAudioProcessor = new SynchronizedSonicAudioProcessor(lock); + sonicAudioProcessor = + new SynchronizedSonicAudioProcessor(lock, /* keepActiveWithDefaultParameters= */ true); pendingCallbackInputTimesUs = new LongArrayQueue(); pendingCallbacks = new ArrayDeque<>(); speedAdjustedTimeAsyncInputTimeUs = C.TIME_UNSET; @@ -174,19 +176,11 @@ public final class SpeedChangingAudioProcessor extends BaseAudioProcessor { } long startPosition = inputBuffer.position(); - if (isUsingSonic()) { - sonicAudioProcessor.queueInput(inputBuffer); - if (bytesToNextSpeedChange != C.LENGTH_UNSET - && (inputBuffer.position() - startPosition) == bytesToNextSpeedChange) { - sonicAudioProcessor.queueEndOfStream(); - endOfStreamQueuedToSonic = true; - } - } else { - ByteBuffer buffer = replaceOutputBuffer(/* size= */ inputBuffer.remaining()); - if (inputBuffer.hasRemaining()) { - buffer.put(inputBuffer); - } - buffer.flip(); + sonicAudioProcessor.queueInput(inputBuffer); + if (bytesToNextSpeedChange != C.LENGTH_UNSET + && (inputBuffer.position() - startPosition) == bytesToNextSpeedChange) { + sonicAudioProcessor.queueEndOfStream(); + endOfStreamQueuedToSonic = true; } long bytesRead = inputBuffer.position() - startPosition; checkState( @@ -204,9 +198,11 @@ public final class SpeedChangingAudioProcessor extends BaseAudioProcessor { } } + // Not using BaseAudioProcessor's buffers. + @SuppressLint("MissingSuperCall") @Override public ByteBuffer getOutput() { - ByteBuffer output = isUsingSonic() ? sonicAudioProcessor.getOutput() : super.getOutput(); + ByteBuffer output = sonicAudioProcessor.getOutput(); processPendingCallbacks(); return output; } @@ -351,10 +347,8 @@ public final class SpeedChangingAudioProcessor extends BaseAudioProcessor { if (newSpeed != currentSpeed) { updateSpeedChangeArrays(timeUs); currentSpeed = newSpeed; - if (isUsingSonic()) { - sonicAudioProcessor.setSpeed(newSpeed); - sonicAudioProcessor.setPitch(newSpeed); - } + sonicAudioProcessor.setSpeed(newSpeed); + sonicAudioProcessor.setPitch(newSpeed); // Invalidate any previously created buffers in SonicAudioProcessor and the base class. sonicAudioProcessor.flush(); endOfStreamQueuedToSonic = false; @@ -378,39 +372,25 @@ public final class SpeedChangingAudioProcessor extends BaseAudioProcessor { } private long getPlayoutDurationUsAtCurrentSpeed(long mediaDurationUs) { - return isUsingSonic() - ? sonicAudioProcessor.getPlayoutDuration(mediaDurationUs) - : mediaDurationUs; + return sonicAudioProcessor.getPlayoutDuration(mediaDurationUs); } private long getMediaDurationUsAtCurrentSpeed(long playoutDurationUs) { - return isUsingSonic() - ? sonicAudioProcessor.getMediaDuration(playoutDurationUs) - : playoutDurationUs; + return sonicAudioProcessor.getMediaDuration(playoutDurationUs); } private void updateLastProcessedInputTime() { synchronized (lock) { - if (isUsingSonic()) { - // TODO - b/320242819: Investigate whether bytesRead can be used here rather than - // sonicAudioProcessor.getProcessedInputBytes(). - long currentProcessedInputDurationUs = - Util.scaleLargeTimestamp( - /* timestamp= */ sonicAudioProcessor.getProcessedInputBytes(), - /* multiplier= */ C.MICROS_PER_SECOND, - /* divisor= */ (long) inputAudioFormat.sampleRate * inputAudioFormat.bytesPerFrame); - lastProcessedInputTimeUs = - inputSegmentStartTimesUs.get(inputSegmentStartTimesUs.size() - 1) - + currentProcessedInputDurationUs; - } else { - lastProcessedInputTimeUs = sampleCountToDurationUs(framesRead, inputAudioFormat.sampleRate); - } - } - } - - private boolean isUsingSonic() { - synchronized (lock) { - return currentSpeed != 1f; + // TODO - b/320242819: Investigate whether bytesRead can be used here rather than + // sonicAudioProcessor.getProcessedInputBytes(). + long currentProcessedInputDurationUs = + Util.scaleLargeTimestamp( + /* timestamp= */ sonicAudioProcessor.getProcessedInputBytes(), + /* multiplier= */ C.MICROS_PER_SECOND, + /* divisor= */ (long) inputAudioFormat.sampleRate * inputAudioFormat.bytesPerFrame); + lastProcessedInputTimeUs = + inputSegmentStartTimesUs.get(inputSegmentStartTimesUs.size() - 1) + + currentProcessedInputDurationUs; } } diff --git a/libraries/common/src/main/java/androidx/media3/common/audio/SynchronizedSonicAudioProcessor.java b/libraries/common/src/main/java/androidx/media3/common/audio/SynchronizedSonicAudioProcessor.java index cd447b5882..78caf65281 100644 --- a/libraries/common/src/main/java/androidx/media3/common/audio/SynchronizedSonicAudioProcessor.java +++ b/libraries/common/src/main/java/androidx/media3/common/audio/SynchronizedSonicAudioProcessor.java @@ -26,9 +26,9 @@ import java.nio.ByteBuffer; private final Object lock; private final SonicAudioProcessor sonicAudioProcessor; - public SynchronizedSonicAudioProcessor(Object lock) { + public SynchronizedSonicAudioProcessor(Object lock, boolean keepActiveWithDefaultParameters) { this.lock = lock; - sonicAudioProcessor = new SonicAudioProcessor(); + sonicAudioProcessor = new SonicAudioProcessor(keepActiveWithDefaultParameters); } public final void setSpeed(float speed) { diff --git a/libraries/common/src/test/java/androidx/media3/common/audio/SonicAudioProcessorTest.java b/libraries/common/src/test/java/androidx/media3/common/audio/SonicAudioProcessorTest.java index d3a844a849..b5581a3a4b 100644 --- a/libraries/common/src/test/java/androidx/media3/common/audio/SonicAudioProcessorTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/audio/SonicAudioProcessorTest.java @@ -86,11 +86,19 @@ public final class SonicAudioProcessorTest { } @Test - public void isNotActiveWithNoChange() throws Exception { + public void isActive_withDefaultParameters_returnsFalse() throws Exception { sonicAudioProcessor.configure(AUDIO_FORMAT_44100_HZ); assertThat(sonicAudioProcessor.isActive()).isFalse(); } + @Test + public void isActive_keepActiveWithDefaultParameters_returnsTrue() throws Exception { + SonicAudioProcessor processor = + new SonicAudioProcessor(/* keepActiveWithDefaultParameters= */ true); + processor.configure(AUDIO_FORMAT_44100_HZ); + assertThat(processor.isActive()).isTrue(); + } + @Test public void doesNotSupportNon16BitInput() throws Exception { try {