Skip to content

Fix some dodgy math #3271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nift4 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix some dodgy math #3271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/PcmAudioUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public static ByteBuffer rampUpVolume(
public static int readAs32BitIntPcm(ByteBuffer buffer, @C.Encoding int pcmEncoding) {
switch (pcmEncoding) {
case C.ENCODING_PCM_8BIT:
return (buffer.get() & 0xFF) << 24;
return ((buffer.get() & 0xFF) - 128) << 24;
case C.ENCODING_PCM_16BIT:
return ((buffer.get() & 0xFF) << 16) | ((buffer.get() & 0xFF) << 24);
case C.ENCODING_PCM_16BIT_BIG_ENDIAN:
Expand Down Expand Up @@ -161,7 +161,7 @@ public static void write32BitIntPcm(
buffer.put((byte) (pcm32bit >> 24));
return;
case C.ENCODING_PCM_8BIT:
buffer.put((byte) (pcm32bit >> 24));
buffer.put((byte) ((pcm32bit >> 24) + 128));
return;
case C.ENCODING_PCM_32BIT_BIG_ENDIAN:
buffer.put((byte) (pcm32bit >> 24));
Expand Down
17 changes: 13 additions & 4 deletions ...xoplayer/src/main/java/androidx/media3/exoplayer/audio/SilenceSkippingAudioProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public final class SilenceSkippingAudioProcessor extends BaseAudioProcessor {
* Default minimum duration of audio that must be below {@code silenceThresholdLevel} before
* silence starts being trimmed. Specified in microseconds.
*/
public static final long DEFAULT_MINIMUM_SILENCE_DURATION_US = 100_000;
public static final long DEFAULT_MINIMUM_SILENCE_DURATION_US = 25_000;

/**
* Default maximum silence to keep in microseconds. This maximum is applied after {@code
Expand Down Expand Up @@ -309,7 +309,9 @@ public void onFlush(StreamMetadata streamMetadata) {
bytesPerFrame = inputAudioFormat.channelCount * 2;
// Divide by 2 to allow the buffer to be split into two bytesPerFrame aligned parts.
int maybeSilenceBufferSize =
alignToBytePerFrameBoundary(durationUsToFrames(minimumSilenceDurationUs) / 2) * 2;
alignToBytePerFrameBoundary(
durationUsToFrames(minimumSilenceDurationUs) * bytesPerFrame / 2)
* 2;
if (maybeSilenceBuffer.length != maybeSilenceBufferSize) {
maybeSilenceBuffer = new byte[maybeSilenceBufferSize];
contiguousOutputBuffer = new byte[maybeSilenceBufferSize];
Expand Down Expand Up @@ -647,6 +649,7 @@ private void modifyVolume(byte[] sampleBuffer, int size, @VolumeChangeType int v
return;
}

int lastFrameIdx = (size / bytesPerFrame) - 1;
for (int idx = 0; idx < size; idx += 2) {
byte mostSignificantByte = sampleBuffer[idx + 1];
byte leastSignificantByte = sampleBuffer[idx];
Expand All @@ -655,10 +658,10 @@ private void modifyVolume(byte[] sampleBuffer, int size, @VolumeChangeType int v
int volumeModificationPercentage;
if (volumeChangeType == FADE_OUT) {
volumeModificationPercentage =
calculateFadeOutPercentage(/* value= */ idx, /* max= */ size - 1);
calculateFadeOutPercentage(/* value= */ idx / bytesPerFrame, /* max= */ lastFrameIdx);
} else if (volumeChangeType == FADE_IN) {
volumeModificationPercentage =
calculateFadeInPercentage(/* value= */ idx, /* max= */ size - 1);
calculateFadeInPercentage(/* value= */ idx / bytesPerFrame, /* max= */ lastFrameIdx);
} else {
volumeModificationPercentage = minVolumeToKeepPercentageWhenMuting;
}
Expand All @@ -669,12 +672,18 @@ private void modifyVolume(byte[] sampleBuffer, int size, @VolumeChangeType int v
}

private int calculateFadeOutPercentage(int value, int max) {
if (max == 0) {
return 0;
}
return ((minVolumeToKeepPercentageWhenMuting - 100) * ((AVOID_TRUNCATION_FACTOR * value) / max))
/ AVOID_TRUNCATION_FACTOR
+ 100;
}

private int calculateFadeInPercentage(int value, int max) {
if (max == 0) {
return 0;
}
return (minVolumeToKeepPercentageWhenMuting
+ ((100 - minVolumeToKeepPercentageWhenMuting) * (AVOID_TRUNCATION_FACTOR * value) / max)
/ AVOID_TRUNCATION_FACTOR);
Expand Down
4 changes: 2 additions & 2 deletions libraries/exoplayer/src/test/java/androidx/media3/exoplayer/audio/PcmAudioUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public final class PcmAudioUtilTest {
@Test
public void readAs32BitIntPcm_read8Bit_returnsExpectedValues() {
ByteBuffer buffer = ByteBuffer.allocate(5);
buffer.put(hexToBytes("80" + "AB" + "00" + "12" + "7F"));
buffer.put(hexToBytes("00" + "2B" + "80" + "92" + "FF"));
buffer.flip();

assertThat(PcmAudioUtil.readAs32BitIntPcm(buffer, C.ENCODING_PCM_8BIT))
Expand Down Expand Up @@ -197,7 +197,7 @@ public void write32BitIntPcm_write8Bit_returnsExpectedValues() {
PcmAudioUtil.write32BitIntPcm(buffer, Integer.MAX_VALUE, C.ENCODING_PCM_8BIT);
buffer.flip();

assertThat(byteBufferToHex(buffer)).isEqualTo("80" + "AB" + "00" + "12" + "7F");
assertThat(byteBufferToHex(buffer)).isEqualTo("00" + "2B" + "80" + "92" + "FF");
}

@Test
Expand Down
27 changes: 19 additions & 8 deletions ...ayer/src/test/java/androidx/media3/exoplayer/audio/SilenceSkippingAudioProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ public void skipInNoisySignalWithShortSilences_skipsNothing() throws Exception {
// Given a signal with only noise.
InputBufferProvider inputBufferProvider =
getInputBufferProviderForAlternatingSilenceAndNoise(
/* silenceDurationMs= */ 30,
TEST_SIGNAL_NOISE_DURATION_MS - 30,
/* silenceDurationMs= */ 24,
TEST_SIGNAL_NOISE_DURATION_MS - 24,
TEST_SIGNAL_FRAME_COUNT);

// When processing the entire signal.
Expand Down Expand Up @@ -172,7 +172,7 @@ public void skipInAlternatingTestSignal_hasCorrectOutputAndSkippedFrameCounts()
process(silenceSkippingAudioProcessor, inputBufferProvider, INPUT_BUFFER_SIZE);

// The output has 50000 frames of noise, plus 50 * 0.2 * 1000 padding (plus rounding errors).
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 500L, 60000L + 500L));
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 1500L, 60000L + 1500L));
assertThat(silenceSkippingAudioProcessor.getSkippedFrames())
.isEqualTo(TEST_SIGNAL_FRAME_COUNT - totalOutputFrames);
}
Expand Down Expand Up @@ -201,7 +201,7 @@ public void skipInAlternatingTestSignal_hasCorrectOutputAndSkippedFrameCounts()
process(silenceSkippingAudioProcessor, inputBufferProvider, INPUT_BUFFER_SIZE);

// The output has 50000 frames of noise, plus 50 * 0.2 * 1000 padding (plus rounding errors).
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 500L, 60000L + 500L));
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 1500L, 60000L + 1500L));
assertThat(silenceSkippingAudioProcessor.getSkippedFrames())
.isEqualTo(TEST_SIGNAL_FRAME_COUNT - totalOutputFrames);
}
Expand All @@ -227,7 +227,7 @@ public void skipWithSmallerInputBufferSize_hasCorrectOutputAndSkippedFrameCounts
process(silenceSkippingAudioProcessor, inputBufferProvider, /* inputBufferSize= */ 80);

