Skip to content

Commit

Permalink
Fix thread access when creating notifications for media sessions
Browse files Browse the repository at this point in the history 
The sessions may have different application threads for their players,
and the service with its notification provider runs on the main thread.
To ensure everything runs on the correct thread, this change labels
methods where needed and fixes thread access in some places.

Issue: #318
PiperOrigin-RevId: 524849598
  • Loading branch information
@tonihei @rohitjoins
tonihei authored and rohitjoins committed Apr 17, 2023
1 parent 9081c70 commit ffa3743
Show file tree
Hide file tree
Showing 10 changed files with 341 additions and 97 deletions.
3 changes: 3 additions & 0 deletions RELEASENOTES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@
([#296](https://github.com/androidx/media/issues/296)).
* Fix issue where `Player.COMMAND_GET_CURRENT_MEDIA_ITEM` needs to be
available to access metadata via the legacy `MediaSessionCompat`.
* Fix issue where `MediaSession` instances on a background thread cause
crashes when used in `MediaSessionService`
([#318](https://github.com/androidx/media/issues/318)).
* Audio:
* Fix bug where some playbacks fail when tunneling is enabled and
`AudioProcessors` are active, e.g. for gapless trimming
Expand Down
1 change: 1 addition & 0 deletions libraries/session/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ dependencies {
androidTestImplementation 'androidx.multidex:multidex:' + androidxMultidexVersion
androidTestImplementation 'androidx.test:runner:' + androidxTestRunnerVersion
testImplementation project(modulePrefix + 'test-utils')
testImplementation project(modulePrefix + 'test-utils-robolectric')
testImplementation project(modulePrefix + 'lib-exoplayer')
testImplementation 'org.robolectric:robolectric:' + robolectricVersion
}
Expand Down
6 changes: 1 addition & 5 deletions ...aries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@
import android.content.Context;
import android.graphics.Bitmap;
import android.os.Bundle;
import android.os.Handler;
import android.os.Looper;
import androidx.annotation.DoNotInline;
import androidx.annotation.DrawableRes;
import androidx.annotation.Nullable;
Expand Down Expand Up @@ -245,7 +243,6 @@ public interface NotificationIdProvider {
private final String channelId;
@StringRes private final int channelNameResourceId;
private final NotificationManager notificationManager;
private final Handler mainHandler;

private @MonotonicNonNull OnBitmapLoadedFutureCallback pendingOnBitmapLoadedFutureCallback;
@DrawableRes private int smallIconResourceId;
Expand Down Expand Up @@ -278,7 +275,6 @@ public DefaultMediaNotificationProvider(
notificationManager =
checkStateNotNull(
(NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE));
mainHandler = new Handler(Looper.getMainLooper());
smallIconResourceId = R.drawable.media3_notification_small_icon;
}

Expand Down Expand Up @@ -346,7 +342,7 @@ public final MediaNotification createNotification(
pendingOnBitmapLoadedFutureCallback,
// This callback must be executed on the next looper iteration, after this method has
// returned a media notification.
mainHandler::post);
mediaSession.getImpl().getApplicationHandler()::post);
}
}
}
Expand Down
16 changes: 15 additions & 1 deletion libraries/session/src/main/java/androidx/media3/session/MediaNotification.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ public final class MediaNotification {
/**
* Creates {@linkplain NotificationCompat.Action actions} and {@linkplain PendingIntent pending
* intents} for notifications.
*
* <p>All methods will be called on the {@link Player#getApplicationLooper() application thread}
* of the {@link Player} associated with the {@link MediaSession} the notification is provided
* for.
*/
@UnstableApi
public interface ActionFactory {
Expand Down Expand Up @@ -109,10 +113,20 @@ PendingIntent createMediaActionPendingIntent(
*
* <p>The provider is required to create a {@linkplain androidx.core.app.NotificationChannelCompat
* notification channel}, which is required to show notification for {@code SDK_INT >= 26}.
*
* <p>All methods will be called on the {@link Player#getApplicationLooper() application thread}
* of the {@link Player} associated with the {@link MediaSession} the notification is provided
* for.
*/
@UnstableApi
public interface Provider {
/** Receives updates for a notification. */
/**
* Receives updates for a notification.
*
* <p>All methods will be called on the {@link Player#getApplicationLooper() application thread}
* of the {@link Player} associated with the {@link MediaSession} the notification is provided
* for.
*/
interface Callback {
/**
* Called when a {@link MediaNotification} is changed.
Expand Down
142 changes: 89 additions & 53 deletions libraries/session/src/main/java/androidx/media3/session/MediaNotificationManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
/**
* Manages media notifications for a {@link MediaSessionService} and sets the service as
* foreground/background according to the player state.
*
* <p>All methods must be called on the main thread.
*/
/* package */ final class MediaNotificationManager {

Expand Down Expand Up @@ -96,11 +98,12 @@ public void addSession(MediaSession session) {
.setListener(listener)
.setApplicationLooper(Looper.getMainLooper())
.buildAsync();
controllerMap.put(session, controllerFuture);
controllerFuture.addListener(
() -> {
try {
MediaController controller = controllerFuture.get(/* time= */ 0, TimeUnit.MILLISECONDS);
listener.onConnected();
listener.onConnected(shouldShowNotification(session));
controller.addListener(listener);
} catch (CancellationException
| ExecutionException
Expand All @@ -111,7 +114,6 @@ public void addSession(MediaSession session) {
}
},
mainExecutor);
controllerMap.put(session, controllerFuture);
}

public void removeSession(MediaSession session) {
Expand All @@ -123,46 +125,19 @@ public void removeSession(MediaSession session) {
}

public void onCustomAction(MediaSession session, String action, Bundle extras) {
@Nullable ListenableFuture<MediaController> controllerFuture = controllerMap.get(session);
if (controllerFuture == null) {
@Nullable MediaController mediaController = getConnectedControllerForSession(session);
if (mediaController == null) {
return;
}
try {
MediaController mediaController = checkStateNotNull(Futures.getDone(controllerFuture));
if (!mediaNotificationProvider.handleCustomCommand(session, action, extras)) {
@Nullable SessionCommand customCommand = null;
for (SessionCommand command : mediaController.getAvailableSessionCommands().commands) {
if (command.commandCode == SessionCommand.COMMAND_CODE_CUSTOM
&& command.customAction.equals(action)) {
customCommand = command;
break;
// Let the notification provider handle the command first before forwarding it directly.
Util.postOrRun(
new Handler(session.getPlayer().getApplicationLooper()),
() -> {
if (!mediaNotificationProvider.handleCustomCommand(session, action, extras)) {
mainExecutor.execute(
() -> sendCustomCommandIfCommandIsAvailable(mediaController, action));
}
}
if (customCommand != null
&& mediaController.getAvailableSessionCommands().contains(customCommand)) {
ListenableFuture<SessionResult> future =
mediaController.sendCustomCommand(customCommand, Bundle.EMPTY);
Futures.addCallback(
future,
new FutureCallback<SessionResult>() {
@Override
public void onSuccess(SessionResult result) {
// Do nothing.
}

@Override
public void onFailure(Throwable t) {
Log.w(
TAG, "custom command " + action + " produced an error: " + t.getMessage(), t);
}
},
MoreExecutors.directExecutor());
}
}
} catch (ExecutionException e) {
// We should never reach this.
throw new IllegalStateException(e);
}
});
}

/**
Expand All @@ -178,27 +153,42 @@ public void updateNotification(MediaSession session, boolean startInForegroundRe
}

int notificationSequence = ++totalNotificationCount;
ImmutableList<CommandButton> customLayout = checkStateNotNull(customLayoutMap.get(session));
MediaNotification.Provider.Callback callback =
notification ->
mainExecutor.execute(
() -> onNotificationUpdated(notificationSequence, session, notification));

MediaNotification mediaNotification =
this.mediaNotificationProvider.createNotification(
session, checkStateNotNull(customLayoutMap.get(session)), actionFactory, callback);
updateNotificationInternal(session, mediaNotification, startInForegroundRequired);
Util.postOrRun(
new Handler(session.getPlayer().getApplicationLooper()),
() -> {
MediaNotification mediaNotification =
this.mediaNotificationProvider.createNotification(
session, customLayout, actionFactory, callback);
mainExecutor.execute(
() ->
updateNotificationInternal(
session, mediaNotification, startInForegroundRequired));
});
}

public boolean isStartedInForeground() {
return startedInForeground;
}

/* package */ boolean shouldRunInForeground(
MediaSession session, boolean startInForegroundWhenPaused) {
@Nullable MediaController controller = getConnectedControllerForSession(session);
return controller != null
&& (controller.getPlayWhenReady() || startInForegroundWhenPaused)
&& (controller.getPlaybackState() == Player.STATE_READY
|| controller.getPlaybackState() == Player.STATE_BUFFERING);
}

private void onNotificationUpdated(
int notificationSequence, MediaSession session, MediaNotification mediaNotification) {
if (notificationSequence == totalNotificationCount) {
boolean startInForegroundRequired =
MediaSessionService.shouldRunInForeground(
session, /* startInForegroundWhenPaused= */ false);
shouldRunInForeground(session, /* startInForegroundWhenPaused= */ false);
updateNotificationInternal(session, mediaNotification, startInForegroundRequired);
}
}
Expand Down Expand Up @@ -236,8 +226,7 @@ private void updateNotificationInternal(
private void maybeStopForegroundService(boolean removeNotifications) {
List<MediaSession> sessions = mediaSessionService.getSessions();
for (int i = 0; i < sessions.size(); i++) {
if (MediaSessionService.shouldRunInForeground(
sessions.get(i), /* startInForegroundWhenPaused= */ false)) {
if (shouldRunInForeground(sessions.get(i), /* startInForegroundWhenPaused= */ false)) {
return;
}
}
Expand All @@ -251,9 +240,56 @@ private void maybeStopForegroundService(boolean removeNotifications) {
}
}

private static boolean shouldShowNotification(MediaSession session) {
Player player = session.getPlayer();
return !player.getCurrentTimeline().isEmpty() && player.getPlaybackState() != Player.STATE_IDLE;
private boolean shouldShowNotification(MediaSession session) {
MediaController controller = getConnectedControllerForSession(session);
return controller != null
&& !controller.getCurrentTimeline().isEmpty()
&& controller.getPlaybackState() != Player.STATE_IDLE;
}

@Nullable
private MediaController getConnectedControllerForSession(MediaSession session) {
@Nullable ListenableFuture<MediaController> controllerFuture = controllerMap.get(session);
if (controllerFuture == null) {
return null;
}
try {
return Futures.getDone(controllerFuture);
} catch (ExecutionException exception) {
// We should never reach this.
throw new IllegalStateException(exception);
}
}

private void sendCustomCommandIfCommandIsAvailable(
MediaController mediaController, String action) {
@Nullable SessionCommand customCommand = null;
for (SessionCommand command : mediaController.getAvailableSessionCommands().commands) {
if (command.commandCode == SessionCommand.COMMAND_CODE_CUSTOM
&& command.customAction.equals(action)) {
customCommand = command;
break;
}
}
if (customCommand != null
&& mediaController.getAvailableSessionCommands().contains(customCommand)) {
ListenableFuture<SessionResult> future =
mediaController.sendCustomCommand(customCommand, Bundle.EMPTY);
Futures.addCallback(
future,
new FutureCallback<SessionResult>() {
@Override
public void onSuccess(SessionResult result) {
// Do nothing.
}

@Override
public void onFailure(Throwable t) {
Log.w(TAG, "custom command " + action + " produced an error: " + t.getMessage(), t);
}
},
MoreExecutors.directExecutor());
}
}

private static final class MediaControllerListener
Expand All @@ -271,8 +307,8 @@ public MediaControllerListener(
this.customLayoutMap = customLayoutMap;
}

public void onConnected() {
if (shouldShowNotification(session)) {
public void onConnected(boolean shouldShowNotification) {
if (shouldShowNotification) {
mediaSessionService.onUpdateNotificationInternal(
session, /* startInForegroundWhenPaused= */ false);
}
Expand Down
18 changes: 15 additions & 3 deletions libraries/session/src/main/java/androidx/media3/session/MediaSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -902,12 +902,20 @@ public final MediaSessionCompat.Token getSessionCompatToken() {
impl.setSessionPositionUpdateDelayMsOnHandler(updateDelayMs);
}

/** Sets the {@linkplain Listener listener}. */
/**
* Sets the {@linkplain Listener listener}.
*
* <p>This method must be called on the main thread.
*/
/* package */ final void setListener(Listener listener) {
impl.setMediaSessionListener(listener);
}

/** Clears the {@linkplain Listener listener}. */
/**
* Clears the {@linkplain Listener listener}.
*
* <p>This method must be called on the main thread.
*/
/* package */ final void clearListener() {
impl.clearMediaSessionListener();
}
Expand Down Expand Up @@ -1439,7 +1447,11 @@ default void onMediaMetadataChanged(int seq, MediaMetadata mediaMetadata)
default void onRenderedFirstFrame(int seq) throws RemoteException {}
}

/** Listener for media session events */
/**
* Listener for media session events.
*
* <p>All methods must be called on the main thread.
*/
/* package */ interface Listener {

/**
Expand Down
24 changes: 21 additions & 3 deletions libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,11 @@
import com.google.common.collect.ImmutableList;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.SettableFuture;
import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.ExecutionException;
import org.checkerframework.checker.initialization.qual.Initialized;

/* package */ class MediaSessionImpl {
Expand Down Expand Up @@ -113,8 +115,10 @@
private final Handler applicationHandler;
private final BitmapLoader bitmapLoader;
private final Runnable periodicSessionPositionInfoUpdateRunnable;
private final Handler mainHandler;

@Nullable private PlayerListener playerListener;

@Nullable private MediaSession.Listener mediaSessionListener;

private PlayerInfo playerInfo;
Expand Down Expand Up @@ -149,6 +153,7 @@ public MediaSessionImpl(
sessionStub = new MediaSessionStub(thisRef);
this.sessionActivity = sessionActivity;

mainHandler = new Handler(Looper.getMainLooper());
applicationHandler = new Handler(player.getApplicationLooper());
this.callback = callback;
this.bitmapLoader = bitmapLoader;
Expand Down Expand Up @@ -546,12 +551,25 @@ protected MediaSessionServiceLegacyStub getLegacyBrowserService() {
}

/* package */ void onNotificationRefreshRequired() {
if (this.mediaSessionListener != null) {
this.mediaSessionListener.onNotificationRefreshRequired(instance);
}
postOrRun(
mainHandler,
() -> {
if (this.mediaSessionListener != null) {
this.mediaSessionListener.onNotificationRefreshRequired(instance);
}
});
}

/* package */ boolean onPlayRequested() {
if (Looper.myLooper() != Looper.getMainLooper()) {
SettableFuture<Boolean> playRequested = SettableFuture.create();
mainHandler.post(() -> playRequested.set(onPlayRequested()));
try {
return playRequested.get();
} catch (InterruptedException | ExecutionException e) {
throw new IllegalStateException(e);
}
}
if (this.mediaSessionListener != null) {
return this.mediaSessionListener.onPlayRequested(instance);
}
Expand Down
Loading

0 comments on commit ffa3743

Please sign in to comment.