androidx / media

Jetpack Media3 support libraries for media use cases, including ExoPlayer, an extensible media player for Android
https://developer.android.com/media/media3
Apache License 2.0
1.59k stars 377 forks source link

Killing the app while audio is playing results in "The controller is not connected" and not being able to play again #1059

Open k-arabadzhiev opened 8 months ago

k-arabadzhiev commented 8 months ago

Version

Media3 1.2.0

More version details

No response

Devices that reproduce the issue

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Not tested

Reproduction steps

  1. Open app and play audio
  2. Kill the app (swipe from recents)
  3. Reopen the app and try to play audio again

Expected result

The audio should play normmally

Actual result

Audio doesn't play. Logs:

MediaController      The controller is not connected. Ignoring setMediaItem().
MediaController      The controller is not connected. Ignoring prepare().

If the app is killed a second time, then the audio will play normally. However the issue can be reproduced again.

Media

Not applicable

Bug Report

k-arabadzhiev commented 8 months ago

A bit more context.

Our app has been migrating to media3 for some time now and we are facing a weird issue with MusicServiceConnection when using dagger2 singleton injection. I'm not absolutely certain that the issue is in media3, but it wasn't happening with androidx.media that we had previously. Also I was able to reproduce the issue (to some degree) using UAMP and I'll explain the differences with our app as in depth as I can.

The issue

Killing the app (swipe from recents) while audio is playing, reopening and trying to play again will result in audio not being able to start.

Here is what I found in the logs on the second app launch:

MediaController      The controller is not connected. Ignoring setMediaItem().
MediaController      The controller is not connected. Ignoring prepare().

Killing the app a second time, reopening and trying to play will work.

Pausing the app manually (from the app itself or notification player) before killing it will not cause the issue. However, pausing the player in code (in onTaskRemoved for example) will not solve the issue.

The workaround that we're currently using is to not use dagger's singleton scope, but instead instantiate the MusicServiceConnection using companion object like it's done in UAMP

Here is my fork of UAMP where I've reproduced the issue. In this branch a lot of libraries are updated, including media3 and dagger2 dependency injection is implemented as well.

UAMP is architecturally very different from our app, because the whole UI depends on the root media item. So after killing the app while the audio is playing the result will be that the UI is not even rendering. I think that's the reason I can't see in the logs The controller is not connected - because I can't even get to play anything, but the root cause of the issue should be the same.

Any idea why is this happening? Why singleton injection doesn't work, but the companion object snippet that looks like a hack is working?

marcbaechinger commented 7 months ago

Difficult to say with the available information.

Can you add an EventLogger to the player and then do a bug report after you went through the repro process of 1) starting the app, 2) swiping from recent tasks while playing 3) restart the app that produces the controller not connected logs then 4) pausing the player and swipe away and 5) restarting to play again?

It would also be interesting to see what adb shell dumpsys media_session gives you after 2) and before 3).

k-arabadzhiev commented 7 months ago

Hello @marcbaechinger
I've sent an email to android-media-github@google.com that includes the bugreport and also shell dumpsys media_session.

Also I've noticed something in logcat, that I'm not sure will be obvious in the bug report, but might be worth mentioning. It's in the email as well, but I'm including it here as it doesn't contain sensitive information and might be useful to someone coming to this issue in the future.

For brevity, let's separate the steps from Marc's comment in 2 different scenarios.

Scenario 1 (results in audio not playing)

  1. Start the app and play audio
  2. Swipe from recents while playing
  3. Reopen the app and playing again doesn't work (controller not connected)
  4. Swipe from recents again
  5. Reopen the app and playing will work

To keep everything clean and consistent, I'm starting with force closed app. Logs:

---------------------------- PROCESS STARTED (9502) for package com.example.app ---------------------------- // step 1
---------------------------- PROCESS ENDED (9502) for package com.example.app ---------------------------- // step 4, doesn't happen after step 2 for some reason
---------------------------- PROCESS STARTED (9902) for package com.example.app ---------------------------- // step 5

Scenario 2 (results in audio playing)

  1. Start the app and play audio
  2. Pause the audio
  3. Swipe from recents
  4. Reopen the app and playing will work

Logs:

