From 80e1b69aa7a7df485c85581293c1c984d8941317 Mon Sep 17 00:00:00 2001 From: chelseahao Date: Thu, 26 Sep 2024 20:01:38 +0800 Subject: [PATCH] Create only one media session per audio stream media service. This might cause callback not being cleaned up properly even if the service is destroyed. Test: atest Flag: com.android.settingslib.flags.enable_le_audio_sharing Bug: 362140159 369452993 Change-Id: I389735162c1f9e41cea25f63f881c20be4effb60 --- .../audiostreams/AudioStreamMediaService.java | 175 +++++++++--------- .../audiostreams/AudioStreamsHelper.java | 2 - .../AudioStreamMediaServiceTest.java | 28 +-- 3 files changed, 93 insertions(+), 112 deletions(-) diff --git a/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaService.java b/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaService.java index d5be2bb63fc..d1af8d9d8b7 100644 --- a/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaService.java +++ b/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaService.java @@ -106,7 +106,7 @@ public class AudioStreamMediaService extends Service { // If the initial volume from `onDeviceVolumeChanged` is larger than zero (not muted), we will // override this value. Otherwise, we raise the volume to 25 when the play button is clicked. private final AtomicInteger mLatestPositiveVolume = new AtomicInteger(25); - private final AtomicBoolean mHasStopped = new AtomicBoolean(false); + private final Object mLocalSessionLock = new Object(); private int mBroadcastId; @Nullable private List mDevices; @Nullable private LocalBluetoothManager mLocalBtManager; @@ -125,7 +125,7 @@ public class AudioStreamMediaService extends Service { if (!BluetoothUtils.isAudioSharingEnabled()) { return; } - + Log.d(TAG, "onCreate()"); super.onCreate(); mLocalBtManager = Utils.getLocalBtManager(this); if (mLocalBtManager == null) { @@ -146,26 +146,35 @@ public class AudioStreamMediaService extends Service { return; } - if (mNotificationManager.getNotificationChannel(CHANNEL_ID) == null) { - NotificationChannel notificationChannel = - new NotificationChannel( - CHANNEL_ID, - getString(com.android.settings.R.string.bluetooth), - NotificationManager.IMPORTANCE_HIGH); - mNotificationManager.createNotificationChannel(notificationChannel); - } + mExecutor.execute( + () -> { + if (mLocalBtManager == null + || mLeBroadcastAssistant == null + || mNotificationManager == null) { + return; + } + if (mNotificationManager.getNotificationChannel(CHANNEL_ID) == null) { + NotificationChannel notificationChannel = + new NotificationChannel( + CHANNEL_ID, + getString(com.android.settings.R.string.bluetooth), + NotificationManager.IMPORTANCE_HIGH); + mNotificationManager.createNotificationChannel(notificationChannel); + } - mBluetoothCallback = new BtCallback(); - mLocalBtManager.getEventManager().registerCallback(mBluetoothCallback); + mBluetoothCallback = new BtCallback(); + mLocalBtManager.getEventManager().registerCallback(mBluetoothCallback); - mVolumeControl = mLocalBtManager.getProfileManager().getVolumeControlProfile(); - if (mVolumeControl != null) { - mVolumeControlCallback = new VolumeControlCallback(); - mVolumeControl.registerCallback(mExecutor, mVolumeControlCallback); - } + mVolumeControl = mLocalBtManager.getProfileManager().getVolumeControlProfile(); + if (mVolumeControl != null) { + mVolumeControlCallback = new VolumeControlCallback(); + mVolumeControl.registerCallback(mExecutor, mVolumeControlCallback); + } - mBroadcastAssistantCallback = new AssistantCallback(); - mLeBroadcastAssistant.registerServiceCallBack(mExecutor, mBroadcastAssistantCallback); + mBroadcastAssistantCallback = new AssistantCallback(); + mLeBroadcastAssistant.registerServiceCallBack( + mExecutor, mBroadcastAssistantCallback); + }); } @Override @@ -175,19 +184,29 @@ public class AudioStreamMediaService extends Service { if (!BluetoothUtils.isAudioSharingEnabled()) { return; } - if (mLocalBtManager != null) { - mLocalBtManager.getEventManager().unregisterCallback(mBluetoothCallback); + if (mDevices != null) { + mDevices.clear(); + mDevices = null; } - if (mLeBroadcastAssistant != null && mBroadcastAssistantCallback != null) { - mLeBroadcastAssistant.unregisterServiceCallBack(mBroadcastAssistantCallback); - } - if (mVolumeControl != null && mVolumeControlCallback != null) { - mVolumeControl.unregisterCallback(mVolumeControlCallback); - } - if (mLocalSession != null) { - mLocalSession.release(); - mLocalSession = null; + synchronized (mLocalSessionLock) { + if (mLocalSession != null) { + mLocalSession.release(); + mLocalSession = null; + } } + mExecutor.execute( + () -> { + if (mLocalBtManager != null) { + mLocalBtManager.getEventManager().unregisterCallback(mBluetoothCallback); + } + if (mLeBroadcastAssistant != null && mBroadcastAssistantCallback != null) { + mLeBroadcastAssistant.unregisterServiceCallBack( + mBroadcastAssistantCallback); + } + if (mVolumeControl != null && mVolumeControlCallback != null) { + mVolumeControl.unregisterCallback(mVolumeControlCallback); + } + }); } @Override @@ -195,43 +214,45 @@ public class AudioStreamMediaService extends Service { Log.d(TAG, "onStartCommand()"); if (intent == null) { Log.w(TAG, "Intent is null. Service will not start."); - mHasStopped.set(true); stopSelf(); return START_NOT_STICKY; } mBroadcastId = intent.getIntExtra(BROADCAST_ID, -1); if (mBroadcastId == -1) { Log.w(TAG, "Invalid broadcast ID. Service will not start."); - mHasStopped.set(true); stopSelf(); return START_NOT_STICKY; } var extra = intent.getParcelableArrayListExtra(DEVICES, BluetoothDevice.class); if (extra == null || extra.isEmpty()) { Log.w(TAG, "No device. Service will not start."); - mHasStopped.set(true); stopSelf(); return START_NOT_STICKY; } mDevices = Collections.synchronizedList(extra); - createLocalMediaSession(intent.getStringExtra(BROADCAST_TITLE)); - startForeground(NOTIFICATION_ID, buildNotification()); - // Reset in case the service is previously stopped but not yet destroyed. - mHasStopped.set(false); + MediaSession.Token token = + getOrCreateLocalMediaSession(intent.getStringExtra(BROADCAST_TITLE)); + startForeground(NOTIFICATION_ID, buildNotification(token)); return START_NOT_STICKY; } - private void createLocalMediaSession(String title) { - mLocalSession = new MediaSession(this, TAG); - mLocalSession.setMetadata( - new MediaMetadata.Builder() - .putString(MediaMetadata.METADATA_KEY_TITLE, title) - .putLong(MediaMetadata.METADATA_KEY_DURATION, STATIC_PLAYBACK_DURATION) - .build()); - mLocalSession.setActive(true); - mLocalSession.setPlaybackState(getPlaybackState()); - mMediaSessionCallback = new MediaSessionCallback(); - mLocalSession.setCallback(mMediaSessionCallback); + private MediaSession.Token getOrCreateLocalMediaSession(String title) { + synchronized (mLocalSessionLock) { + if (mLocalSession != null) { + return mLocalSession.getSessionToken(); + } + mLocalSession = new MediaSession(this, TAG); + mLocalSession.setMetadata( + new MediaMetadata.Builder() + .putString(MediaMetadata.METADATA_KEY_TITLE, title) + .putLong(MediaMetadata.METADATA_KEY_DURATION, STATIC_PLAYBACK_DURATION) + .build()); + mLocalSession.setActive(true); + mLocalSession.setPlaybackState(getPlaybackState()); + mMediaSessionCallback = new MediaSessionCallback(); + mLocalSession.setCallback(mMediaSessionCallback); + return mLocalSession.getSessionToken(); + } } private PlaybackState getPlaybackState() { @@ -252,12 +273,9 @@ public class AudioStreamMediaService extends Service { return device != null ? device.getName() : DEFAULT_DEVICE_NAME; } - private Notification buildNotification() { + private Notification buildNotification(MediaSession.Token token) { String deviceName = getDeviceName(); - Notification.MediaStyle mediaStyle = - new Notification.MediaStyle() - .setMediaSession( - mLocalSession != null ? mLocalSession.getSessionToken() : null); + Notification.MediaStyle mediaStyle = new Notification.MediaStyle().setMediaSession(token); if (deviceName != null && !deviceName.isEmpty()) { mediaStyle.setRemotePlaybackInfo( deviceName, com.android.settingslib.R.drawable.ic_bt_le_audio, null); @@ -291,20 +309,15 @@ public class AudioStreamMediaService extends Service { } private void handleRemoveSource() { - var unused = - ThreadUtils.postOnBackgroundThread( - () -> { - List connected = - mAudioStreamsHelper == null - ? emptyList() - : mAudioStreamsHelper.getAllConnectedSources(); - if (connected.stream() - .map(BluetoothLeBroadcastReceiveState::getBroadcastId) - .noneMatch(id -> id == mBroadcastId)) { - mHasStopped.set(true); - stopSelf(); - } - }); + List connected = + mAudioStreamsHelper == null + ? emptyList() + : mAudioStreamsHelper.getAllConnectedSources(); + if (connected.stream() + .map(BluetoothLeBroadcastReceiveState::getBroadcastId) + .noneMatch(id -> id == mBroadcastId)) { + stopSelf(); + } } } @@ -326,7 +339,11 @@ public class AudioStreamMediaService extends Service { mIsMuted.set(false); mLatestPositiveVolume.set(volume); } - updateNotification(getPlaybackState()); + synchronized (mLocalSessionLock) { + if (mLocalSession != null) { + mLocalSession.setPlaybackState(getPlaybackState()); + } + } } } } @@ -336,7 +353,6 @@ public class AudioStreamMediaService extends Service { public void onBluetoothStateChanged(int bluetoothState) { if (BluetoothAdapter.STATE_OFF == bluetoothState) { Log.d(TAG, "onBluetoothStateChanged() : stopSelf"); - mHasStopped.set(true); stopSelf(); } } @@ -362,7 +378,6 @@ public class AudioStreamMediaService extends Service { } if (mDevices == null || mDevices.isEmpty()) { Log.d(TAG, "onProfileConnectionStateChanged() : stopSelf"); - mHasStopped.set(true); stopSelf(); } } @@ -371,7 +386,11 @@ public class AudioStreamMediaService extends Service { private class MediaSessionCallback extends MediaSession.Callback { public void onSeekTo(long pos) { Log.d(TAG, "onSeekTo: " + pos); - updateNotification(getPlaybackState()); + synchronized (mLocalSessionLock) { + if (mLocalSession != null) { + mLocalSession.setPlaybackState(getPlaybackState()); + } + } } @Override @@ -425,18 +444,4 @@ public class AudioStreamMediaService extends Service { }); } } - - private void updateNotification(PlaybackState playbackState) { - var unused = - ThreadUtils.postOnBackgroundThread( - () -> { - if (mLocalSession != null) { - mLocalSession.setPlaybackState(playbackState); - if (mNotificationManager != null && !mHasStopped.get()) { - mNotificationManager.notify( - NOTIFICATION_ID, buildNotification()); - } - } - }); - } } diff --git a/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamsHelper.java b/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamsHelper.java index c0d91626d78..7c1281f7ac5 100644 --- a/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamsHelper.java +++ b/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamsHelper.java @@ -139,7 +139,6 @@ public class AudioStreamsHelper { } /** Retrieves a list of all LE broadcast receive states from active sinks. */ - @VisibleForTesting public List getAllConnectedSources() { if (mLeBroadcastAssistant == null) { Log.w(TAG, "getAllSources(): LeBroadcastAssistant is null!"); @@ -165,7 +164,6 @@ public class AudioStreamsHelper { } /** Retrieves LocalBluetoothLeBroadcastAssistant. */ - @VisibleForTesting @Nullable public LocalBluetoothLeBroadcastAssistant getLeBroadcastAssistant() { return mLeBroadcastAssistant; diff --git a/tests/robotests/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaServiceTest.java b/tests/robotests/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaServiceTest.java index abdd7438ea2..bfb474b711f 100644 --- a/tests/robotests/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaServiceTest.java +++ b/tests/robotests/src/com/android/settings/connecteddevice/audiosharing/audiostreams/AudioStreamMediaServiceTest.java @@ -80,6 +80,7 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; import org.robolectric.RobolectricTestRunner; +import org.robolectric.android.util.concurrent.InlineExecutorService; import org.robolectric.annotation.Config; import org.robolectric.shadow.api.Shadow; import org.robolectric.util.ReflectionHelpers; @@ -143,6 +144,8 @@ public class AudioStreamMediaServiceTest { mAudioStreamMediaService = spy(new AudioStreamMediaService()); ReflectionHelpers.setField(mAudioStreamMediaService, "mBase", mContext); + ReflectionHelpers.setField( + mAudioStreamMediaService, "mExecutor", new InlineExecutorService()); when(mAudioStreamMediaService.getSystemService(anyString())) .thenReturn(mMediaSessionManager); when(mMediaSessionManager.createSession(any(), anyString(), any())).thenReturn(mISession); @@ -352,18 +355,6 @@ public class AudioStreamMediaServiceTest { verify(mAudioStreamMediaService).stopSelf(); } - @Test - public void mediaSessionCallback_onSeekTo_updateNotification() { - mSetFlagsRule.enableFlags(Flags.FLAG_ENABLE_LE_AUDIO_SHARING); - - mAudioStreamMediaService.onCreate(); - mAudioStreamMediaService.onStartCommand(setupIntent(), /* flags= */ 0, /* startId= */ 0); - assertThat(mAudioStreamMediaService.mMediaSessionCallback).isNotNull(); - mAudioStreamMediaService.mMediaSessionCallback.onSeekTo(100); - - verify(mNotificationManager).notify(anyInt(), any()); - } - @Test public void mediaSessionCallback_onPause_setVolume() { mSetFlagsRule.enableFlags(Flags.FLAG_ENABLE_LE_AUDIO_SHARING); @@ -415,19 +406,6 @@ public class AudioStreamMediaServiceTest { eq(SettingsEnums.ACTION_AUDIO_STREAM_NOTIFICATION_LEAVE_BUTTON_CLICK)); } - @Test - public void volumeControlCallback_onDeviceVolumeChanged_updateNotification() { - mSetFlagsRule.enableFlags(Flags.FLAG_ENABLE_LE_AUDIO_SHARING); - - mAudioStreamMediaService.onCreate(); - assertThat(mAudioStreamMediaService.mVolumeControlCallback).isNotNull(); - mAudioStreamMediaService.onStartCommand(setupIntent(), /* flags= */ 0, /* startId= */ 0); - mAudioStreamMediaService.mVolumeControlCallback.onDeviceVolumeChanged( - mDevice, /* volume= */ 0); - - verify(mNotificationManager).notify(anyInt(), any()); - } - @Test public void onBind_returnNull() { IBinder binder = mAudioStreamMediaService.onBind(new Intent());