Handle player and player service separately

Stypox · Stypox · commit a7a7dc536315 · 2025-02-18T19:27:46.000+01:00
This is, again, a consequence of the commit "Drop some assumptions on how PlayerService is started and reused".
This commit notified VideoDetailFragment of player starting and stopping independently of the player.
Read the comments in the code changes for more information.
diff --git a/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java
@@ -236,11 +236,14 @@ public final class VideoDetailFragment
     // Service management
     //////////////////////////////////////////////////////////////////////////*/
     @Override
-    public void onServiceConnected(final Player connectedPlayer,
-                                   final PlayerService connectedPlayerService,
-                                   final boolean playAfterConnect) {
-        player = connectedPlayer;
+    public void onServiceConnected(@NonNull final PlayerService connectedPlayerService) {
         playerService = connectedPlayerService;
+    }
+
+    @Override
+    public void onPlayerConnected(@NonNull final Player connectedPlayer,
+                                  final boolean playAfterConnect) {
+        player = connectedPlayer;
 
         // It will do nothing if the player is not in fullscreen mode
         hideSystemUiIfNeeded();
@@ -272,11 +275,18 @@ && isAutoplayEnabled()
         updateOverlayPlayQueueButtonVisibility();
     }
 
+    @Override
+    public void onPlayerDisconnected() {
+        player = null;
+        // the binding could be null at this point, if the app is finishing
+        if (binding != null) {
+            restoreDefaultBrightness();
+        }
+    }
+
     @Override
     public void onServiceDisconnected() {
         playerService = null;
-        player = null;
-        restoreDefaultBrightness();
     }
 
 
diff --git a/app/src/main/java/org/schabi/newpipe/player/PlayQueueActivity.java b/app/src/main/java/org/schabi/newpipe/player/PlayQueueActivity.java
@@ -224,7 +224,7 @@ public void onServiceConnected(final ComponentName name, final IBinder service)
                 Log.d(TAG, "Player service is connected");
 
                 if (service instanceof PlayerService.LocalBinder) {
-                    player = ((PlayerService.LocalBinder) service).getPlayer();
+                    player = ((PlayerService.LocalBinder) service).getService().getPlayer();
                 }
 
                 if (player == null || player.getPlayQueue() == null || player.exoPlayerIsNull()) {
diff --git a/app/src/main/java/org/schabi/newpipe/player/PlayerService.java b/app/src/main/java/org/schabi/newpipe/player/PlayerService.java
@@ -46,6 +46,7 @@
 
 import java.lang.ref.WeakReference;
 import java.util.List;
+import java.util.function.Consumer;
 
 
 /**
@@ -74,6 +75,13 @@ public final class PlayerService extends MediaBrowserServiceCompat {
 
     private final IBinder mBinder = new PlayerService.LocalBinder(this);
 
+    /**
+     * The parameter taken by this {@link Consumer} can be null to indicate the player is being
+     * stopped.
+     */
+    @Nullable
+    private Consumer<Player> onPlayerStartedOrStopped = null;
+
 
     //region Service lifecycle
     @Override
@@ -127,7 +135,8 @@ public int onStartCommand(final Intent intent, final int flags, final int startI
         // PlayerService using startForegroundService(), will have SHOULD_START_FOREGROUND_EXTRA,
         // to ensure startForeground() is called (otherwise Android will force-crash the app).
         if (intent.getBooleanExtra(SHOULD_START_FOREGROUND_EXTRA, false)) {
-            if (player == null) {
+            final boolean playerWasNull = (player == null);
+            if (playerWasNull) {
                 // make sure the player exists, in case the service was resumed
                 player = new Player(this, mediaSession, sessionConnector);
             }
@@ -141,6 +150,13 @@ public int onStartCommand(final Intent intent, final int flags, final int startI
             // shouldn't do anything.
             player.UIs().get(NotificationPlayerUi.class)
                     .ifPresent(NotificationPlayerUi::createNotificationAndStartForeground);
+
+            if (playerWasNull && onPlayerStartedOrStopped != null) {
+                // notify that a new player was created (but do it after creating the foreground
+                // notification just to make sure we don't incur, due to slowness, in
+                // "Context.startForegroundService() did not then call Service.startForeground()")
+                onPlayerStartedOrStopped.accept(player);
+            }
         }
 
         if (Intent.ACTION_MEDIA_BUTTON.equals(intent.getAction())
@@ -204,6 +220,10 @@ public void onDestroy() {
 
     private void cleanup() {
         if (player != null) {
+            if (onPlayerStartedOrStopped != null) {
+                // notify that the player is being destroyed
+                onPlayerStartedOrStopped.accept(null);
+            }
             player.destroy();
             player = null;
         }
@@ -279,9 +299,31 @@ public static class LocalBinder extends Binder {
         public PlayerService getService() {
             return playerService.get();
         }
+    }
 
-        public Player getPlayer() {
-            return playerService.get().player;
+    /**
+     * @return the current active player instance. May be null, since the player service can outlive
+     * the player e.g. to respond to Android Auto media browser queries.
+     */
+    @Nullable
+    public Player getPlayer() {
+        return player;
+    }
+
+    /**
+     * Sets the listener that will be called when the player is started or stopped. If a
+     * {@code null} listener is passed, then the current listener will be unset. The parameter taken
+     * by the {@link Consumer} can be null to indicate that the player is stopping.
+     * @param listener the listener to set or unset
+     */
+    public void setPlayerListener(@Nullable final Consumer<Player> listener) {
+        this.onPlayerStartedOrStopped = listener;
+        if (listener != null) {
+            if (player == null) {
+                listener.accept(null);
+            } else {
+                listener.accept(player);
+            }
         }
     }
     //endregion
diff --git a/app/src/main/java/org/schabi/newpipe/player/event/PlayerServiceExtendedEventListener.java b/app/src/main/java/org/schabi/newpipe/player/event/PlayerServiceExtendedEventListener.java
@@ -1,11 +1,48 @@
 package org.schabi.newpipe.player.event;
 
+import androidx.annotation.NonNull;
+
 import org.schabi.newpipe.player.PlayerService;
 import org.schabi.newpipe.player.Player;
 
+/**
+ * In addition to {@link PlayerServiceEventListener}, provides callbacks for service and player
+ * connections and disconnections. "Connected" here means that the service (resp. the
+ * player) is running and is bound to {@link org.schabi.newpipe.player.helper.PlayerHolder}.
+ * "Disconnected" means that either the service (resp. the player) was stopped completely, or that
+ * {@link org.schabi.newpipe.player.helper.PlayerHolder} is not bound.
+ */
 public interface PlayerServiceExtendedEventListener extends PlayerServiceEventListener {
-    void onServiceConnected(Player player,
-                            PlayerService playerService,
-                            boolean playAfterConnect);
+    /**
+     * The player service just connected to {@link org.schabi.newpipe.player.helper.PlayerHolder},
+     * but the player may not be active at this moment, e.g. in case the service is running to
+     * respond to Android Auto media browser queries without playing anything.
+     * {@link #onPlayerConnected(Player, boolean)} will be called right after this function if there
+     * is a player.
+     *
+     * @param playerService the newly connected player service
+     */
+    void onServiceConnected(@NonNull PlayerService playerService);
+
+    /**
+     * The player service is already connected and the player was just started.
+     *
+     * @param player the newly connected or started player
+     * @param playAfterConnect whether to open the video player in the video details fragment
+     */
+    void onPlayerConnected(@NonNull Player player, boolean playAfterConnect);
+
+    /**
+     * The player got disconnected, for one of these reasons: the player is getting closed while
+     * leaving the service open for future media browser queries, the service is stopping
+     * completely, or {@link org.schabi.newpipe.player.helper.PlayerHolder} is unbinding.
+     */
+    void onPlayerDisconnected();
+
+    /**
+     * The service got disconnected from {@link org.schabi.newpipe.player.helper.PlayerHolder},
+     * either because {@link org.schabi.newpipe.player.helper.PlayerHolder} is unbinding or because
+     * the service is stopping completely.
+     */
     void onServiceDisconnected();
 }
diff --git a/app/src/main/java/org/schabi/newpipe/player/helper/PlayerHolder.java b/app/src/main/java/org/schabi/newpipe/player/helper/PlayerHolder.java
@@ -24,6 +24,9 @@
 import org.schabi.newpipe.player.playqueue.PlayQueue;
 import org.schabi.newpipe.util.NavigationHelper;
 
+import java.util.Optional;
+import java.util.function.Consumer;
+
 public final class PlayerHolder {
 
     private PlayerHolder() {
@@ -45,7 +48,16 @@ public static synchronized PlayerHolder getInstance() {
     private final PlayerServiceConnection serviceConnection = new PlayerServiceConnection();
     private boolean bound;
     @Nullable private PlayerService playerService;
-    @Nullable private Player player;
+
+    private Optional<Player> getPlayer() {
+        return Optional.ofNullable(playerService)
+                .flatMap(s -> Optional.ofNullable(s.getPlayer()));
+    }
+
+    private Optional<PlayQueue> getPlayQueue() {
+        // player play queue might be null e.g. while player is starting
+        return getPlayer().flatMap(p -> Optional.ofNullable(p.getPlayQueue()));
+    }
 
     /**
      * Returns the current {@link PlayerType} of the {@link PlayerService} service,
@@ -55,21 +67,15 @@ public static synchronized PlayerHolder getInstance() {
      */
     @Nullable
     public PlayerType getType() {
-        if (player == null) {
-            return null;
-        }
-        return player.getPlayerType();
+        return getPlayer().map(Player::getPlayerType).orElse(null);
     }
 
     public boolean isPlaying() {
-        if (player == null) {
-            return false;
-        }
-        return player.isPlaying();
+        return getPlayer().map(Player::isPlaying).orElse(false);
     }
 
     public boolean isPlayerOpen() {
-        return player != null;
+        return getPlayer().isPresent();
     }
 
     /**
@@ -78,26 +84,19 @@ public boolean isPlayerOpen() {
      * @return true only if the player is open and its play queue is ready (i.e. it is not null)
      */
     public boolean isPlayQueueReady() {
-        return player != null && player.getPlayQueue() != null;
+        return getPlayQueue().isPresent();
     }
 
     public boolean isBound() {
         return bound;
     }
 
     public int getQueueSize() {
-        if (player == null || player.getPlayQueue() == null) {
-            // player play queue might be null e.g. while player is starting
-            return 0;
-        }
-        return player.getPlayQueue().size();
+        return getPlayQueue().map(PlayQueue::size).orElse(0);
     }
 
     public int getQueuePosition() {
-        if (player == null || player.getPlayQueue() == null) {
-            return 0;
-        }
-        return player.getPlayQueue().getIndex();
+        return getPlayQueue().map(PlayQueue::getIndex).orElse(0);
     }
 
     public void setListener(@Nullable final PlayerServiceExtendedEventListener newListener) {
@@ -108,9 +107,10 @@ public void setListener(@Nullable final PlayerServiceExtendedEventListener newLi
         }
 
         // Force reload data from service
-        if (player != null) {
-            listener.onServiceConnected(player, playerService, false);
+        if (playerService != null) {
+            listener.onServiceConnected(playerService);
             startPlayerListener();
+            // ^ will call listener.onPlayerConnected() down the line if there is an active player
         }
     }
 
@@ -181,11 +181,12 @@ public void onServiceConnected(final ComponentName compName, final IBinder servi
             final PlayerService.LocalBinder localBinder = (PlayerService.LocalBinder) service;
 
             playerService = localBinder.getService();
-            player = localBinder.getPlayer();
             if (listener != null) {
-                listener.onServiceConnected(player, playerService, playAfterConnect);
+                listener.onServiceConnected(playerService);
+                getPlayer().ifPresent(p -> listener.onPlayerConnected(p, playAfterConnect));
             }
             startPlayerListener();
+            // ^ will call listener.onPlayerConnected() down the line if there is an active player
 
             // notify the main activity that binding the service has completed, so that it can
             // open the bottom mini-player
@@ -229,25 +230,32 @@ private void unbind(final Context context) {
             bound = false;
             stopPlayerListener();
             playerService = null;
-            player = null;
             if (listener != null) {
+                listener.onPlayerDisconnected();
                 listener.onServiceDisconnected();
             }
         }
     }
 
     private void startPlayerListener() {
-        if (player != null) {
-            player.setFragmentListener(internalListener);
+        if (playerService != null) {
+            // setting the player listener will take care of calling relevant callbacks if the
+            // player in the service is (not) already active, also see playerStateListener below
+            playerService.setPlayerListener(playerStateListener);
         }
+        getPlayer().ifPresent(p -> p.setFragmentListener(internalListener));
     }
 
     private void stopPlayerListener() {
-        if (player != null) {
-            player.removeFragmentListener(internalListener);
+        if (playerService != null) {
+            playerService.setPlayerListener(null);
         }
+        getPlayer().ifPresent(p -> p.removeFragmentListener(internalListener));
     }
 
+    /**
+     * This listener will be held by the players created by {@link PlayerService}.
+     */
     private final PlayerServiceEventListener internalListener =
             new PlayerServiceEventListener() {
                 @Override
@@ -334,4 +342,23 @@ public void onServiceStopped() {
                     unbind(getCommonContext());
                 }
             };
+
+    /**
+     * This listener will be held by bound {@link PlayerService}s to notify of the player starting
+     * or stopping. This is necessary since the service outlives the player e.g. to answer Android
+     * Auto media browser queries.
+     */
+    private final Consumer<Player> playerStateListener = (@Nullable final Player player) -> {
+        if (listener != null) {
+            if (player == null) {
+                // player.fragmentListener=null is already done by player.stopActivityBinding(),
+                // which is called by player.destroy(), which is in turn called by PlayerService
+                // before setting its player to null
+                listener.onPlayerDisconnected();
+            } else {
+                listener.onPlayerConnected(player, serviceConnection.playAfterConnect);
+                player.setFragmentListener(internalListener);
+            }
+        }
+    };
 }

Original file line number	Diff line number	Diff line change
`@@ -224,7 +224,7 @@ public void onServiceConnected(final ComponentName name, final IBinder service)`
`224`	`224`	`Log.d(TAG, "Player service is connected");`
`225`	`225`
`226`	`226`	`if (service instanceof PlayerService.LocalBinder) {`
`227`		`- player = ((PlayerService.LocalBinder) service).getPlayer();`
	`227`	`+ player = ((PlayerService.LocalBinder) service).getService().getPlayer();`
`228`	`228`	`}`
`229`	`229`
`230`	`230`	`if (player == null \|\| player.getPlayQueue() == null \|\| player.exoPlayerIsNull()) {`