---------------------------- PROCESS STARTED (9134) for package com.example.app ---------------------------- // step 1
---------------------------- PROCESS ENDED (9134) for package com.example.app ---------------------------- // step 3
---------------------------- PROCESS STARTED (10319) for package com.example.app ---------------------------- // happens automatically few seconds after 3
marcbaechinger commented 7 months ago

Thanks. Just checking. In the case that the player is not paused, you pause it in onTaskRemoved before calling stopSelf as documented here (see code snippets there)?

Because from what you are posting it looks that the app isn't terminated as you expect.

k-arabadzhiev commented 7 months ago

I'm doing it the same way as UAMP in media3 branch. I've also tried to use player.pause() before releasing it (line 243), but doesn't seem to make a difference.

I just tried with the snippets you've provided. Here is how my code looks like now:

   override fun onTaskRemoved(rootIntent: Intent) {
        Timber.tag(TAG).d("onTaskRemoved")
        super.onTaskRemoved(rootIntent)

        val player = mediaSession.player
        if (player.playWhenReady) {
            Timber.tag(TAG).d("player paused")
            player.pause()
        }
        stopSelf()
    }

    override fun onDestroy() {
        releaseMediaSession()
        Timber.tag(TAG).d("onDestroy")
        super.onDestroy()
    }

    private fun releaseMediaSession() {
        mediaSession.run {
            player.release()
            release()
            Timber.tag(TAG).d("mediasession released")
        }
        serviceJob.cancel()
    }

Scenario 1 logs:

---------------------------- PROCESS STARTED (28214) for package com.example.app ----------------------------
15:03:13.269 28214-28214                      onCreate
15:03:20.351 28214-28214                      onTaskRemoved
15:03:20.351 28214-28214                      player paused

At this point we're at step 2 and the app has been swiped away. However, because the media session hasn't been released, the notification player remains. Playing from it seems to be working as well. If I reopen the app and play again it will also work, but we have a requirement to remove the notification player when app is killed (we don't support playback resumption yet). Also I'm not sure why neither onDestroy nor "PROCESS ENDED" are called. For the former, I'm assuming it's because the session hasn't been released. In UAMP (and also my code before trying these snippets) there's a call to releaseMediaSession before stopSelf(). If this call is present, then onDestroy does get called for this scenario.

Scenario 2 logs:

---------------------------- PROCESS STARTED (28705) for package com.example.app ----------------------------
15:06:34.156 28705-28705                      onCreate
15:06:42.985 28705-28705                      onTaskRemoved
---------------------------- PROCESS ENDED (28705) for package com.example.app ----------------------------

Again, onDestroy isn't called so the session isn't released and the notification player remains, but unlike in scenario 1 it doesn't work. Releasing the session before stopSelf doesn't result in onDestroy being called, but the notification does get removed and everything seems to be working properly as before.

marcbaechinger commented 7 months ago

I'm assuming it's because the session hasn't been released.

If this is the case, then there is a Media3 controller bound to the service that isn't yet unbound. Then stopSelf will not stop the service and hence onDestroy() isn't called.

Are you using a controller in your activivty as well? Where and when in your Activity do you release the controller? Because if the controller isn't released it is still bound to the service and the service won't stop.

k-arabadzhiev commented 7 months ago

Are you using a controller in your activivty as well?

Well in some ViewModels I have injected MusicServiceConnection which has MediaBrowser inside. That's how I'm interacting with the media3 player (i.e. adding MediaItems to the browser using setMediaItems, play/pause, collecting playback state). Releasing the controller is the same as here. I hope that answers your question, because other than that I don't think I'm using a controller in my activity.

marcbaechinger commented 7 months ago

Ok, then you need to make sure that the browser you are having is disconnected before you call stopSelf. Else your browser is still bound to the service and the service won't stop.

In UAMP (and also my code before trying these snippets) there's a call to releaseMediaSession before stopSelf(). If this call is present, then onDestroy does get called for this scenario.

Yeah, that sounds like you solved the problem. When you release the session, the session will disconnect all browser/controllers and stopSelf should work. Is there a reason to not release the session in onTaskRemoved?

Alternatively, you can disconnect your browse/controller either by calling browser.release on the activty/viewModel side. In the session demo each activity takes care of this by releasing the browser in onStop.

I'd say releasing the browser like in the session demo app is a good practice. It's a proper design to release the resources when the activity is stopped. I'm not an expert in view models, but I'd guess there is a similar callback that allows to release resources.

What isn't working is mixing these approaches. If the service can't be sure all browser/controllers are properly disconnected when onTaskRemoved is called, then the session must be released in onTaskRemoved or the service needs to be kept running until the last bound client unbinds.

k-arabadzhiev commented 7 months ago

Yeah, that sounds like you solved the problem. When you release the session, the session will disconnect all browser/controllers and stopSelf should work.

If by problem you mean the "controller not connected" then it is not solved. It's still happening if MusicServiceConnection is injected as singleton using dagger2.

Is there a reason to not release the session in onTaskRemoved?

Well I'm not sure. Initially I was doing it because it's done in UAMP, so I thought that's how it's supposed to be, but in the documentation you've sent, the release of the session is done in onDestroy only. Now that I've tried both approaches it seems like for my use case I must release the session in onTaskRemoved.

I'm not an expert in view models, but I'd guess there is a similar callback that allows to release resources.

There's [onCleared](https://developer.android.com/reference/androidx/lifecycle/ViewModel#onCleared()) method, which is called before Activity's onDestroy and I guess it can't be used for that.

For some reason the init block in MusicServiceConnection, when the class injected as a Singleton, is not being called after the app is reopened (step3, scenario 1) and I guess that's why we get "controller not connected". This block is responsible for connecting it after all. What I tried is to make that init block an actual fun init() and then call that in the ViewModel (fun initBrowser()), then call this ViewModel method in Activity's onStart. I did the same for release and call it in Activity's onStop. This actually fixed my UAMP branch and works even when using onCreate (to initialise the browser) / onDestroy (to release it) instead of onStart/onStop (these are executed each time app is minimised/reopened so I thought it's better to init/release the browser less often). However, the process doesn't end if the app was killed while audio was playing and does end if audio was paused, so there definitely is some difference between these scenarios, but I can't tell what.

