Add listener bookkeeping to ForwardingPlayer #2676

nift4 · 2025-07-27T18:35:26Z

icbaker

You can probably also remove the ForwardingListener.equals/hashCode implementations in this same PR?

Please can you also add some tests to ForwardingPlayerTest for this change.

icbaker · 2025-07-28T12:59:31Z

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

@@ -78,7 +81,9 @@ public Looper getApplicationLooper() {
   */
  @Override
  public void addListener(Listener listener) {
-    player.addListener(new ForwardingListener(this, listener));
+    ForwardingListener forwardingListener = new ForwardingListener(this, listener);
+    listeners.put(listener, forwardingListener);


Player.addListener() can be called on any thread, so you need some concurrency protection here.

icbaker · 2025-07-28T13:51:21Z

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

@@ -78,7 +81,9 @@ public Looper getApplicationLooper() {
   */
  @Override
  public void addListener(Listener listener) {
-    player.addListener(new ForwardingListener(this, listener));
+    ForwardingListener forwardingListener = new ForwardingListener(this, listener);
+    listeners.put(listener, forwardingListener);


You probably want to check whether the map already contains key = listener or not - otherwise the following operations go wrong:

// Adds listener -> forwardingListener1 mapping & passes // forwardingListener1 to underlying player. forwardingPlayer.addListener(listener); // Replaces listener -> forwardingListener1 mapping with // listener -> forwardingListener2 and passes // forwardingListener1 to underlying player. forwardingPlayer.addListener(listener); // Removes listener -> forwardingListener2 mapping // and removes forwardingListener2 from underlying player. forwardingPlayer.removeListener(listener); // No way to remove forwardingListener1 from the underlying player...

icbaker · 2025-07-28T13:53:30Z

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

@@ -54,6 +56,7 @@
 public class ForwardingPlayer implements Player {

  private final Player player;
+  protected final Map<Listener, Listener> listeners = new HashMap<>();


Can this be private?

Consider also using IdentityHashMap to reinforce the idea of identity equality.

icbaker · 2025-07-28T13:59:32Z

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

-    player.removeListener(new ForwardingListener(this, listener));
+    Listener forwardingListener = listeners.remove(listener);
+    if (forwardingListener == null) {
+      throw new IllegalArgumentException("Trying to remove listener that never was registered");


I wonder if we can avoid always throwing an exception in this case - atm ExoPlayer doesn't throw if you remove an unrecognized listener (since it's assumed that the goal state of "this listener isn't registered" is met by doing nothing, so the method can do nothing).

I don't think the Player interface mandates what happens in this case. We could delegate the decision of "what happens when an unrecognized listener is removed?" to the underlying impl by deliberately passing in a known-unrecognized impl in here:

if (forwardingListener == null) { // Try and remove a listener instance that is guaranteed not to be // registered, allowing the underlying impl to either throw or ignore. player.removeListener(new Listener() {}); }

icbaker · 2025-07-28T14:05:07Z

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

@@ -78,7 +81,9 @@ public Looper getApplicationLooper() {
   */
  @Override
  public void addListener(Listener listener) {
-    player.addListener(new ForwardingListener(this, listener));
+    ForwardingListener forwardingListener = new ForwardingListener(this, listener);
+    listeners.put(listener, forwardingListener);


I wonder if this addition to the bookkeeping set should go after the call to the underlying player (in case it fails for some reason)?

nift4 · 2025-07-28T19:47:24Z

Thanks for the thoughful review. :)

You can probably also remove the ForwardingListener.equals/hashCode implementations in this same PR?

Done.

Please can you also add some tests to ForwardingPlayerTest for this change.

Done.

Player.addListener() can be called on any thread, so you need some concurrency protection here.

Done.

You probably want to check whether the map already contains key = listener or not - otherwise the following operations go wrong:

Done, and this is tested in the test now.

Can this be private?

Yeah, it could, but I think it's better to make it protected. While it's unrelated to the bug that prompted this PR, having access to all registered listeners to be able to send them events is quite useful in the context of a ForwardingPlayer (as it generally has to dispatch events for everything if it changes anything at all).

Consider also using IdentityHashMap to reinforce the idea of identity equality.

Done. It's also tested in the test that this works even if player does it differently.

I wonder if we can avoid always throwing an exception in this case - atm ExoPlayer doesn't throw if you remove an unrecognized listener (since it's assumed that the goal state of "this listener isn't registered" is met by doing nothing, so the method can do nothing).

I don't think the Player interface mandates what happens in this case. We could delegate the decision of "what happens when an unrecognized listener is removed?" to the underlying impl by deliberately passing in a known-unrecognized impl in here:

Although not in the way you proposed, this is done as well, and tested as well.

I wonder if this addition to the bookkeeping set should go after the call to the underlying player (in case it fails for some reason)?

Done.

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

icbaker · 2025-07-29T10:39:56Z

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

+    player.addListener(listener1);
+    player.addListener(listener2);
+    assertThat(player.listeners).hasSize(1);
+    player.listeners.clear();


Up to here it looks like you are basically testing your test/fake implementations, and not the actual prod code.

I think the implementation of AllIsEqualPlayerListener and EqualityBasedFakePlayer is trivial enough that you don't need to check it every time (so you can remove those assertions here and below).

Point taken, will remove.

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

icbaker · 2025-07-29T10:51:48Z

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

+    // Add listener1 again.
+    assertThrows(IllegalArgumentException.class, () -> forwardingPlayer.addListener(listener1));
+    assertThat(player.listeners).hasSize(1);
+    assertThrows(IllegalArgumentException.class, () -> forwardingPlayer.removeListener(listener2));


I think this case of removing an un-registered listener is covered by the assertion below on L159, so it can likely be removed from here.

icbaker · 2025-07-29T10:52:39Z

Can this be private?

Yeah, it could, but I think it's better to make it protected. While it's unrelated to the bug that prompted this PR, having access to all registered listeners to be able to send them events is quite useful in the context of a ForwardingPlayer (as it generally has to dispatch events for everything if it changes anything at all).

I understand this desire, but I think we should keep it private in this PR and consider whether we want to open up the visibility as a follow-up (since as you say, it's pretty unrelated to this fix). Arguments against opening it up:

An implementation of ForwardingPlayer that is getting complex enough to need to fire its own listener events should probably be using ForwardingSimpleBasePlayer instead.
Subclasses would have to be careful to add their own synchronized block on every access too.
Simply making it protected allows subclasses to also add/remove listeners, which gets increasingly hard to reason about
- This and (2) could be mitigated by explicit 'fire event X' methods instead of exposing the whole collection of listeners, but I think (1) is probably the most compelling argument that this is trying to make ForwardingPlayer "too clever".

nift4 · 2025-07-29T11:05:36Z

I understand this desire, but I think we should keep it private in this PR

Point taken, I admit I'm only using ForwardingPlayer for this because ForwardingSimpleBasePlayer didn't work out as ExoPlayer keeps triggering asserts (see #2674). The long term solution is to fix ExoPlayer to not trigger asserts and use ForwardingSimpleBasePlayer.

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

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

Issue: androidx#2675

icbaker · 2025-07-29T13:06:53Z

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

nift4 mentioned this pull request Jul 27, 2025

Playback resumption - How to opt-out at runtime? #770

Open

1 task

icbaker self-requested a review July 28, 2025 08:21

icbaker self-assigned this Jul 28, 2025

icbaker linked an issue Jul 28, 2025 that may be closed by this pull request

ForwardingPlayer removeListener() doesn't work if Player uses object sameness instead of equality #2675

Open

1 task

icbaker requested changes Jul 28, 2025

View reviewed changes

nift4 force-pushed the fwplistener branch 2 times, most recently from 3f70575 to 98fa66f Compare July 28, 2025 19:43

nift4 force-pushed the fwplistener branch from 98fa66f to 1be7446 Compare July 28, 2025 20:08

icbaker reviewed Jul 29, 2025

View reviewed changes

nift4 force-pushed the fwplistener branch from 1be7446 to 4a37380 Compare July 29, 2025 11:04

icbaker approved these changes Jul 29, 2025

View reviewed changes

libraries/common/src/main/java/androidx/media3/common/ForwardingPlayer.java Show resolved Hide resolved

libraries/common/src/test/java/androidx/media3/common/ForwardingPlayerTest.java Show resolved Hide resolved

nift4 and others added 2 commits July 29, 2025 14:04

Add listener bookkeeping to ForwardingPlayer

04f7564

Issue: androidx#2675

Small clean-ups in preparation for internal review

230c032

icbaker added the should merge label Jul 29, 2025

Add release note

82d3b1a

icbaker force-pushed the fwplistener branch from 4a37380 to 82d3b1a Compare July 29, 2025 13:07

Add listener bookkeeping to ForwardingPlayer #2676

Are you sure you want to change the base?

Add listener bookkeeping to ForwardingPlayer #2676

Conversation

nift4 commented Jul 27, 2025

Uh oh!

icbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nift4 commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icbaker commented Jul 29, 2025

Uh oh!

nift4 commented Jul 29, 2025

Uh oh!

Uh oh!

Uh oh!

icbaker commented Jul 29, 2025

Uh oh!

Uh oh!

nift4 commented Jul 28, 2025 •

edited

Loading