From 92a07a34456c5df560454a335f89ba677719c313 Mon Sep 17 00:00:00 2001 From: Stypox Date: Fri, 5 Sep 2025 22:47:45 +0200 Subject: [PATCH 1/2] Use tryBindIfNeeded(), send player started only if player!=null This commit fixes one way ghost notifications could be produced (although I don't know if there are other ways). This is the call chain that would lead to ghost notifications being created: 1. the system starts `PlayerService` to query information from it, without providing `SHOULD_START_FOREGROUND_EXTRA=true`, so NewPipe does not start the player nor show any notification, as expected 2. the `PlayerHolder::serviceConnection.onServiceConnected()` gets called by the system to inform `PlayerHolder` that the player started 3. `PlayerHolder` notifies `MainActivity` that the player has started (although in fact only the service has started), by sending a `ACTION_PLAYER_STARTED` broadcast 4. `MainActivity` receives the `ACTION_PLAYER_STARTED` broadcast and brings up the mini-player, but then also tries to make `PlayerHolder` bind to `PlayerService` just in case it was not bound yet, but does so using `PlayerHolder::startService()` instead of the more passive `PlayerHolder::tryBindIfNeeded()` 5. `PlayerHolder::startService()` sends an intent to the `PlayerService` again, this time with `startForegroundService` and with `SHOULD_START_FOREGROUND_EXTRA=true` 6. the `PlayerService` receives the intent and due to `SHOULD_START_FOREGROUND_EXTRA=true` decides to start up the player and show a dummy notification Steps 3 and 4 are wrong, and this commit fixes them: 3. `PlayerHolder` will now broadcast `ACTION_PLAYER_STARTED` when the service connects, only if the player is not-null 4. `PlayerHolder::tryBindIfNeeded()` is now used to passively try to bind, instead of `PlayerHolder::startService()` --- .../newpipe/fragments/detail/VideoDetailFragment.java | 6 ++---- .../org/schabi/newpipe/player/helper/PlayerHolder.java | 8 +++++--- 2 files changed, 7 insertions(+), 7 deletions(-) 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 c43007da4..7b8705565 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 @@ -1416,10 +1416,8 @@ public final class VideoDetailFragment bottomSheetBehavior.setState(BottomSheetBehavior.STATE_COLLAPSED); } // Rebound to the service if it was closed via notification or mini player - if (!playerHolder.isBound()) { - playerHolder.startService( - false, VideoDetailFragment.this); - } + playerHolder.setListener(VideoDetailFragment.this); + playerHolder.tryBindIfNeeded(context); break; } } 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 97f2d6717..9edfc804a 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 @@ -192,9 +192,11 @@ public final class PlayerHolder { 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 - NavigationHelper.sendPlayerStartedEvent(localBinder.getService()); + if (playerService != null && playerService.getPlayer() != null) { + // notify the main activity that binding the service has completed and that there is + // a player, so that it can open the bottom mini-player + NavigationHelper.sendPlayerStartedEvent(localBinder.getService()); + } } } From aa2b4821e26e8432c04a0fa5afc7a6eb7145ce7c Mon Sep 17 00:00:00 2001 From: Stypox Date: Wed, 17 Sep 2025 11:45:02 +0200 Subject: [PATCH 2/2] Post dummy notification then close player service on invalid intent This should solve "Context.startForegroundService() did not then call Service.startForeground()" according to https://github.com/TeamNewPipe/NewPipe/issues/12489#issuecomment-3290318112 --- .../schabi/newpipe/player/PlayerService.java | 30 ++++----- .../player/notification/NotificationUtil.java | 61 ++++++++++++------- 2 files changed, 55 insertions(+), 36 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/player/PlayerService.java b/app/src/main/java/org/schabi/newpipe/player/PlayerService.java index 3b6224b47..dba30f9e8 100644 --- a/app/src/main/java/org/schabi/newpipe/player/PlayerService.java +++ b/app/src/main/java/org/schabi/newpipe/player/PlayerService.java @@ -40,6 +40,7 @@ import org.schabi.newpipe.player.mediabrowser.MediaBrowserImpl; import org.schabi.newpipe.player.mediabrowser.MediaBrowserPlaybackPreparer; import org.schabi.newpipe.player.mediasession.MediaSessionPlayerUi; import org.schabi.newpipe.player.notification.NotificationPlayerUi; +import org.schabi.newpipe.player.notification.NotificationUtil; import org.schabi.newpipe.util.ThemeHelper; import java.lang.ref.WeakReference; @@ -156,25 +157,24 @@ public final class PlayerService extends MediaBrowserServiceCompat { } } - if (Intent.ACTION_MEDIA_BUTTON.equals(intent.getAction()) - && (player == null || player.getPlayQueue() == null)) { - /* - No need to process media button's actions if the player is not working, otherwise - the player service would strangely start with nothing to play - Stop the service in this case, which will be removed from the foreground and its - notification cancelled in its destruction - */ + if (player == null) { + // No need to process media button's actions or other system intents if the player is + // not running. However, since the current intent might have been issued by the system + // with `startForegroundService()` (for unknown reasons), we need to ensure that we post + // a (dummy) foreground notification, otherwise we'd incur in + // "Context.startForegroundService() did not then call Service.startForeground()". Then + // we stop the service again. + Log.d(TAG, "onStartCommand() got a useless intent, closing the service"); + NotificationUtil.startForegroundWithDummyNotification(this); destroyPlayerAndStopService(); return START_NOT_STICKY; } - if (player != null) { - final PlayerType oldPlayerType = player.getPlayerType(); - player.handleIntent(intent); - player.handleIntentPost(oldPlayerType); - player.UIs().get(MediaSessionPlayerUi.class) - .ifPresent(ui -> ui.handleMediaButtonIntent(intent)); - } + final PlayerType oldPlayerType = player.getPlayerType(); + player.handleIntent(intent); + player.handleIntentPost(oldPlayerType); + player.UIs().get(MediaSessionPlayerUi.class) + .ifPresent(ui -> ui.handleMediaButtonIntent(intent)); return START_NOT_STICKY; } diff --git a/app/src/main/java/org/schabi/newpipe/player/notification/NotificationUtil.java b/app/src/main/java/org/schabi/newpipe/player/notification/NotificationUtil.java index 79ae81de2..9b9c47b0e 100644 --- a/app/src/main/java/org/schabi/newpipe/player/notification/NotificationUtil.java +++ b/app/src/main/java/org/schabi/newpipe/player/notification/NotificationUtil.java @@ -5,7 +5,9 @@ import static androidx.media.app.NotificationCompat.MediaStyle; import static org.schabi.newpipe.player.notification.NotificationConstants.ACTION_CLOSE; import android.annotation.SuppressLint; +import android.app.Notification; import android.app.PendingIntent; +import android.content.Context; import android.content.Intent; import android.content.pm.ServiceInfo; import android.graphics.Bitmap; @@ -24,6 +26,7 @@ import org.schabi.newpipe.MainActivity; import org.schabi.newpipe.R; import org.schabi.newpipe.player.Player; import org.schabi.newpipe.player.PlayerIntentType; +import org.schabi.newpipe.player.PlayerService; import org.schabi.newpipe.player.mediasession.MediaSessionPlayerUi; import org.schabi.newpipe.util.NavigationHelper; @@ -90,12 +93,9 @@ public final class NotificationUtil { Log.d(TAG, "createNotification()"); } notificationManager = NotificationManagerCompat.from(player.getContext()); - final NotificationCompat.Builder builder = - new NotificationCompat.Builder(player.getContext(), - player.getContext().getString(R.string.notification_channel_id)); - final MediaStyle mediaStyle = new MediaStyle(); // setup media style (compact notification slots and media session) + final MediaStyle mediaStyle = new MediaStyle(); if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) { // notification actions are ignored on Android 13+, and are replaced by code in // MediaSessionPlayerUi @@ -108,18 +108,9 @@ public final class NotificationUtil { .ifPresent(mediaStyle::setMediaSession); // setup notification builder - builder.setStyle(mediaStyle) - .setPriority(NotificationCompat.PRIORITY_HIGH) - .setVisibility(NotificationCompat.VISIBILITY_PUBLIC) - .setCategory(NotificationCompat.CATEGORY_TRANSPORT) - .setShowWhen(false) - .setSmallIcon(R.drawable.ic_newpipe_triangle_white) - .setColor(ContextCompat.getColor(player.getContext(), - R.color.dark_background_color)) + final var builder = setupNotificationBuilder(player.getContext(), mediaStyle) .setColorized(player.getPrefs().getBoolean( - player.getContext().getString(R.string.notification_colorize_key), true)) - .setDeleteIntent(PendingIntentCompat.getBroadcast(player.getContext(), - NOTIFICATION_ID, new Intent(ACTION_CLOSE), FLAG_UPDATE_CURRENT, false)); + player.getContext().getString(R.string.notification_colorize_key), true)); // set the initial value for the video thumbnail, updatable with updateNotificationThumbnail setLargeIcon(builder); @@ -168,17 +159,17 @@ public final class NotificationUtil { && notificationBuilder.mActions.get(2).actionIntent != null); } + public static void startForegroundWithDummyNotification(final PlayerService service) { + final var builder = setupNotificationBuilder(service, new MediaStyle()); + startForeground(service, builder.build()); + } + public void createNotificationAndStartForeground() { if (notificationBuilder == null) { notificationBuilder = createNotification(); } updateNotification(); - - // ServiceInfo constants are not used below Android Q, so 0 is set here - final int serviceType = Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q - ? ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK : 0; - ServiceCompat.startForeground(player.getService(), NOTIFICATION_ID, - notificationBuilder.build(), serviceType); + startForeground(player.getService(), notificationBuilder.build()); } public void cancelNotificationAndStopForeground() { @@ -192,6 +183,34 @@ public final class NotificationUtil { } + ///////////////////////////////////////////////////// + // STATIC FUNCTIONS IN COMMON BETWEEN DUMMY AND REAL NOTIFICATION + ///////////////////////////////////////////////////// + + private static NotificationCompat.Builder setupNotificationBuilder(final Context context, + final MediaStyle style) { + return new NotificationCompat.Builder(context, + context.getString(R.string.notification_channel_id)) + .setStyle(style) + .setPriority(NotificationCompat.PRIORITY_HIGH) + .setVisibility(NotificationCompat.VISIBILITY_PUBLIC) + .setCategory(NotificationCompat.CATEGORY_TRANSPORT) + .setShowWhen(false) + .setSmallIcon(R.drawable.ic_newpipe_triangle_white) + .setColor(ContextCompat.getColor(context, R.color.dark_background_color)) + .setDeleteIntent(PendingIntentCompat.getBroadcast(context, + NOTIFICATION_ID, new Intent(ACTION_CLOSE), FLAG_UPDATE_CURRENT, false)); + } + + private static void startForeground(final PlayerService service, + final Notification notification) { + // ServiceInfo constants are not used below Android Q, so 0 is set here + final int serviceType = Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q + ? ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PLAYBACK : 0; + ServiceCompat.startForeground(service, NOTIFICATION_ID, notification, serviceType); + } + + ///////////////////////////////////////////////////// // ACTIONS /////////////////////////////////////////////////////