From df8763ae0d295b16921a3eab312b98c0294a0b29 Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 11 Apr 2025 06:40:14 -0700 Subject: [PATCH] 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 --- .../media3/session/MediaSessionService.java | 83 ++++++++----------- 1 file changed, 36 insertions(+), 47 deletions(-) diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionService.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionService.java index 35107f19bc..c31270dd1b 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionService.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionService.java @@ -169,20 +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 sessions; - @GuardedBy("lock") - @Nullable - private MediaSessionServiceStub stub; - - @GuardedBy("lock") - private @MonotonicNonNull MediaNotificationManager mediaNotificationManager; - - @GuardedBy("lock") - private @MonotonicNonNull DefaultActionFactory actionFactory; - @GuardedBy("lock") @Nullable private Listener listener; @@ -208,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); } /** @@ -274,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()); }); } @@ -300,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(); }); } @@ -486,6 +475,8 @@ public abstract class MediaSessionService extends Service { *

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}. * + *

This method must be called on the main thread. + * * @param foregroundServiceTimeoutMs The timeout in milliseconds. */ @UnstableApi @@ -509,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. + * + *

This method must be called on the main thread. */ @UnstableApi public boolean isPlaybackOngoing() { @@ -521,6 +514,8 @@ public abstract class MediaSessionService extends Service { * *

This terminates the service lifecycle and triggers {@link #onDestroy()} that an app can * override to release the sessions and other resources. + * + *

This method must be called on the main thread. */ @UnstableApi public void pauseAllPlayersAndStopSelf() { @@ -580,11 +575,9 @@ public abstract class MediaSessionService extends Service { @Override public void onDestroy() { super.onDestroy(); - synchronized (lock) { - if (stub != null) { - stub.release(); - stub = null; - } + if (stub != null) { + stub.release(); + stub = null; } } @@ -640,16 +633,16 @@ public abstract class MediaSessionService extends Service { protected final void setMediaNotificationProvider( MediaNotification.Provider mediaNotificationProvider) { checkNotNull(mediaNotificationProvider); - synchronized (lock) { - getMediaNotificationManager(/* initialMediaNotificationProvider= */ mediaNotificationProvider) - .setMediaNotificationProvider(mediaNotificationProvider); - } + Util.postOrRun( + mainHandler, + () -> + getMediaNotificationManager( + /* initialMediaNotificationProvider= */ mediaNotificationProvider) + .setMediaNotificationProvider(mediaNotificationProvider)); } /* package */ IBinder getServiceBinder() { - synchronized (lock) { - return checkStateNotNull(stub).asBinder(); - } + return checkStateNotNull(stub).asBinder(); } /** @@ -680,30 +673,26 @@ public abstract class MediaSessionService extends Service { private MediaNotificationManager getMediaNotificationManager( @Nullable MediaNotification.Provider initialMediaNotificationProvider) { - synchronized (lock) { - if (mediaNotificationManager == null) { - if (initialMediaNotificationProvider == null) { - checkStateNotNull(getBaseContext(), "Accessing service context before onCreate()"); - initialMediaNotificationProvider = - new DefaultMediaNotificationProvider.Builder(getApplicationContext()).build(); - } - mediaNotificationManager = - new MediaNotificationManager( - /* mediaSessionService= */ this, - initialMediaNotificationProvider, - getActionFactory()); + if (mediaNotificationManager == null) { + if (initialMediaNotificationProvider == null) { + checkStateNotNull(getBaseContext(), "Accessing service context before onCreate()"); + initialMediaNotificationProvider = + new DefaultMediaNotificationProvider.Builder(getApplicationContext()).build(); } - return mediaNotificationManager; + mediaNotificationManager = + new MediaNotificationManager( + /* mediaSessionService= */ this, + initialMediaNotificationProvider, + getActionFactory()); } + return mediaNotificationManager; } private DefaultActionFactory getActionFactory() { - synchronized (lock) { - if (actionFactory == null) { - actionFactory = new DefaultActionFactory(/* service= */ this); - } - return actionFactory; + if (actionFactory == null) { + actionFactory = new DefaultActionFactory(/* service= */ this); } + return actionFactory; } @Nullable