diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 5c2225fb8c0..3270bfa6d63 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -8,6 +8,9 @@ * Add `void mute()` and `void unmute()` methods to Player that preserve and consequently restore Player's volume before and after setting it to zero. + * Fix `ForwardingPlayer` listener handling when the underlying delegate + player uses reference equality for comparing listener instances + ([#2675](https://github.com/androidx/media/issues/2675)). * ExoPlayer: * Add a stuck buffering detection that triggers a `StuckPlayerException` player error after 10 minutes of `STATE_BUFFERING` while trying to play diff --git a/libraries/common/src/main/java/androidx/media3/common/ForwardingPlayer.java b/libraries/common/src/main/java/androidx/media3/common/ForwardingPlayer.java index e0de4d7429b..3f217b64dc2 100644 --- a/libraries/common/src/main/java/androidx/media3/common/ForwardingPlayer.java +++ b/libraries/common/src/main/java/androidx/media3/common/ForwardingPlayer.java @@ -20,11 +20,13 @@ import android.view.SurfaceHolder; import android.view.SurfaceView; import android.view.TextureView; +import androidx.annotation.GuardedBy; import androidx.annotation.Nullable; import androidx.media3.common.text.Cue; import androidx.media3.common.text.CueGroup; import androidx.media3.common.util.Size; import androidx.media3.common.util.UnstableApi; +import java.util.IdentityHashMap; import java.util.List; /** @@ -55,6 +57,9 @@ public class ForwardingPlayer implements Player { private final Player player; + @GuardedBy("listeners") + private final IdentityHashMap listeners = new IdentityHashMap<>(); + /** Creates a new instance that forwards all operations to {@code player}. */ public ForwardingPlayer(Player player) { this.player = player; @@ -78,7 +83,14 @@ public Looper getApplicationLooper() { */ @Override public void addListener(Listener listener) { - player.addListener(new ForwardingListener(this, listener)); + synchronized (listeners) { + ForwardingListener forwardingListener = listeners.get(listener); + if (forwardingListener == null) { + forwardingListener = new ForwardingListener(this, listener); + } + player.addListener(forwardingListener); + listeners.put(listener, forwardingListener); + } } /** @@ -90,7 +102,12 @@ public void addListener(Listener listener) { */ @Override public void removeListener(Listener listener) { - player.removeListener(new ForwardingListener(this, listener)); + synchronized (listeners) { + Listener forwardingListener = listeners.remove(listener); + // If forwardingListener is null, we don't know this listener. Just pass in the real listener + // as the underlying player would not know it either. It can decide to throw or ignore. + player.removeListener(forwardingListener != null ? forwardingListener : listener); + } } /** Calls {@link Player#setMediaItems(List)} on the delegate. */ @@ -1070,27 +1087,5 @@ public void onDeviceInfoChanged(DeviceInfo deviceInfo) { public void onDeviceVolumeChanged(int volume, boolean muted) { listener.onDeviceVolumeChanged(volume, muted); } - - @Override - public boolean equals(@Nullable Object o) { - if (this == o) { - return true; - } - if (!(o instanceof ForwardingListener)) { - return false; - } - ForwardingListener that = (ForwardingListener) o; - if (!forwardingPlayer.equals(that.forwardingPlayer)) { - return false; - } - return listener.equals(that.listener); - } - - @Override - public int hashCode() { - int result = forwardingPlayer.hashCode(); - result = 31 * result + listener.hashCode(); - return result; - } } } diff --git a/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java b/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java index 2973583f5c9..74e755699a1 100644 --- a/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java @@ -23,14 +23,18 @@ import static androidx.media3.test.utils.TestUtil.assertSubclassOverridesAllMethods; import static androidx.media3.test.utils.TestUtil.getInnerClass; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import androidx.annotation.Nullable; import androidx.media3.test.utils.StubPlayer; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.common.collect.ImmutableSet; +import java.util.ArrayList; import java.util.HashSet; +import java.util.List; import java.util.Set; import org.junit.Test; import org.junit.runner.RunWith; @@ -41,11 +45,12 @@ public class ForwardingPlayerTest { @Test - public void addListener_addsForwardingListener() { - FakePlayer player = new FakePlayer(); - Player.Listener listener1 = mock(Player.Listener.class); - Player.Listener listener2 = mock(Player.Listener.class); + public void addListener_addsForwardingListener_toEqualityBasedPlayer() { + EqualityBasedRelaxedFakePlayer player = new EqualityBasedRelaxedFakePlayer(); + Player.Listener listener1 = new AllIsEqualPlayerListener(); + Player.Listener listener2 = new AllIsEqualPlayerListener(); + // Even though the listeners are equal, ForwardingPlayer should hide this from the player. ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player); forwardingPlayer.addListener(listener1); // Add listener1 again. @@ -56,10 +61,12 @@ public void addListener_addsForwardingListener() { } @Test - public void removeListener_removesForwardingListener() { - FakePlayer player = new FakePlayer(); - Player.Listener listener1 = mock(Player.Listener.class); - Player.Listener listener2 = mock(Player.Listener.class); + public void removeListener_removesForwardingListener_toEqualityBasedPlayer() { + EqualityBasedRelaxedFakePlayer player = new EqualityBasedRelaxedFakePlayer(); + Player.Listener listener1 = new AllIsEqualPlayerListener(); + Player.Listener listener2 = new AllIsEqualPlayerListener(); + + // Even though the listeners are equal, ForwardingPlayer should hide this from the player. ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player); forwardingPlayer.addListener(listener1); forwardingPlayer.addListener(listener2); @@ -73,9 +80,45 @@ public void removeListener_removesForwardingListener() { assertThat(player.listeners).isEmpty(); } + @Test + public void addListener_addsForwardingListener_toIdentityBasedPlayer() { + IdentityBasedStrictFakePlayer player = new IdentityBasedStrictFakePlayer(); + Player.Listener listener1 = new AllIsEqualPlayerListener(); + Player.Listener listener2 = new AllIsEqualPlayerListener(); + + // The listeners are equal, but the Player handles that, and ForwardingPlayer should, too. + ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player); + forwardingPlayer.addListener(listener1); + // Add listener1 again. + assertThrows(IllegalArgumentException.class, () -> forwardingPlayer.addListener(listener1)); + assertThat(player.listeners).hasSize(1); + forwardingPlayer.addListener(listener2); + assertThat(player.listeners).hasSize(2); + } + + @Test + public void removeListener_removesForwardingListener_toIdentityBasedPlayer() { + IdentityBasedStrictFakePlayer player = new IdentityBasedStrictFakePlayer(); + Player.Listener listener1 = new AllIsEqualPlayerListener(); + Player.Listener listener2 = new AllIsEqualPlayerListener(); + + // The listeners are equal, but the Player handles that, and ForwardingPlayer should, too. + ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player); + forwardingPlayer.addListener(listener1); + forwardingPlayer.addListener(listener2); + + forwardingPlayer.removeListener(listener1); + assertThat(player.listeners).hasSize(1); + // Remove same listener again. + assertThrows(IllegalArgumentException.class, () -> forwardingPlayer.removeListener(listener1)); + assertThat(player.listeners).hasSize(1); + forwardingPlayer.removeListener(listener2); + assertThat(player.listeners).isEmpty(); + } + @Test public void onEvents_passesForwardingPlayerAsArgument() { - FakePlayer player = new FakePlayer(); + EqualityBasedRelaxedFakePlayer player = new EqualityBasedRelaxedFakePlayer(); Player.Listener listener = mock(Player.Listener.class); ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player); forwardingPlayer.addListener(listener); @@ -125,7 +168,12 @@ public void forwardingListener_overridesAllListenerMethods() throws Exception { assertSubclassOverridesAllMethods(Player.Listener.class, forwardingListenerClass); } - private static class FakePlayer extends StubPlayer { + /** + * A {@link Player} that compares registered {@link Player.Listener} instances with {@link + * Object#equals(Object)}, and silently ignores duplicate registrations and removals of + * unrecognized listeners. + */ + private static final class EqualityBasedRelaxedFakePlayer extends StubPlayer { private final Set listeners = new HashSet<>(); @@ -139,4 +187,51 @@ public void removeListener(Listener listener) { listeners.remove(listener); } } + + /** + * A {@link Player} that compares registered {@link Player.Listener} instances with reference + * equality ({@code ==}), and throws an error on duplicate registrations and removals of + * unrecognized listeners. + */ + private static final class IdentityBasedStrictFakePlayer extends StubPlayer { + + private final List listeners = new ArrayList<>(); + + @Override + public void addListener(Listener listener) { + for (Listener listener1 : listeners) { + if (listener == listener1) { + throw new IllegalArgumentException("Trying to add duplicate listener"); + } + } + listeners.add(listener); + } + + @Override + public void removeListener(Listener listener) { + int found = -1; + for (int i = 0; i < listeners.size(); i++) { + if (listener == listeners.get(i)) { + found = i; + } + } + if (found == -1) { + throw new IllegalArgumentException("Trying to remove listener that doesn't exist"); + } + listeners.remove(found); + } + } + + private static final class AllIsEqualPlayerListener implements Player.Listener { + + @Override + public boolean equals(@Nullable Object obj) { + return true; + } + + @Override + public int hashCode() { + return 2; + } + } }