// The output has 50000 frames of noise, plus 50 * 0.2 * 1000 padding (plus rounding errors).
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 500L, 60000L + 500L));
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 1500L, 60000L + 1500L));
assertThat(silenceSkippingAudioProcessor.getSkippedFrames())
.isEqualTo(TEST_SIGNAL_FRAME_COUNT - totalOutputFrames);
}
Expand All @@ -253,7 +253,7 @@ public void skipWithLargerInputBufferSize_hasCorrectOutputAndSkippedFrameCounts(
process(silenceSkippingAudioProcessor, inputBufferProvider, /* inputBufferSize= */ 120);

// The output has 50000 frames of noise, plus 50 * 0.2 * 1000 padding (plus rounding errors).
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 500L, 60000L + 500L));
assertThat(totalOutputFrames).isIn(Range.closed(60000L - 1500L, 60000L + 1500L));
assertThat(silenceSkippingAudioProcessor.getSkippedFrames())
.isEqualTo(TEST_SIGNAL_FRAME_COUNT - totalOutputFrames);
}
Expand Down Expand Up @@ -283,7 +283,7 @@ public void customSilenceRetentionValue_hasCorrectOutputAndSkippedFrameCounts()
process(silenceSkippingAudioProcessor, inputBufferProvider, /* inputBufferSize= */ 120);

