From 48c2c156cb694b597700122887504e1db3cf06af Mon Sep 17 00:00:00 2001 From: evermind Date: Thu, 24 Jun 2021 10:00:56 +0200 Subject: [PATCH] convert PlayerHolder to Singleton, handle context within, bugfix ServiceConnection leak - bugfix: have ServiceConnection created only once! - select the context within the PlayerHolder to start, stop, bind or unbind the service -> we have to make sure the Service is started AND stopped within the same context -> so let PlayerHolder be the one to select the context - remove removeListener() and replace the call with setListener(null) - Compatibility: use ContextCompat.startForegroundService instead of startService() --- .../java/org/schabi/newpipe/MainActivity.java | 2 +- .../org/schabi/newpipe/RouterActivity.java | 2 +- .../fragments/detail/VideoDetailFragment.java | 25 ++-- .../fragments/list/BaseListFragment.java | 2 +- .../list/playlist/PlaylistFragment.java | 2 +- .../schabi/newpipe/local/feed/FeedFragment.kt | 2 +- .../history/StatisticsPlaylistFragment.java | 2 +- .../local/playlist/LocalPlaylistFragment.java | 2 +- .../newpipe/player/helper/PlayerHolder.java | 129 ++++++++++-------- .../schabi/newpipe/util/NavigationHelper.java | 4 +- .../newpipe/util/StreamDialogEntry.java | 2 +- 11 files changed, 98 insertions(+), 76 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/MainActivity.java b/app/src/main/java/org/schabi/newpipe/MainActivity.java index 9bd289376..da3a8cc7d 100644 --- a/app/src/main/java/org/schabi/newpipe/MainActivity.java +++ b/app/src/main/java/org/schabi/newpipe/MainActivity.java @@ -823,7 +823,7 @@ public class MainActivity extends AppCompatActivity { return; } - if (PlayerHolder.isPlayerOpen()) { + if (PlayerHolder.getInstance().isPlayerOpen()) { // if the player is already open, no need for a broadcast receiver openMiniPlayerIfMissing(); } else { diff --git a/app/src/main/java/org/schabi/newpipe/RouterActivity.java b/app/src/main/java/org/schabi/newpipe/RouterActivity.java index 0c6165084..c8636c66c 100644 --- a/app/src/main/java/org/schabi/newpipe/RouterActivity.java +++ b/app/src/main/java/org/schabi/newpipe/RouterActivity.java @@ -453,7 +453,7 @@ public class RouterActivity extends AppCompatActivity { returnList.add(showInfo); returnList.add(videoPlayer); } else { - final MainPlayer.PlayerType playerType = PlayerHolder.getType(); + final MainPlayer.PlayerType playerType = PlayerHolder.getInstance().getType(); if (capabilities.contains(VIDEO) && PlayerHelper.isAutoplayAllowedByUser(context) && playerType == null || playerType == MainPlayer.PlayerType.VIDEO) { 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 index 9d8fd443f..18e5bb7bf 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java @@ -201,6 +201,7 @@ public final class VideoDetailFragment @Nullable private MainPlayer playerService; private Player player; + private PlayerHolder playerHolder = PlayerHolder.getInstance(); /*////////////////////////////////////////////////////////////////////////// // Service management @@ -360,9 +361,9 @@ public final class VideoDetailFragment // Stop the service when user leaves the app with double back press // if video player is selected. Otherwise unbind if (activity.isFinishing() && isPlayerAvailable() && player.videoPlayerSelected()) { - PlayerHolder.stopService(App.getApp()); + playerHolder.stopService(); } else { - PlayerHolder.removeListener(); + playerHolder.setListener(null); } PreferenceManager.getDefaultSharedPreferences(activity) @@ -660,10 +661,10 @@ public final class VideoDetailFragment }); setupBottomPlayer(); - if (!PlayerHolder.bound) { + if (!playerHolder.bound) { setHeightThumbnail(); } else { - PlayerHolder.startService(App.getApp(), false, this); + playerHolder.startService(false, this); } } @@ -1097,7 +1098,7 @@ public final class VideoDetailFragment // See UI changes while remote playQueue changes if (!isPlayerAvailable()) { - PlayerHolder.startService(App.getApp(), false, this); + playerHolder.startService(false, this); } toggleFullscreenIfInFullscreenMode(); @@ -1123,7 +1124,7 @@ public final class VideoDetailFragment private void openNormalBackgroundPlayer(final boolean append) { // See UI changes while remote playQueue changes if (!isPlayerAvailable()) { - PlayerHolder.startService(App.getApp(), false, this); + playerHolder.startService(false, this); } final PlayQueue queue = setupPlayQueueForIntent(append); @@ -1137,7 +1138,7 @@ public final class VideoDetailFragment private void openMainPlayer() { if (!isPlayerServiceAvailable()) { - PlayerHolder.startService(App.getApp(), autoPlayEnabled, this); + playerHolder.startService(autoPlayEnabled, this); return; } if (currentInfo == null) { @@ -1155,7 +1156,7 @@ public final class VideoDetailFragment final Intent playerIntent = NavigationHelper .getPlayerIntent(requireContext(), MainPlayer.class, queue, true, autoPlayEnabled); - activity.startService(playerIntent); + ContextCompat.startForegroundService(activity, playerIntent); } private void hideMainPlayer() { @@ -1373,9 +1374,9 @@ public final class VideoDetailFragment bottomSheetBehavior.setState(BottomSheetBehavior.STATE_COLLAPSED); } // Rebound to the service if it was closed via notification or mini player - if (!PlayerHolder.bound) { - PlayerHolder.startService( - App.getApp(), false, VideoDetailFragment.this); + if (!playerHolder.bound) { + playerHolder.startService( + false, VideoDetailFragment.this); } break; } @@ -2119,7 +2120,7 @@ public final class VideoDetailFragment if (currentWorker != null) { currentWorker.dispose(); } - PlayerHolder.stopService(App.getApp()); + playerHolder.stopService(); setInitialData(0, null, "", null); currentInfo = null; updateOverlayData(null, null, null); diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java index 45436ab6b..ae661cfa3 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java @@ -353,7 +353,7 @@ public abstract class BaseListFragment extends BaseStateFragment final List entries = new ArrayList<>(); - if (PlayerHolder.getType() != null) { + if (PlayerHolder.getInstance().getType() != null) { entries.add(StreamDialogEntry.enqueue); } if (item.getStreamType() == StreamType.AUDIO_STREAM) { diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/playlist/PlaylistFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/playlist/PlaylistFragment.java index de96905db..824aa2612 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/playlist/PlaylistFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/playlist/PlaylistFragment.java @@ -144,7 +144,7 @@ public class PlaylistFragment extends BaseListInfoFragment { final ArrayList entries = new ArrayList<>(); - if (PlayerHolder.getType() != null) { + if (PlayerHolder.getInstance().getType() != null) { entries.add(StreamDialogEntry.enqueue); } if (item.getStreamType() == StreamType.AUDIO_STREAM) { diff --git a/app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt b/app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt index 4c1bb0732..c235b22df 100644 --- a/app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt +++ b/app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt @@ -331,7 +331,7 @@ class FeedFragment : BaseStateFragment() { if (context == null || context.resources == null || activity == null) return val entries = ArrayList() - if (PlayerHolder.getType() != null) { + if (PlayerHolder.getInstance().getType() != null) { entries.add(StreamDialogEntry.enqueue) } if (item.streamType == StreamType.AUDIO_STREAM) { diff --git a/app/src/main/java/org/schabi/newpipe/local/history/StatisticsPlaylistFragment.java b/app/src/main/java/org/schabi/newpipe/local/history/StatisticsPlaylistFragment.java index aa871190f..166b9c04e 100644 --- a/app/src/main/java/org/schabi/newpipe/local/history/StatisticsPlaylistFragment.java +++ b/app/src/main/java/org/schabi/newpipe/local/history/StatisticsPlaylistFragment.java @@ -340,7 +340,7 @@ public class StatisticsPlaylistFragment final ArrayList entries = new ArrayList<>(); - if (PlayerHolder.getType() != null) { + if (PlayerHolder.getInstance().getType() != null) { entries.add(StreamDialogEntry.enqueue); } if (infoItem.getStreamType() == StreamType.AUDIO_STREAM) { diff --git a/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java b/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java index cefc63c0d..3edbff45a 100644 --- a/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java +++ b/app/src/main/java/org/schabi/newpipe/local/playlist/LocalPlaylistFragment.java @@ -749,7 +749,7 @@ public class LocalPlaylistFragment extends BaseLocalListFragment entries = new ArrayList<>(); - if (PlayerHolder.getType() != null) { + if (PlayerHolder.getInstance().getType() != null) { entries.add(StreamDialogEntry.enqueue); } if (infoItem.getStreamType() == StreamType.AUDIO_STREAM) { 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 index da1238c81..68de8ce9f 100644 --- a/app/src/main/java/org/schabi/newpipe/player/helper/PlayerHolder.java +++ b/app/src/main/java/org/schabi/newpipe/player/helper/PlayerHolder.java @@ -8,6 +8,7 @@ import android.os.IBinder; import android.util.Log; import androidx.annotation.Nullable; +import androidx.core.content.ContextCompat; import com.google.android.exoplayer2.ExoPlaybackException; import com.google.android.exoplayer2.PlaybackParameters; @@ -22,18 +23,27 @@ import org.schabi.newpipe.player.event.PlayerServiceExtendedEventListener; import org.schabi.newpipe.player.playqueue.PlayQueue; public final class PlayerHolder { + private PlayerHolder() { } - private static final boolean DEBUG = MainActivity.DEBUG; - private static final String TAG = "PlayerHolder"; + private static PlayerHolder instance; + public static synchronized PlayerHolder getInstance() { + if (PlayerHolder.instance == null) { + PlayerHolder.instance = new PlayerHolder(); + } + return PlayerHolder.instance; + } - private static PlayerServiceExtendedEventListener listener; + private final boolean DEBUG = MainActivity.DEBUG; + private final String TAG = PlayerHolder.class.getSimpleName(); - private static ServiceConnection serviceConnection; - public static boolean bound; - private static MainPlayer playerService; - private static Player player; + private PlayerServiceExtendedEventListener listener; + + private final PlayerServiceConnection serviceConnection = new PlayerServiceConnection(); + public boolean bound; + private MainPlayer playerService; + private Player player; /** * Returns the current {@link MainPlayer.PlayerType} of the {@link MainPlayer} service, @@ -42,26 +52,31 @@ public final class PlayerHolder { * @return Current PlayerType */ @Nullable - public static MainPlayer.PlayerType getType() { + public MainPlayer.PlayerType getType() { if (player == null) { return null; } return player.getPlayerType(); } - public static boolean isPlaying() { + public boolean isPlaying() { if (player == null) { return false; } return player.isPlaying(); } - public static boolean isPlayerOpen() { + public boolean isPlayerOpen() { return player != null; } - public static void setListener(final PlayerServiceExtendedEventListener newListener) { + public void setListener(@Nullable final PlayerServiceExtendedEventListener newListener) { listener = newListener; + + if (listener == null) { + return; + } + // Force reload data from service if (player != null) { listener.onServiceConnected(player, playerService, false); @@ -69,14 +84,15 @@ public final class PlayerHolder { } } - public static void removeListener() { - listener = null; + // helper to handle context in common place as using the same + // context to bind/unbind a service is crucial + private Context getCommonContext() { + return App.getApp(); } - - public static void startService(final Context context, - final boolean playAfterConnect, - final PlayerServiceExtendedEventListener newListener) { + public void startService(final boolean playAfterConnect, + final PlayerServiceExtendedEventListener newListener) { + final Context context = getCommonContext(); setListener(newListener); if (bound) { return; @@ -85,58 +101,65 @@ public final class PlayerHolder { // and NullPointerExceptions inside the service because the service will be // bound twice. Prevent it with unbinding first unbind(context); - context.startService(new Intent(context, MainPlayer.class)); - serviceConnection = getServiceConnection(context, playAfterConnect); + ContextCompat.startForegroundService(context, new Intent(context, MainPlayer.class)); + serviceConnection.doPlayAfterConnect(playAfterConnect); bind(context); } - public static void stopService(final Context context) { + public void stopService() { + final Context context = getCommonContext(); unbind(context); context.stopService(new Intent(context, MainPlayer.class)); } - private static ServiceConnection getServiceConnection(final Context context, - final boolean playAfterConnect) { - return new ServiceConnection() { - @Override - public void onServiceDisconnected(final ComponentName compName) { - if (DEBUG) { - Log.d(TAG, "Player service is disconnected"); - } + class PlayerServiceConnection implements ServiceConnection { - unbind(context); + private boolean playAfterConnect = false; + + public void doPlayAfterConnect(final boolean playAfterConnection) { + this.playAfterConnect = playAfterConnection; + } + + @Override + public void onServiceDisconnected(final ComponentName compName) { + if (DEBUG) { + Log.d(TAG, "Player service is disconnected"); } - @Override - public void onServiceConnected(final ComponentName compName, final IBinder service) { - if (DEBUG) { - Log.d(TAG, "Player service is connected"); - } - final MainPlayer.LocalBinder localBinder = (MainPlayer.LocalBinder) service; + final Context context = getCommonContext(); + unbind(context); + } - playerService = localBinder.getService(); - player = localBinder.getPlayer(); - if (listener != null) { - listener.onServiceConnected(player, playerService, playAfterConnect); - } - startPlayerListener(); + @Override + public void onServiceConnected(final ComponentName compName, final IBinder service) { + if (DEBUG) { + Log.d(TAG, "Player service is connected"); } - }; - } + final MainPlayer.LocalBinder localBinder = (MainPlayer.LocalBinder) service; - private static void bind(final Context context) { + playerService = localBinder.getService(); + player = localBinder.getPlayer(); + if (listener != null) { + listener.onServiceConnected(player, playerService, playAfterConnect); + } + startPlayerListener(); + } + }; + + private void bind(final Context context) { if (DEBUG) { Log.d(TAG, "bind() called"); } final Intent serviceIntent = new Intent(context, MainPlayer.class); - bound = context.bindService(serviceIntent, serviceConnection, Context.BIND_AUTO_CREATE); + bound = context.bindService(serviceIntent, serviceConnection, + Context.BIND_AUTO_CREATE); if (!bound) { context.unbindService(serviceConnection); } } - private static void unbind(final Context context) { + private void unbind(final Context context) { if (DEBUG) { Log.d(TAG, "unbind() called"); } @@ -153,21 +176,19 @@ public final class PlayerHolder { } } - - private static void startPlayerListener() { + private void startPlayerListener() { if (player != null) { - player.setFragmentListener(INNER_LISTENER); + player.setFragmentListener(internalListener); } } - private static void stopPlayerListener() { + private void stopPlayerListener() { if (player != null) { - player.removeFragmentListener(INNER_LISTENER); + player.removeFragmentListener(internalListener); } } - - private static final PlayerServiceEventListener INNER_LISTENER = + private final PlayerServiceEventListener internalListener = new PlayerServiceEventListener() { @Override public void onFullscreenStateChanged(final boolean fullscreen) { @@ -242,7 +263,7 @@ public final class PlayerHolder { if (listener != null) { listener.onServiceStopped(); } - unbind(App.getApp()); + unbind(getCommonContext()); } }; } diff --git a/app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java b/app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java index d6e1888e1..f44bd6f9f 100644 --- a/app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java +++ b/app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java @@ -349,13 +349,13 @@ public final class NavigationHelper { final boolean switchingPlayers) { final boolean autoPlay; - @Nullable final MainPlayer.PlayerType playerType = PlayerHolder.getType(); + @Nullable final MainPlayer.PlayerType playerType = PlayerHolder.getInstance().getType(); if (playerType == null) { // no player open autoPlay = PlayerHelper.isAutoplayAllowedByUser(context); } else if (switchingPlayers) { // switching player to main player - autoPlay = PlayerHolder.isPlaying(); // keep play/pause state + autoPlay = PlayerHolder.getInstance().isPlaying(); // keep play/pause state } else if (playerType == MainPlayer.PlayerType.VIDEO) { // opening new stream while already playing in main player autoPlay = PlayerHelper.isAutoplayAllowedByUser(context); diff --git a/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java b/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java index 50eeef7e7..8bfd428b8 100644 --- a/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java +++ b/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java @@ -39,7 +39,7 @@ public enum StreamDialogEntry { * Info: Add this entry within showStreamDialog. */ enqueue(R.string.enqueue_stream, (fragment, item) -> { - final MainPlayer.PlayerType type = PlayerHolder.getType(); + final MainPlayer.PlayerType type = PlayerHolder.getInstance().getType(); if (type == AUDIO) { NavigationHelper.enqueueOnBackgroundPlayer(fragment.getContext(),