From c377a34a5aa4475c025ef61075c9962187d84a5c Mon Sep 17 00:00:00 2001 From: ibaker Date: Tue, 10 Dec 2024 10:28:26 -0800 Subject: [PATCH] Don't call `onAudioPositionAdvancing` if `AudioTrack` is paused `AudioTrackPositionTracker.pause` "re-arms" the event, to ensure it fires again when playback resumes. However the `AudioTrack` position advances by about 100ms **after** `AudioTrack.pause()`, which consistently causes the event to fire immediately after pausing (and then **not** after a subsequent resumption). This change checks whether the `AudioTrack` is paused before firing the event, to avoid this spurious trigger. PiperOrigin-RevId: 704759929 --- RELEASENOTES.md | 2 + .../exoplayer/AudioPositionAdvancingTest.java | 145 ++++++++++++++++++ .../audio/AudioTrackPositionTracker.java | 7 +- 3 files changed, 152 insertions(+), 2 deletions(-) create mode 100644 libraries/exoplayer/src/androidTest/java/androidx/media3/exoplayer/AudioPositionAdvancingTest.java diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 93cb82b3b8..d552dc269c 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -39,6 +39,8 @@ ([#1904](https://github.com/androidx/media/issues/1904)). * DataSource: * Audio: + * Fix `onAudioPositionAdvancing` to be called when playback resumes + (previously it was called when playback was paused). * Video: * Rollback of using `MediaCodecAdapter` supplied pixel aspect ratio values when provided while processing `onOutputFormatChanged` diff --git a/libraries/exoplayer/src/androidTest/java/androidx/media3/exoplayer/AudioPositionAdvancingTest.java b/libraries/exoplayer/src/androidTest/java/androidx/media3/exoplayer/AudioPositionAdvancingTest.java new file mode 100644 index 0000000000..b703463b6e --- /dev/null +++ b/libraries/exoplayer/src/androidTest/java/androidx/media3/exoplayer/AudioPositionAdvancingTest.java @@ -0,0 +1,145 @@ +/* + * Copyright 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package androidx.media3.exoplayer; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static com.google.common.truth.Truth.assertThat; + +import androidx.media3.common.C; +import androidx.media3.common.MediaItem; +import androidx.media3.common.util.ConditionVariable; +import androidx.media3.exoplayer.analytics.AnalyticsListener; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.common.collect.Iterables; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * Tests for the {@link AnalyticsListener#onAudioPositionAdvancing(AnalyticsListener.EventTime, + * long)} callback. + */ +// Deliberately using System.currentTimeMillis for consistency with onAudioPositionAdvancing. +@SuppressWarnings("NowMillisWithoutTimeSource") +@RunWith(AndroidJUnit4.class) +public class AudioPositionAdvancingTest { + + private static final int TIMEOUT_MS = 500; + + // Regression test for b/378871275 + @Test + public void calledWhenPlaybackResumesNotPaused() throws Exception { + MediaItem mediaItem = MediaItem.fromUri("asset:///media/mp3/bear-id3.mp3"); + List playoutStartSystemTimes = Collections.synchronizedList(new ArrayList<>()); + ConditionVariable onAdvancingCalled = new ConditionVariable(); + AnalyticsListener analyticsListener = + new AnalyticsListener() { + @Override + public void onAudioPositionAdvancing(EventTime eventTime, long playoutStartSystemTimeMs) { + playoutStartSystemTimes.add(playoutStartSystemTimeMs); + onAdvancingCalled.open(); + } + }; + AtomicReference player = new AtomicReference<>(); + AtomicLong playbackTriggeredSystemTimeMs = new AtomicLong(C.TIME_UNSET); + getInstrumentation() + .runOnMainSync( + () -> { + player.set(new ExoPlayer.Builder(getInstrumentation().getContext()).build()); + player.get().addAnalyticsListener(analyticsListener); + player.get().setMediaItem(mediaItem); + player.get().prepare(); + playbackTriggeredSystemTimeMs.set(System.currentTimeMillis()); + player.get().play(); + }); + + assertThat(onAdvancingCalled.block(TIMEOUT_MS)).isTrue(); + long currentTimeMs = System.currentTimeMillis(); + long playoutStartSystemTimeMs = Iterables.getOnlyElement(playoutStartSystemTimes); + assertThat(playoutStartSystemTimeMs).isAtLeast(playbackTriggeredSystemTimeMs.get()); + assertThat(playoutStartSystemTimeMs).isAtMost(currentTimeMs); + + onAdvancingCalled.close(); + getInstrumentation().runOnMainSync(() -> player.get().pause()); + + // Expect the callback to *not* be called. + assertThat(onAdvancingCalled.block(50)).isFalse(); + + getInstrumentation() + .runOnMainSync( + () -> { + playbackTriggeredSystemTimeMs.set(System.currentTimeMillis()); + player.get().play(); + }); + assertThat(onAdvancingCalled.block(TIMEOUT_MS)).isTrue(); + currentTimeMs = System.currentTimeMillis(); + playoutStartSystemTimeMs = Iterables.getLast(playoutStartSystemTimes); + assertThat(playoutStartSystemTimeMs).isAtLeast(playbackTriggeredSystemTimeMs.get()); + assertThat(playoutStartSystemTimeMs).isAtMost(currentTimeMs); + + getInstrumentation().runOnMainSync(() -> player.get().release()); + } + + @Test + public void pauseThenPlayInSameLooperIteration() throws Exception { + MediaItem mediaItem = MediaItem.fromUri("asset:///media/mp3/bear-id3.mp3"); + List playoutStartSystemTimes = Collections.synchronizedList(new ArrayList<>()); + ConditionVariable onAdvancingCalled = new ConditionVariable(); + AnalyticsListener analyticsListener = + new AnalyticsListener() { + @Override + public void onAudioPositionAdvancing(EventTime eventTime, long playoutStartSystemTimeMs) { + playoutStartSystemTimes.add(playoutStartSystemTimeMs); + onAdvancingCalled.open(); + } + }; + AtomicReference player = new AtomicReference<>(); + getInstrumentation() + .runOnMainSync( + () -> { + player.set(new ExoPlayer.Builder(getInstrumentation().getContext()).build()); + player.get().addAnalyticsListener(analyticsListener); + player.get().setMediaItem(mediaItem); + player.get().prepare(); + player.get().play(); + }); + + assertThat(onAdvancingCalled.block(TIMEOUT_MS)).isTrue(); + onAdvancingCalled.close(); + + AtomicLong playbackTriggeredSystemTimeMs = new AtomicLong(C.TIME_UNSET); + getInstrumentation() + .runOnMainSync( + () -> { + player.get().pause(); + playbackTriggeredSystemTimeMs.set(System.currentTimeMillis()); + player.get().play(); + }); + + assertThat(onAdvancingCalled.block(TIMEOUT_MS)).isTrue(); + long currentTimeMs = System.currentTimeMillis(); + assertThat(playoutStartSystemTimes).hasSize(2); + long playoutStartSystemTimeMs = playoutStartSystemTimes.get(1); + assertThat(playoutStartSystemTimeMs).isAtLeast(playbackTriggeredSystemTimeMs.get()); + assertThat(playoutStartSystemTimeMs).isAtMost(currentTimeMs); + + getInstrumentation().runOnMainSync(() -> player.get().release()); + } +} diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioTrackPositionTracker.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioTrackPositionTracker.java index e2b372591d..8ce02f9987 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioTrackPositionTracker.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/audio/AudioTrackPositionTracker.java @@ -285,7 +285,8 @@ import java.lang.reflect.Method; } public long getCurrentPositionUs(boolean sourceEnded) { - if (checkNotNull(this.audioTrack).getPlayState() == PLAYSTATE_PLAYING) { + AudioTrack audioTrack = checkNotNull(this.audioTrack); + if (audioTrack.getPlayState() == PLAYSTATE_PLAYING) { maybeSampleSyncParams(); } @@ -340,7 +341,9 @@ import java.lang.reflect.Method; positionUs /= 1000; } - if (!notifiedPositionIncreasing && positionUs > lastPositionUs) { + if (!notifiedPositionIncreasing + && positionUs > lastPositionUs + && audioTrack.getPlayState() == AudioTrack.PLAYSTATE_PLAYING) { notifiedPositionIncreasing = true; long mediaDurationSinceLastPositionUs = Util.usToMs(positionUs - lastPositionUs); long playoutDurationSinceLastPositionUs =