// The output has 50000 frames of noise, plus 50 * 0.05 * 1000 padding (plus rounding errors).
assertThat(totalOutputFrames).isIn(Range.closed(52500L - 500L, 52500L + 500L));
assertThat(totalOutputFrames).isIn(Range.closed(52500L - 1500L, 52500L + 1500L));
assertThat(silenceSkippingAudioProcessor.getSkippedFrames())
.isEqualTo(TEST_SIGNAL_FRAME_COUNT - totalOutputFrames);
}
Expand Down Expand Up @@ -347,16 +347,21 @@ private static InputBufferProvider getInputBufferProviderForAlternatingSilenceAn
int silenceDurationMs, int noiseDurationMs, int totalFrameCount) {
int sampleRate = AUDIO_FORMAT.sampleRate;
int channelCount = AUDIO_FORMAT.channelCount;
int lastSize = 0;
Pcm16BitAudioBuilder audioBuilder = new Pcm16BitAudioBuilder(channelCount, totalFrameCount);
while (!audioBuilder.isFull()) {
int silenceDurationFrames = (silenceDurationMs * sampleRate) / 1000;
// Append stereo silence.
audioBuilder.appendFrames(
/* count= */ silenceDurationFrames, /* channelLevels...= */ (short) 0, (short) 0);
lastSize += silenceDurationFrames * channelCount * 2;
assertThat(audioBuilder.getSize()).isEqualTo(lastSize);
int noiseDurationFrames = (noiseDurationMs * sampleRate) / 1000;
// Append stereo noise.
audioBuilder.appendFrames(
/* count= */ noiseDurationFrames, /* channelLevels...= */ MAX_VALUE, MAX_VALUE);
lastSize += noiseDurationFrames * channelCount * 2;
assertThat(audioBuilder.getSize()).isEqualTo(lastSize);
}
return new InputBufferProvider(audioBuilder.build());
}
Expand Down Expand Up @@ -410,13 +415,19 @@ public Pcm16BitAudioBuilder(int channelCount, int frameCount) {
public void appendFrames(int count, short... channelLevels) {
checkState(!built);
checkState(channelLevels.length == channelCount);
for (int i = 0; i < count; i += channelCount) {
for (int i = 0; i < count; i++) {
for (short channelLevel : channelLevels) {
buffer.put(channelLevel);
}
}
}

/** Returns how many bytes the buffer contains so far. */
public int getSize() {
checkState(!built);
return buffer.position() * 2;
}

/** Returns whether the buffer is full. */
public boolean isFull() {
checkState(!built);
Expand Down