Remove some misleading locks in MediaSessionService

The stub, mediaNotificationManager and actionFactory fields were
already only allowed to be accessed on the main thread, so need to
lock any access to them. Also add a corresponding note to methods
that were already meant to be called on the main thread only, but
didn't have the corresponding note in the Javadoc yet.

PiperOrigin-RevId: 746440272

.github/ISSUE_TEMPLATE/bug.yml

@@ -19,6 +19,7 @@ body:
       options:
         - Media3 main branch
         - Media3 pre-release (alpha, beta or RC not in this list)
+        - Media3 1.6.1
         - Media3 1.6.0
         - Media3 1.5.1
         - Media3 1.5.0
@@ -44,9 +45,6 @@ body:
         - ExoPlayer 2.16.0
         - ExoPlayer 2.15.1
         - ExoPlayer 2.15.0
-        - ExoPlayer 2.14.2
-        - ExoPlayer 2.14.1
-        - ExoPlayer 2.14.0
         - ExoPlayer dev-v2 branch
         - Older (unsupported)
     validations:

RELEASENOTES.md

@@ -73,6 +73,11 @@
         player doesn't have `COMMAND_GET_TIMELINE` available while
         `COMMAND_GET_CURRENT_MEDIA_ITEM` is available and the wrapped player is
         empty ([#2320](https://github.com/androidx/media/issues/2320)).
+    *   Fix a bug where calling
+        `MediaSessionService.setMediaNotificationProvider` is silently ignored
+        after other interactions with the service like
+        `setForegroundServiceTimeoutMs`
+        ([#2305](https://github.com/androidx/media/issues/2305)).
 *   UI:
     *   Enable `PlayerSurface` to work with `ExoPlayer.setVideoEffects` and
         `CompositionPlayer`.

constants.gradle

@@ -12,8 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 project.ext {
-    releaseVersion = '1.6.0'
+    releaseVersion = '1.6.1'
-    releaseVersionCode = 1_006_000_3_00
+    releaseVersionCode = 1_006_001_3_00
     minSdkVersion = 21
     // See https://developer.android.com/training/cars/media/automotive-os#automotive-module
     automotiveMinSdkVersion = 28

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

@@ -29,11 +29,11 @@ public final class MediaLibraryInfo {
 
   /** The version of the library expressed as a string, for example "1.2.3" or "1.2.0-beta01". */
   // Intentionally hardcoded. Do not derive from other constants (e.g. VERSION_INT) or vice versa.
-  public static final String VERSION = "1.6.0";
+  public static final String VERSION = "1.6.1";
 
   /** The version of the library expressed as {@code TAG + "/" + VERSION}. */
   // Intentionally hardcoded. Do not derive from other constants (e.g. VERSION) or vice versa.
-  public static final String VERSION_SLASHY = "AndroidXMedia3/1.6.0";
+  public static final String VERSION_SLASHY = "AndroidXMedia3/1.6.1";
 
   /**
    * The version of the library expressed as an integer, for example 1002003300.
@@ -47,7 +47,7 @@ public final class MediaLibraryInfo {
    * (123-045-006-3-00).
    */
   // Intentionally hardcoded. Do not derive from other constants (e.g. VERSION) or vice versa.
-  public static final int VERSION_INT = 1_006_000_3_00;
+  public static final int VERSION_INT = 1_006_001_3_00;
 
   /** Whether the library was compiled with {@link Assertions} checks enabled. */
   public static final boolean ASSERTIONS_ENABLED = true;

libraries/session/src/main/java/androidx/media3/session/MediaNotificationManager.java

@@ -60,7 +60,7 @@ import java.util.concurrent.TimeoutException;
   private static final int MSG_USER_ENGAGED_TIMEOUT = 1;
 
   private final MediaSessionService mediaSessionService;
-  private final MediaNotification.Provider mediaNotificationProvider;
+
   private final MediaNotification.ActionFactory actionFactory;
   private final NotificationManagerCompat notificationManagerCompat;
   private final Handler mainHandler;
@@ -68,6 +68,7 @@ import java.util.concurrent.TimeoutException;
   private final Intent startSelfIntent;
   private final Map<MediaSession, ControllerInfo> controllerMap;
 
+  private MediaNotification.Provider mediaNotificationProvider;
   private int totalNotificationCount;
   @Nullable private MediaNotification mediaNotification;
   private boolean startedInForeground;
@@ -146,6 +147,15 @@ import java.util.concurrent.TimeoutException;
         });
   }
 
+  /**
+   * Updates the media notification provider.
+   *
+   * @param mediaNotificationProvider The {@link MediaNotification.Provider}.
+   */
+  public void setMediaNotificationProvider(MediaNotification.Provider mediaNotificationProvider) {
+    this.mediaNotificationProvider = mediaNotificationProvider;
+  }
+
   /**
    * Updates the notification.
    *

libraries/session/src/main/java/androidx/media3/session/MediaSessionService.java

@@ -169,23 +169,13 @@ public abstract class MediaSessionService extends Service {
 
   private final Object lock;
   private final Handler mainHandler;
+  @Nullable private MediaSessionServiceStub stub;
+  private @MonotonicNonNull MediaNotificationManager mediaNotificationManager;
+  private @MonotonicNonNull DefaultActionFactory actionFactory;
 
   @GuardedBy("lock")
   private final Map<String, MediaSession> sessions;
 
-  @GuardedBy("lock")
-  @Nullable
-  private MediaSessionServiceStub stub;
-
-  @GuardedBy("lock")
-  private @MonotonicNonNull MediaNotificationManager mediaNotificationManager;
-
-  @GuardedBy("lock")
-  private MediaNotification.@MonotonicNonNull Provider mediaNotificationProvider;
-
-  @GuardedBy("lock")
-  private @MonotonicNonNull DefaultActionFactory actionFactory;
-
   @GuardedBy("lock")
   @Nullable
   private Listener listener;
@@ -211,9 +201,7 @@ public abstract class MediaSessionService extends Service {
   @Override
   public void onCreate() {
     super.onCreate();
-    synchronized (lock) {
+    stub = new MediaSessionServiceStub(this);
-      stub = new MediaSessionServiceStub(this);
-    }
   }
 
   /**
@@ -277,11 +265,10 @@ public abstract class MediaSessionService extends Service {
     if (old == null) {
       // Session has returned for the first time. Register callbacks.
       // TODO(b/191644474): Check whether the session is registered to multiple services.
-      MediaNotificationManager notificationManager = getMediaNotificationManager();
       postOrRun(
           mainHandler,
           () -> {
-            notificationManager.addSession(session);
+            getMediaNotificationManager().addSession(session);
             session.setListener(new MediaSessionListener());
           });
     }
@@ -303,11 +290,10 @@ public abstract class MediaSessionService extends Service {
       checkArgument(sessions.containsKey(session.getId()), "session not found");
       sessions.remove(session.getId());
     }
-    MediaNotificationManager notificationManager = getMediaNotificationManager();
     postOrRun(
         mainHandler,
         () -> {
-          notificationManager.removeSession(session);
+          getMediaNotificationManager().removeSession(session);
           session.clearListener();
         });
   }
@@ -489,6 +475,8 @@ public abstract class MediaSessionService extends Service {
    * <p>The default and maximum value is {@link #DEFAULT_FOREGROUND_SERVICE_TIMEOUT_MS}. If a larger
    * value is provided, it will be clamped down to {@link #DEFAULT_FOREGROUND_SERVICE_TIMEOUT_MS}.
    *
+   * <p>This method must be called on the main thread.
+   *
    * @param foregroundServiceTimeoutMs The timeout in milliseconds.
    */
   @UnstableApi
@@ -512,6 +500,8 @@ public abstract class MediaSessionService extends Service {
    * {@linkplain #setForegroundServiceTimeoutMs foreground service timeout} after they paused,
    * stopped, failed or ended. Use {@link #pauseAllPlayersAndStopSelf()} to pause all ongoing
    * playbacks immediately and terminate the service.
+   *
+   * <p>This method must be called on the main thread.
    */
   @UnstableApi
   public boolean isPlaybackOngoing() {
@@ -524,6 +514,8 @@ public abstract class MediaSessionService extends Service {
    *
    * <p>This terminates the service lifecycle and triggers {@link #onDestroy()} that an app can
    * override to release the sessions and other resources.
+   *
+   * <p>This method must be called on the main thread.
    */
   @UnstableApi
   public void pauseAllPlayersAndStopSelf() {
@@ -583,11 +575,9 @@ public abstract class MediaSessionService extends Service {
   @Override
   public void onDestroy() {
     super.onDestroy();
-    synchronized (lock) {
+    if (stub != null) {
-      if (stub != null) {
+      stub.release();
-        stub.release();
+      stub = null;
-        stub = null;
-      }
     }
   }
 
@@ -637,23 +627,22 @@ public abstract class MediaSessionService extends Service {
   /**
    * Sets the {@link MediaNotification.Provider} to customize notifications.
    *
-   * <p>This should be called before {@link #onCreate()} returns.
-   *
    * <p>This method can be called from any thread.
    */
   @UnstableApi
   protected final void setMediaNotificationProvider(
       MediaNotification.Provider mediaNotificationProvider) {
     checkNotNull(mediaNotificationProvider);
-    synchronized (lock) {
+    Util.postOrRun(
-      this.mediaNotificationProvider = mediaNotificationProvider;
+        mainHandler,
-    }
+        () ->
+            getMediaNotificationManager(
+                    /* initialMediaNotificationProvider= */ mediaNotificationProvider)
+                .setMediaNotificationProvider(mediaNotificationProvider));
   }
 
   /* package */ IBinder getServiceBinder() {
-    synchronized (lock) {
+    return checkStateNotNull(stub).asBinder();
-      return checkStateNotNull(stub).asBinder();
-    }
   }
 
   /**
@@ -679,28 +668,31 @@ public abstract class MediaSessionService extends Service {
   }
 
   private MediaNotificationManager getMediaNotificationManager() {
-    synchronized (lock) {
+    return getMediaNotificationManager(/* initialMediaNotificationProvider= */ null);
-      if (mediaNotificationManager == null) {
+  }
-        if (mediaNotificationProvider == null) {
+
-          checkStateNotNull(getBaseContext(), "Accessing service context before onCreate()");
+  private MediaNotificationManager getMediaNotificationManager(
-          mediaNotificationProvider =
+      @Nullable MediaNotification.Provider initialMediaNotificationProvider) {
-              new DefaultMediaNotificationProvider.Builder(getApplicationContext()).build();
+    if (mediaNotificationManager == null) {
-        }
+      if (initialMediaNotificationProvider == null) {
-        mediaNotificationManager =
+        checkStateNotNull(getBaseContext(), "Accessing service context before onCreate()");
-            new MediaNotificationManager(
+        initialMediaNotificationProvider =
-                /* mediaSessionService= */ this, mediaNotificationProvider, getActionFactory());
+            new DefaultMediaNotificationProvider.Builder(getApplicationContext()).build();
       }
-      return mediaNotificationManager;
+      mediaNotificationManager =
+          new MediaNotificationManager(
+              /* mediaSessionService= */ this,
+              initialMediaNotificationProvider,
+              getActionFactory());
     }
+    return mediaNotificationManager;
   }
 
   private DefaultActionFactory getActionFactory() {
-    synchronized (lock) {
+    if (actionFactory == null) {
-      if (actionFactory == null) {
+      actionFactory = new DefaultActionFactory(/* service= */ this);
-        actionFactory = new DefaultActionFactory(/* service= */ this);
-      }
-      return actionFactory;
     }
+    return actionFactory;
   }
 
   @Nullable

libraries/session/src/test/java/androidx/media3/session/MediaSessionServiceTest.java

@@ -504,6 +504,36 @@ public class MediaSessionServiceTest {
     serviceController.destroy();
   }
 
+  @Test
+  public void setMediaNotificationProvider_afterSetForegroundServiceTimeoutMs_usesCustomProvider()
+      throws TimeoutException {
+    Context context = ApplicationProvider.getApplicationContext();
+    ExoPlayer player = new TestExoPlayerBuilder(context).build();
+    MediaSession session = new MediaSession.Builder(context, player).build();
+    ServiceController<TestService> serviceController = Robolectric.buildService(TestService.class);
+    TestService service = serviceController.create().get();
+
+    service.setForegroundServiceTimeoutMs(100);
+    service.setMediaNotificationProvider(
+        new DefaultMediaNotificationProvider(
+            service,
+            /* notificationIdProvider= */ mediaSession -> 2000,
+            DefaultMediaNotificationProvider.DEFAULT_CHANNEL_ID,
+            DefaultMediaNotificationProvider.DEFAULT_CHANNEL_NAME_RESOURCE_ID));
+    service.addSession(session);
+    // Start a player to trigger notification creation.
+    player.setMediaItem(MediaItem.fromUri("asset:///media/mp4/sample.mp4"));
+    player.prepare();
+    player.play();
+    runMainLooperUntil(() -> notificationManager.getActiveNotifications().length == 1);
+
+    assertThat(getStatusBarNotification(/* notificationId= */ 2000)).isNotNull();
+
+    session.release();
+    player.release();
+    serviceController.destroy();
+  }
+
   @Test
   public void onStartCommand_mediaButtonEvent_pausedByMediaNotificationController()
       throws InterruptedException {