Skip to content

Commit 1be7446

Browse files
nift4nift4
committed
Add listener bookkeeping to ForwardingPlayer
Issue: #2675
1 parent f99b4e7 commit 1be7446

File tree

2 files changed

+170
-34
lines changed

2 files changed

+170
-34
lines changed

‎libraries/common/src/main/java/androidx/media3/common/ForwardingPlayer.java

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import androidx.media3.common.text.CueGroup;
2626
import androidx.media3.common.util.Size;
2727
import androidx.media3.common.util.UnstableApi;
28+
import java.util.IdentityHashMap;
2829
import java.util.List;
2930

3031
/**
@@ -54,6 +55,7 @@
5455
public class ForwardingPlayer implements Player {
5556

5657
private final Player player;
58+
protected final IdentityHashMap<Listener, Listener> listeners = new IdentityHashMap<>();
5759

5860
/** Creates a new instance that forwards all operations to {@code player}. */
5961
public ForwardingPlayer(Player player) {
@@ -78,7 +80,14 @@ public Looper getApplicationLooper() {
7880
*/
7981
@Override
8082
public void addListener(Listener listener) {
81-
player.addListener(new ForwardingListener(this, listener));
83+
synchronized (listeners) {
84+
Listener forwardingListener = listeners.get(listener);
85+
if (forwardingListener == null) {
86+
forwardingListener = new ForwardingListener(this, listener);
87+
}
88+
player.addListener(forwardingListener);
89+
listeners.put(listener, forwardingListener);
90+
}
8291
}
8392

8493
/**
@@ -90,7 +99,12 @@ public void addListener(Listener listener) {
9099
*/
91100
@Override
92101
public void removeListener(Listener listener) {
93-
player.removeListener(new ForwardingListener(this, listener));
102+
synchronized (listeners) {
103+
Listener forwardingListener = listeners.remove(listener);
104+
// If forwardingListener is null, we don't know this listener. Just pass in the real listener
105+
// as the underlying player would not know it either. It can decide to throw or ignore.
106+
player.removeListener(forwardingListener != null ? forwardingListener : listener);
107+
}
94108
}
95109

96110
/** Calls {@link Player#setMediaItems(List)} on the delegate. */
@@ -1070,27 +1084,5 @@ public void onDeviceInfoChanged(DeviceInfo deviceInfo) {
10701084
public void onDeviceVolumeChanged(int volume, boolean muted) {
10711085
listener.onDeviceVolumeChanged(volume, muted);
10721086
}
1073-
1074-
@Override
1075-
public boolean equals(@Nullable Object o) {
1076-
if (this == o) {
1077-
return true;
1078-
}
1079-
if (!(o instanceof ForwardingListener)) {
1080-
return false;
1081-
}
1082-
ForwardingListener that = (ForwardingListener) o;
1083-
if (!forwardingPlayer.equals(that.forwardingPlayer)) {
1084-
return false;
1085-
}
1086-
return listener.equals(that.listener);
1087-
}
1088-
1089-
@Override
1090-
public int hashCode() {
1091-
int result = forwardingPlayer.hashCode();
1092-
result = 31 * result + listener.hashCode();
1093-
return result;
1094-
}
10951087
}
10961088
}

‎libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java

Lines changed: 154 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,18 @@
2323
import static androidx.media3.test.utils.TestUtil.assertSubclassOverridesAllMethods;
2424
import static androidx.media3.test.utils.TestUtil.getInnerClass;
2525
import static com.google.common.truth.Truth.assertThat;
26+
import static org.junit.Assert.assertThrows;
2627
import static org.mockito.ArgumentMatchers.same;
2728
import static org.mockito.Mockito.mock;
2829
import static org.mockito.Mockito.verify;
2930

31+
import androidx.annotation.Nullable;
3032
import androidx.media3.test.utils.StubPlayer;
3133
import androidx.test.ext.junit.runners.AndroidJUnit4;
3234
import com.google.common.collect.ImmutableSet;
35+
import java.util.ArrayList;
3336
import java.util.HashSet;
37+
import java.util.List;
3438
import java.util.Set;
3539
import org.junit.Test;
3640
import org.junit.runner.RunWith;
@@ -41,11 +45,23 @@
4145
public class ForwardingPlayerTest {
4246

4347
@Test
44-
public void addListener_addsForwardingListener() {
45-
FakePlayer player = new FakePlayer();
46-
Player.Listener listener1 = mock(Player.Listener.class);
47-
Player.Listener listener2 = mock(Player.Listener.class);
48+
public void addListener_addsForwardingListener_toEqualityBasedPlayer() {
49+
EqualityBasedFakePlayer player = new EqualityBasedFakePlayer();
50+
Player.Listener listener1 = new AllIsEqualPlayerListener();
51+
Player.Listener listener2 = new AllIsEqualPlayerListener();
52+
assertThat(listener1).isEqualTo(listener2);
53+
assertThat(listener2).isEqualTo(listener1);
54+
assertThat(listener1.hashCode()).isEqualTo(2);
55+
assertThat(listener2.hashCode()).isEqualTo(2);
4856

57+
player.addListener(listener1);
58+
// Add listener1 again.
59+
player.addListener(listener1);
60+
player.addListener(listener2);
61+
assertThat(player.listeners).hasSize(1);
62+
player.listeners.clear();
63+
64+
// Even though the listeners are equal, ForwardingPlayer should hide this from the player.
4965
ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player);
5066
forwardingPlayer.addListener(listener1);
5167
// Add listener1 again.
@@ -56,10 +72,27 @@ public void addListener_addsForwardingListener() {
5672
}
5773

5874
@Test
59-
public void removeListener_removesForwardingListener() {
60-
FakePlayer player = new FakePlayer();
61-
Player.Listener listener1 = mock(Player.Listener.class);
62-
Player.Listener listener2 = mock(Player.Listener.class);
75+
public void removeListener_removesForwardingListener_toEqualityBasedPlayer() {
76+
EqualityBasedFakePlayer player = new EqualityBasedFakePlayer();
77+
Player.Listener listener1 = new AllIsEqualPlayerListener();
78+
Player.Listener listener2 = new AllIsEqualPlayerListener();
79+
assertThat(listener1).isEqualTo(listener2);
80+
assertThat(listener2).isEqualTo(listener1);
81+
assertThat(listener1.hashCode()).isEqualTo(2);
82+
assertThat(listener2.hashCode()).isEqualTo(2);
83+
84+
player.addListener(listener1);
85+
player.addListener(listener2);
86+
assertThat(player.listeners).hasSize(1);
87+
player.removeListener(listener1);
88+
assertThat(player.listeners).isEmpty();
89+
// Remove same listener again.
90+
player.removeListener(listener1);
91+
assertThat(player.listeners).isEmpty();
92+
player.removeListener(listener2);
93+
assertThat(player.listeners).isEmpty();
94+
95+
// Even though the listeners are equal, ForwardingPlayer should hide this from the player.
6396
ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player);
6497
forwardingPlayer.addListener(listener1);
6598
forwardingPlayer.addListener(listener2);
@@ -73,9 +106,78 @@ public void removeListener_removesForwardingListener() {
73106
assertThat(player.listeners).isEmpty();
74107
}
75108

109+
@Test
110+
public void addListener_addsForwardingListener_toIdentityBasedPlayer() {
111+
IdentityBasedFakePlayer player = new IdentityBasedFakePlayer();
112+
Player.Listener listener1 = new AllIsEqualPlayerListener();
113+
Player.Listener listener2 = new AllIsEqualPlayerListener();
114+
assertThat(listener1).isEqualTo(listener2);
115+
assertThat(listener2).isEqualTo(listener1);
116+
assertThat(listener1.hashCode()).isEqualTo(2);
117+
assertThat(listener2.hashCode()).isEqualTo(2);
118+
119+
player.addListener(listener1);
120+
// Add listener1 again.
121+
assertThrows(IllegalArgumentException.class, () -> player.addListener(listener1));
122+
assertThat(player.listeners).hasSize(1);
123+
assertThrows(IllegalArgumentException.class, () -> player.removeListener(listener2));
124+
assertThat(player.listeners).hasSize(1);
125+
player.addListener(listener2);
126+
127+
assertThat(player.listeners).hasSize(2);
128+
player.listeners.clear();
129+
130+
// The listeners are equal, but the Player handles that, and ForwardingPlayer should, too.
131+
ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player);
132+
forwardingPlayer.addListener(listener1);
133+
// Add listener1 again.
134+
assertThrows(IllegalArgumentException.class, () -> forwardingPlayer.addListener(listener1));
135+
assertThat(player.listeners).hasSize(1);
136+
assertThrows(IllegalArgumentException.class, () -> forwardingPlayer.removeListener(listener2));
137+
assertThat(player.listeners).hasSize(1);
138+
forwardingPlayer.addListener(listener2);
139+
140+
assertThat(player.listeners).hasSize(2);
141+
}
142+
143+
@Test
144+
public void removeListener_removesForwardingListener_toIdentityBasedPlayer() {
145+
IdentityBasedFakePlayer player = new IdentityBasedFakePlayer();
146+
Player.Listener listener1 = new AllIsEqualPlayerListener();
147+
Player.Listener listener2 = new AllIsEqualPlayerListener();
148+
assertThat(listener1).isEqualTo(listener2);
149+
assertThat(listener2).isEqualTo(listener1);
150+
assertThat(listener1.hashCode()).isEqualTo(2);
151+
assertThat(listener2.hashCode()).isEqualTo(2);
152+
153+
player.addListener(listener1);
154+
player.addListener(listener2);
155+
156+
player.removeListener(listener1);
157+
assertThat(player.listeners).hasSize(1);
158+
// Remove same listener again.
159+
assertThrows(IllegalArgumentException.class, () -> player.removeListener(listener1));
160+
assertThat(player.listeners).hasSize(1);
161+
player.removeListener(listener2);
162+
assertThat(player.listeners).isEmpty();
163+
164+
// The listeners are equal, but the Player handles that, and ForwardingPlayer should, too.
165+
ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player);
166+
forwardingPlayer.addListener(listener1);
167+
forwardingPlayer.addListener(listener2);
168+
169+
forwardingPlayer.removeListener(listener1);
170+
assertThat(player.listeners).hasSize(1);
171+
// Remove same listener again.
172+
assertThrows(IllegalArgumentException.class, () -> forwardingPlayer.removeListener(listener1));
173+
assertThat(player.listeners).hasSize(1);
174+
forwardingPlayer.removeListener(listener2);
175+
assertThat(player.listeners).isEmpty();
176+
}
177+
76178
@Test
77179
public void onEvents_passesForwardingPlayerAsArgument() {
78-
FakePlayer player = new FakePlayer();
180+
EqualityBasedFakePlayer player = new EqualityBasedFakePlayer();
79181
Player.Listener listener = mock(Player.Listener.class);
80182
ForwardingPlayer forwardingPlayer = new ForwardingPlayer(player);
81183
forwardingPlayer.addListener(listener);
@@ -125,7 +227,7 @@ public void forwardingListener_overridesAllListenerMethods() throws Exception {
125227
assertSubclassOverridesAllMethods(Player.Listener.class, forwardingListenerClass);
126228
}
127229

128-
private static class FakePlayer extends StubPlayer {
230+
private static class EqualityBasedFakePlayer extends StubPlayer {
129231

130232
private final Set<Listener> listeners = new HashSet<>();
131233

@@ -139,4 +241,46 @@ public void removeListener(Listener listener) {
139241
listeners.remove(listener);
140242
}
141243
}
244+
245+
private static class IdentityBasedFakePlayer extends StubPlayer {
246+
247+
private final List<Listener> listeners = new ArrayList<>();
248+
249+
@Override
250+
public void addListener(Listener listener) {
251+
for (Listener listener1 : listeners) {
252+
if (listener == listener1) {
253+
throw new IllegalArgumentException("Trying to add duplicate listener added");
254+
}
255+
}
256+
listeners.add(listener);
257+
}
258+
259+
@Override
260+
public void removeListener(Listener listener) {
261+
int found = -1;
262+
for (int i = 0; i < listeners.size(); i++) {
263+
if (listener == listeners.get(i)) {
264+
found = i;
265+
}
266+
}
267+
if (found == -1) {
268+
throw new IllegalArgumentException("Trying to remove listener that doesn't exist");
269+
}
270+
listeners.remove(found);
271+
}
272+
}
273+
274+
private static class AllIsEqualPlayerListener implements Player.Listener {
275+
276+
@Override
277+
public boolean equals(@Nullable Object obj) {
278+
return true;
279+
}
280+
281+
@Override
282+
public int hashCode() {
283+
return 2;
284+
}
285+
}
142286
}

0 commit comments

Comments
 (0)