Add listener bookkeeping to ForwardingPlayer

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

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<Listener, ForwardingListener> 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;
-    }
   }
 }

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<Listener> 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<Listener> 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;
+    }
+  }
 }