Merge pull request #8899 from Stypox/fix-player-thumbnail-handling

Fix wrong thumbnail in notification on Android 13

Author: Stypox

app/src/main/java/org/schabi/newpipe/player/Player.java

@@ -765,17 +765,15 @@ public void onBitmapLoaded(final Bitmap bitmap, final Picasso.LoadedFrom from) {
                             + " -> " + bitmap.getWidth() + "x" + bitmap.getHeight() + "], from = ["
                             + from + "]");
                 }
-                currentThumbnail = bitmap;
                 // there is a new thumbnail, so e.g. the end screen thumbnail needs to change, too.
-                UIs.call(playerUi -> playerUi.onThumbnailLoaded(bitmap));
+                onThumbnailLoaded(bitmap);
             }
 
             @Override
             public void onBitmapFailed(final Exception e, final Drawable errorDrawable) {
                 Log.e(TAG, "Thumbnail - onBitmapFailed() called", e);
-                currentThumbnail = null;
                 // there is a new thumbnail, so e.g. the end screen thumbnail needs to change, too.
-                UIs.call(playerUi -> playerUi.onThumbnailLoaded(null));
+                onThumbnailLoaded(null);
             }
 
             @Override
@@ -798,7 +796,7 @@ private void loadCurrentThumbnail(final String url) {
 
         // Unset currentThumbnail, since it is now outdated. This ensures it is not used in media
         // session metadata while the new thumbnail is being loaded by Picasso.
-        currentThumbnail = null;
+        onThumbnailLoaded(null);
         if (isNullOrEmpty(url)) {
             return;
         }
@@ -813,6 +811,16 @@ private void cancelLoadingCurrentThumbnail() {
         // cancel the Picasso job associated with the player thumbnail, if any
         PicassoHelper.cancelTag(PICASSO_PLAYER_THUMBNAIL_TAG);
     }
+
+    private void onThumbnailLoaded(@Nullable final Bitmap bitmap) {
+        // Avoid useless thumbnail updates, if the thumbnail has not actually changed. Based on the
+        // thumbnail loading code, this if would be skipped only when both bitmaps are `null`, since
+        // onThumbnailLoaded won't be called twice with the same nonnull bitmap by Picasso's target.
+        if (currentThumbnail != bitmap) {
+            currentThumbnail = bitmap;
+            UIs.call(playerUi -> playerUi.onThumbnailLoaded(bitmap));
+        }
+    }
     //endregion
 
 
@@ -1501,48 +1509,50 @@ public void onPlaybackSynchronize(@NonNull final PlayQueueItem item, final boole
             Log.d(TAG, "Playback - onPlaybackSynchronize(was blocked: " + wasBlocked
                     + ") called with item=[" + item.getTitle() + "], url=[" + item.getUrl() + "]");
         }
-        if (exoPlayerIsNull() || playQueue == null) {
-            return;
+        if (exoPlayerIsNull() || playQueue == null || currentItem == item) {
+            return; // nothing to synchronize
         }
 
-        final boolean hasPlayQueueItemChanged = currentItem != item;
-
-        final int currentPlayQueueIndex = playQueue.indexOf(item);
-        final int currentPlaylistIndex = simpleExoPlayer.getCurrentMediaItemIndex();
-        final int currentPlaylistSize = simpleExoPlayer.getCurrentTimeline().getWindowCount();
+        final int playQueueIndex = playQueue.indexOf(item);
+        final int playlistIndex = simpleExoPlayer.getCurrentMediaItemIndex();
+        final int playlistSize = simpleExoPlayer.getCurrentTimeline().getWindowCount();
+        final boolean removeThumbnailBeforeSync = currentItem == null
+                || currentItem.getServiceId() != item.getServiceId()
+                || !currentItem.getUrl().equals(item.getUrl());
 
-        // If nothing to synchronize
-        if (!hasPlayQueueItemChanged) {
-            return;
-        }
         currentItem = item;
 
-        // Check if on wrong window
-        if (currentPlayQueueIndex != playQueue.getIndex()) {
-            Log.e(TAG, "Playback - Play Queue may be desynchronized: item "
-                    + "index=[" + currentPlayQueueIndex + "], "
-                    + "queue index=[" + playQueue.getIndex() + "]");
-
-            // Check if bad seek position
-        } else if ((currentPlaylistSize > 0 && currentPlayQueueIndex >= currentPlaylistSize)
-                || currentPlayQueueIndex < 0) {
-            Log.e(TAG, "Playback - Trying to seek to invalid "
-                    + "index=[" + currentPlayQueueIndex + "] with "
-                    + "playlist length=[" + currentPlaylistSize + "]");
-
-        } else if (wasBlocked || currentPlaylistIndex != currentPlayQueueIndex || !isPlaying()) {
+        if (playQueueIndex != playQueue.getIndex()) {
+            // wrong window (this should be impossible, as this method is called with
+            // `item=playQueue.getItem()`, so the index of that item must be equal to `getIndex()`)
+            Log.e(TAG, "Playback - Play Queue may be not in sync: item index=["
+                    + playQueueIndex + "], " + "queue index=[" + playQueue.getIndex() + "]");
+
+        } else if ((playlistSize > 0 && playQueueIndex >= playlistSize) || playQueueIndex < 0) {
+            // the queue and the player's timeline are not in sync, since the play queue index
+            // points outside of the timeline
+            Log.e(TAG, "Playback - Trying to seek to invalid index=[" + playQueueIndex
+                    + "] with playlist length=[" + playlistSize + "]");
+
+        } else if (wasBlocked || playlistIndex != playQueueIndex || !isPlaying()) {
+            // either the player needs to be unblocked, or the play queue index has just been
+            // changed and needs to be synchronized, or the player is not playing
             if (DEBUG) {
-                Log.d(TAG, "Playback - Rewinding to correct "
-                        + "index=[" + currentPlayQueueIndex + "], "
-                        + "from=[" + currentPlaylistIndex + "], "
-                        + "size=[" + currentPlaylistSize + "].");
+                Log.d(TAG, "Playback - Rewinding to correct index=[" + playQueueIndex + "], "
+                        + "from=[" + playlistIndex + "], size=[" + playlistSize + "].");
+            }
+
+            if (removeThumbnailBeforeSync) {
+                // unset the current (now outdated) thumbnail to ensure it is not used during sync
+                onThumbnailLoaded(null);
             }
 
+            // sync the player index with the queue index, and seek to the correct position
             if (item.getRecoveryPosition() != PlayQueueItem.RECOVERY_UNSET) {
-                simpleExoPlayer.seekTo(currentPlayQueueIndex, item.getRecoveryPosition());
-                playQueue.unsetRecovery(currentPlayQueueIndex);
+                simpleExoPlayer.seekTo(playQueueIndex, item.getRecoveryPosition());
+                playQueue.unsetRecovery(playQueueIndex);
             } else {
-                simpleExoPlayer.seekToDefaultPosition(currentPlayQueueIndex);
+                simpleExoPlayer.seekToDefaultPosition(playQueueIndex);
             }
         }
     }

app/src/main/java/org/schabi/newpipe/player/notification/NotificationPlayerUi.java

@@ -43,7 +43,7 @@ public void destroy() {
     @Override
     public void onThumbnailLoaded(@Nullable final Bitmap bitmap) {
         super.onThumbnailLoaded(bitmap);
-        notificationUtil.createNotificationIfNeededAndUpdate(false);
+        notificationUtil.updateThumbnail();
     }
 
     @Override

app/src/main/java/org/schabi/newpipe/player/notification/NotificationUtil.java

@@ -24,6 +24,8 @@
 import org.schabi.newpipe.util.NavigationHelper;
 
 import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
 
 import static android.app.PendingIntent.FLAG_UPDATE_CURRENT;
 import static androidx.media.app.NotificationCompat.MediaStyle;
@@ -40,8 +42,6 @@
 
 /**
  * This is a utility class for player notifications.
- *
- * @author cool-student
  */
 public final class NotificationUtil {
     private static final String TAG = NotificationUtil.class.getSimpleName();
@@ -79,6 +79,19 @@ public synchronized void createNotificationIfNeededAndUpdate(final boolean force
         notificationManager.notify(NOTIFICATION_ID, notificationBuilder.build());
     }
 
+    public synchronized void updateThumbnail() {
+        if (notificationBuilder != null) {
+            if (DEBUG) {
+                Log.d(TAG, "updateThumbnail() called with thumbnail = [" + Integer.toHexString(
+                        Optional.ofNullable(player.getThumbnail()).map(Objects::hashCode).orElse(0))
+                        + "], title = [" + player.getVideoTitle() + "]");
+            }
+
+            setLargeIcon(notificationBuilder);
+            notificationManager.notify(NOTIFICATION_ID, notificationBuilder.build());
+        }
+    }
+
     private synchronized NotificationCompat.Builder createNotification() {
         if (DEBUG) {
             Log.d(TAG, "createNotification()");
@@ -123,6 +136,9 @@ private synchronized NotificationCompat.Builder createNotification() {
                 .setDeleteIntent(PendingIntent.getBroadcast(player.getContext(), NOTIFICATION_ID,
                         new Intent(ACTION_CLOSE), FLAG_UPDATE_CURRENT));
 
+        // set the initial value for the video thumbnail, updatable with updateNotificationThumbnail
+        setLargeIcon(builder);
+
         return builder;
     }
 
@@ -142,7 +158,6 @@ private synchronized void updateNotification() {
         notificationBuilder.setTicker(player.getVideoTitle());
 
         updateActions(notificationBuilder);
-        setLargeIcon(notificationBuilder);
     }
 
 

app/src/main/java/org/schabi/newpipe/player/ui/VideoPlayerUi.java

@@ -109,7 +109,6 @@ public abstract class VideoPlayerUi extends PlayerUi
     private final Handler controlsVisibilityHandler = new Handler(Looper.getMainLooper());
     @Nullable private SurfaceHolderCallback surfaceHolderCallback;
     boolean surfaceIsSetup = false;
-    @Nullable private Bitmap thumbnail = null;
 
 
     /*//////////////////////////////////////////////////////////////////////////
@@ -385,9 +384,7 @@ public void destroyPlayer() {
     @Override
     public void destroy() {
         super.destroy();
-        if (binding != null) {
-            binding.endScreen.setImageBitmap(null);
-        }
+        binding.endScreen.setImageDrawable(null);
         deinitPlayerSeekOverlay();
         deinitListeners();
     }
@@ -422,12 +419,10 @@ protected void setupElementsSize(final int buttonsMinWidth,
     public void onBroadcastReceived(final Intent intent) {
         super.onBroadcastReceived(intent);
         if (Intent.ACTION_CONFIGURATION_CHANGED.equals(intent.getAction())) {
-            // When the orientation changed, the screen height might be smaller.
-            // If the end screen thumbnail is not re-scaled,
-            // it can be larger than the current screen height
-            // and thus enlarging the whole player.
-            // This causes the seekbar to be ouf the visible area.
-            updateEndScreenThumbnail();
+            // When the orientation changes, the screen height might be smaller. If the end screen
+            // thumbnail is not re-scaled, it can be larger than the current screen height and thus
+            // enlarging the whole player. This causes the seekbar to be out of the visible area.
+            updateEndScreenThumbnail(player.getThumbnail());
         }
     }
     //endregion
@@ -449,11 +444,10 @@ public void onBroadcastReceived(final Intent intent) {
     @Override
     public void onThumbnailLoaded(@Nullable final Bitmap bitmap) {
         super.onThumbnailLoaded(bitmap);
-        thumbnail = bitmap;
-        updateEndScreenThumbnail();
+        updateEndScreenThumbnail(bitmap);
     }
 
-    private void updateEndScreenThumbnail() {
+    private void updateEndScreenThumbnail(@Nullable final Bitmap thumbnail) {
         if (thumbnail == null) {
             // remove end screen thumbnail
             binding.endScreen.setImageDrawable(null);