The fix also worked for our app, however for some reason one of our VMs (that has MusicServiceConnection injected) doesn't get cleared but that's probably off-topic. I guess for now we will stick to this way of initialising MusicServiceConnection. Still don't understand what the magic behind it is, but seems to work the best.

marcbaechinger commented 7 months ago

onStart/onStop (these are executed each time app is minimised/reopened so I thought it's better to init/release the browser less often)

I'd recommend to release the browser/controller when the app goes to background. That's cheap because you only connect to the running service. I'd recommend using onStart/onStop. After Googling I think this is the page to get lifecycle aware view models? There are different ways probably.

For some reason the init block in MusicServiceConnection, when the class injected as a Singleton, is not being called

You can use dagger or singletongs or something else, you still need to make sure that the browser/controller disconnects when the app is dismissed.

The process doesn't end if the app was killed while audio was playing and does end if audio was paused, so there definitely is some difference between these scenarios, but I can't tell what.

An app must pause playback to get the service out of the foreground and an app needs to disconnect all browser/controllers either on the controller side or by releasing the session before calling stopSelf. Otherwise, 'stopSelf' is a no-op.

k-arabadzhiev commented 7 months ago

An app must pause playback to get the service out of the foreground

What do you mean by that? If we have released the browser/controller from Activity's onStop, then we can't actually pause the audio (before stopSelf) in onTaskRemoved, because the result will be "controller not connected".

or by releasing the session before calling stopSelf. Otherwise, 'stopSelf' is a no-op.

In UAMP the session is released in onTaskRemoved (before stopSelf) and this is what triggers onDisconnected to release the browser as well. Could it be that it takes too much time to release the session (while audio is playing, compared to when it's not) and by the time session/browser are released, stopSelf has been executed as a no-op?

marcbaechinger commented 7 months ago

You can call session.player.pause() I think.

May I ask you to read this section?

k-arabadzhiev commented 7 months ago

You can call session.player.pause() I think.

Yeah you're right, I got confused with similar situation, while testing different things yesterday, so I thought it will result in "controller not connected" but it doesn't.

May I ask you to read this section?

I've read it and experimented with the snippets there. This one for example:

override fun onTaskRemoved(rootIntent: Intent?) {
  val player = mediaSession.player
  if (player.playWhenReady) {
    // Make sure the service is not in foreground.
    player.pause()
  }
  stopSelf()
}

In theory, should be the same as if the user paused the player before swiping away the app. However, it's different. If the user paused before swiping, the process is ended. If pausing is done in onTaskRemoved, the process doesn't end even though onDestroy is also called. Also I don't think I fully understand the comment. Does it mean I have to somehow (if so how?) make sure the service is not in foreground before calling player.pause(), or player.pause() is making sure that the service is not in the foreground?

k-arabadzhiev commented 7 months ago

Looks like the issue is indeed the singleton scope in particular. I've migrated my UAMP fork to Hilt. My initial idea was to use @ServicedScope instead of @Singleton. However that didn't allow me to inject MusicServiceConnection into ViewModels due to how the component hierarchy is defined. I had to use either Singleton or ActivityRetainedScoped. The latter actually did the trick when used for MusicServiceConnection. Killing the app, while audio is playing, will result in the init block (responsible for initializing the browser) of MusicServiceConnection being executed again, which wasn't the case with Singleton. Still doesn't end the process compared to killing the app with paused player, but I guess we can't have it all. As long as it doesn't result in memory leak, should be fine.

On the other hand, as discussed previously, It's probably better instead of relying on init block to use init method (for the browser/controller) that will be called in Activity's onStart and release in onStop, so perhaps that should be documented somewhere.

kelmer44 commented 6 months ago

I'm experiencing this very issue with some of my components being declared as @Singleton using Dagger. Looks like this somehow prevents the MediaLibraryService from being garbage collected thus leaking it.

Installing LeakCanary on @k-arabadzhiev's UAMP dagger branch, MusicService is reportedly being leaked:

Screenshot_20240313_155323

Somehow this seems easier to repro on Samsung devices in my experience.

kelmer44 commented 6 months ago

any plans to investigate this?

marcbaechinger commented 6 months ago

I don't understand much about Dagger or the @Singleton annotiation I'm afraid. The leak report from above is saying that a session is leaked that has still a reference to the service. It looks to me as this is about dependency injection and how to architect an app with it, rather than an issue in the library components.

kelmer44 commented 5 months ago

I really dont see how this can be a problem of the app architecture.

Dagger's @Singleton works by creating a component on the Application level (Hilt will bytecode-replace the Application subclass's parent with a HiltApplication that holds on to that component), and that component is the one keeping all the singleton-scoped instances.

When the Application object is destroyed, that component can be garbage collected as there will be nothing else holding on to it.

In @k-arabadzhiev's dagger branch or in this branch in my fork the only thing really being injected is the MusicServiceConnection, which is already a Singleton object in UAMP's media3 branch.

It is my understanding that the way UAMP's media3 branch implements this singleton (i.e. the classic static object approach) is at the Garbage collection root, so in theory it should even be longer-lived than dagger's approach (but I might be wrong here, I'll admit)

The MusicService in our dagger UAMP's branch doesnt even need to be an @AndroidEntryPoint (so dagger doesnt need to be even involved in its lifecycle).

Looking at the Leak report it looks as though what's holding on to the mediasession and transitively to the service is the OS media framework. I saw a similar report here where they do acknowledge the existence of certain memory leaks in their framework so I'm hoping they can shed some light here.

marcbaechinger commented 5 months ago

any plans to investigate this?

I don't have plans to investigate the Dagger implementation I'm afraid. The reason is simply that I don't have dagger experience.

so in theory it should even be longer-lived than dagger's approach

Are you seeing the same problem with UAMP without Dagger?

kelmer44 commented 5 months ago

I don't have plans to investigate the Dagger implementation I'm afraid. The reason is simply that I don't have dagger experience.

Yes I fully understand that, I was posting a follow-up for awareness of everyone involved :) However I do believe Dagger is a de-facto DI standard and universally used in Android so this issue is probably going to impact more implementors IMO.

Are you seeing the same problem with UAMP without Dagger?

Nope.

DimovskiD commented 1 month ago

I've recently stumbled across the same problem. As described by @kelmer44 , this is reproducible when the instance is a singleton, but that does not mean it is a problem with the DI framework (FYI I was using Koin). The problem likely occurs due to a resource not being properly released in this specific scenario. Let me elaborate:

I need a single instance of the audio player in the app, so that audio playback can continue uninterrupted when switching between screens, but also allowing the user to interact with the audio player from different locations in the app. So, I opted in for using a singleton instance of the player. This instance is being released when the PlaybackService is destroyed, as described in the official documentation, or when the MainViewModel is destroyed (at this point I want to stop the playback). This works fine in most cases, except for the one described in this issue. Namely, when the audio player is playing and the app is force closed, then this problem can be reproduced. After some debugging, this are my findings:

  1. When the application is force closed, the player is released, the service is destroyed, yet the process remains alive;
  2. The next time the user opens the app, they are in the same process, hence using the instance of the audio player that has been already released;
  3. Trying to perform actions on the player that has already been released results in The controller is not connected. Ignoring *
  4. When the user closes the app, now the process ends as well;
  5. When the user opens the app again, everything is back to normal, as a new instance of the player has been created.

We do not see this problem when not using the singleton pattern, as you'd get a new instance of the player in step 2, rather than the one that has already been released. From this, we can conclude that this is not a problem with the DI.

Furthermore, I tried to see what happens if the process ends together with the service by killing the process manually when the onDestroy method of the service is called. In this case, everything works as expected as the next time the user opens the app, they are in a new process with a new instance of the player. But, of course, this is not a solution to the problem, just a way to prove the theory.

So, the question is: why does the OS keep the process alive when force closing the app while audio is playing, but kills the process when audio is not playing? 🤔 Possibly some unreleased resources?

Here's a simplified summary of the problem I faced and how I worked around this issue in my case:

class AudioPlayer(
    private val context: Context,
) {
    private val sessionToken =
        SessionToken(context, ComponentName(context, PlaybackService::class.java))
    private val factory = MediaController.Builder(context, sessionToken).buildAsync()

    private var player: Player? = null

    init {
        factory.addListener({
            player = factory?.get()
    })

    fun release() {
        MediaController.releaseFuture(factory)
        player?.release()
    }
}
class MainViewModel(
    private val audioPlayer: AudioPlayer, //this is injected as singleton instance
) : ViewModel() {

    override onCleared() {
        super.onCleared()
        audioPlayer.release()
    }
}
class PlaybackService : MediaSessionService() {
    private var mediaSession: MediaSession? = null

     override fun onCreate() {
        super.onCreate()
        val player = ExoPlayer.Builder(this).setPauseAtEndOfMediaItems(true).build()
        mediaSession = MediaSession.Builder(this, player)
            .setCallback(CustomMediaSessionCallback())
            .build()
    }

    override fun onTaskRemoved(rootIntent: Intent?) {
        val player = mediaSession?.player!!
        if (player.playWhenReady) {
            player.stop()
        }
        stopSelf()
    }

    override fun onGetSession(controllerInfo: MediaSession.ControllerInfo): MediaSession? =
        mediaSession

    override fun onDestroy() {
        Log.i("PlaybackService", "onDestroy")
        mediaSession?.run {
            player.release()
            release()
            mediaSession = null
        }
        super.onDestroy()
    }
}

And here's the result with this setup: MediaController

In order to work around the issue, I made a couple of changes:

  1. Instead of making the connection with the service once, in the init block of the singleton AudioPlayer, I've moved this logic to an init method
class AudioPlayer(
    private val context: Context,
) : AudioPlayer {
    private var factory: ListenableFuture<MediaController>? = null
    private var player: Player? = null

    fun init() {
        val sessionToken =
            SessionToken(context, ComponentName(context, PlaybackService::class.java))
        factory = MediaController.Builder(context, sessionToken).buildAsync()
        factory?.addListener( { 
            player = factory?.get()
        })
    }

   fun release() {
        factory?.let { MediaController.releaseFuture(it) }
        player?.release()
    }
}
  1. In the MainViewModel, I call the init method in the init block of the view model.
class MainViewModel(
    private val audioPlayer: AudioPlayer, //this is injected as singleton instance
) : ViewModel() {

    init { audioPlayer.init() }

    override onCleared() {
        super.onCleared()
        audioPlayer.release()
    }
}

With these changes, I have a singleton instance of my own AudioPlayer implementation, while the actual media3 player used for the reproduction is created for every session, rather than every process. This is how the changes affect the app behavior:

MediaController (1)

With this setup I was able to keep the desired effects of a single audio player in the application. I hope it helps some of you that are facing the same issue 🙂