Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MediaSessionService never destroyed #389

Open
1 task
bubenheimer opened this issue May 12, 2023 · 16 comments
Open
1 task

MediaSessionService never destroyed #389

bubenheimer opened this issue May 12, 2023 · 16 comments
Assignees
Labels

Comments

@bubenheimer
Copy link

Media3 Version

Media3 1.0.1

Devices that reproduce the issue

Android emulator API 33

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Not tested

Reproduction steps

I first mentioned this issue in #346.

  1. Connect MediaController in one process to MediaSession (via MediaSessionService) in another process.
  2. Perform some amount of player interaction, like starting and pausing playback.
  3. Release MediaController on client.
  4. Observe MediaController being disconnected on server side.
  5. For good measure, manually release MediaSession and Player from MediaSession.Callback.onDisconnected()
  6. Wait for a long time (30+ minutes)
  7. Observe that MediaSessionService.onDestroy() still has not been called

Notably this issue does not occur when there is no player interaction, i.e. Player and MediaSession created but not really used, e.g. no call to prepare().

Expected result

MediaSessionService.onDestroy() called shortly after MediaController released.

Actual result

MediaSessionService.onDestroy() never called.

Media

N/A

Bug Report

@bubenheimer
Copy link
Author

bubenheimer commented May 12, 2023

This issue also exists in media3 1.1.0-alpha01.

NB I use a custom player based on SimpleBasePlayer.

Also, I see that MediaSession.Callback onConnect() and onPostConnect() are called twice for some reason, within a few milliseconds. This may not be related, the double call also occurs when there is no player interaction, and in this case onDestroy() is called. I create the Controller just once, so this looks like a (separate?) bug.

@marcbaechinger marcbaechinger self-assigned this May 12, 2023
@graymind75
Copy link

This starts with 1.0.0-beta03. In the 1.0.0-beta02, The playback service is destroyed and the notification got cleared as I mentioned here

@bubenheimer
Copy link
Author

@graymind75 Thanks for the link. I'm not entirely sure it's the same issue. There is no visible notification in my case after controller release.

Also, just to clarify, all interaction in my case comes from the controller, the user does not interact with the notification.

@bubenheimer
Copy link
Author

bubenheimer commented May 12, 2023

Then again, just hypothesizing, perhaps one of those duplicate onConnect() calls is coming from the notification? (I just use the default notification mechanism, zero customization.) They look very similar, same package, same UID (10159), but small difference in connectionHints. I'm attaching debugger screenshots for the 2 ControllerInfo objects, in onConnect() call order.

There is only a single onDisconnected() call.

(Using media3 1.1.0-alpha01)

Controller#1

Controller#2

@marcbaechinger
Copy link
Contributor

marcbaechinger commented May 12, 2023

Thanks for your report. I'll look into this more closely next week.

Just writing to let you quickly know about the reason of the second connect: it comes from an internal controller in MediaNotificationManager that is used to update the notification when the controller state changes.

@marcbaechinger
Copy link
Contributor

Notably this issue does not occur when there is no player interaction, i.e. Player and MediaSession
created but not really used, e.g. no call to prepare().

Just another note :)

Once the player is playing, the service starts into the foreground. Once the service is started (in the foreground), the system doesn't stop the service anymore, instead the app needs to stop the service.

https://developer.android.com/guide/components/services#Stopping

So if you at the moment when you release the session and player (step 5 above) call stopSelf(), onDestroy is called. If that's not the case this is a bug we need to investigate, but that how it works reliably with the demo app.

@bubenheimer
Copy link
Author

@marcbaechinger that did the trick, thanks. Quite the hack. Hope you guys will be able to improve the API on that front. Will have to think about how to best incorporate this cleanly.

I'm very familiar with service lifecycles. Media3 seems to take care of the grunt work for me, it's surprising that it does not handle stopping the service. I just hope I won't have to recreate the whole service start/stop/foreground mess myself again, it required some delicate logic in my MediaBrowserServiceCompat version.

@bubenheimer
Copy link
Author

What MediaSessionService documentation says:

The service will be destroyed when all sessions are released, or no controller is binding to the service while the service is in the background.

The "released" link points at MediaController.release(). There is some more relevant info in the preceding paragraph.

@bubenheimer
Copy link
Author

@marcbaechinger I am curious about plans to address this issue, so I can move forward with my implementation. Current behavior is a violation of the documented API contract. Alternatives I see:

  1. Change behavior to reflect contract. This is what I'd expect to happen by default: the API is in charge of starting the service at the right time, so it's responsible for stopping it at the right time. "The right time" may be difficult for app code to determine, see next point.
  2. Change contract to reflect current behavior. Weird offloading of service stopping responsibility onto app. Can put a heavy burden on app:
    • I imagine for a general solution, an app would have to count controller connections on the service side. I doubt this is a good idea, there is that extra API-internal controller that I cannot identify as such, and likely other external controllers (Android Auto, headset?) that may never really release, etc. I need the API to deal with these messes.
    • An app may be able to stop the service from the controller side based on app-specific state and behavior. This is unlikely to be possible in general, because an app is not necessarily in charge of all controllers; the controllers may be the ones controlling the app lifetime. So then you rely on the OS to suspend your started background service and kill it eventually, which looks messy.
  3. Make it configurable, offering a choice between 1 and 2, while defaulting to 1. This could be useful when the app is indeed capable of stopping the service from the controller side based on its own state, and has a reason to keep the service & session alive past controller lifetime. For example, in my case it could make sense to do this, because session initialization can be expensive due to playlist assembly and playback initialization, and there is a well-defined end to all possible playback and service lifetime.

@marcbaechinger
Copy link
Contributor

marcbaechinger commented May 31, 2023

Thanks for your reasoning. We are thinking about what the best option would be. It's good to know the requirements for your use case.

We will have to think about other use cases as well. I don't think we can have a one-size-fit-all solution I'm afraid so it will probably be some modes, or similar that a developer can choose when building/creating the session/service. Not sure about details now. We are currently in the design stage of this.

Currently, as in with the current version 1.0.2, an app needs to stop the service itself as you already mentioned.

Please note that the internal controller connects to the session, but does not bind to the service. This is a small but important detail. Further, on API levels where System UI does not connect to the session (I think from top of my head this is below API 29), we can not simply stop the service when all controller disconnect because apps that want to keep the service playing when the activity is removed, need a way where the service keeps running in the foreground without any controllers connected,.

These are some cases that we need to take into account as well. To not remove any of these options for apps, the Media3 library does currently neither stop the service nor does it disconnect any controllers or release a session automatically.

However, I agree with you that we can take some burden from developer if we have some life-cycle modes that developers can opt-in to make Media3 handle some common cases. Besides this there will be, probably by default, a manual mode that keeps the current behavior.

Many thanks again for your contribution and the detailed reasoning. That's very valuable for us. We will keep you posted when we push some related changes.

@marcbaechinger
Copy link
Contributor

I forgot:

extra API-internal controller that I cannot identify as such

We will mark this controller and document this properly. Thanks for raising awareness that this is difficult to identify.

@bubenheimer
Copy link
Author

@marcbaechinger Thank you for adding all that detail. For the time being I may indeed try to keep the service started until I know I really will not not need it again, and then stop it at that time (plus listen for trimMemory, etc.). My preferences here have evolved over the course of our discussion. I actually might choose to ensure it will indeed be started by always issuing a prepare() or such, rather than letting this depend on "meaningful user interaction" like play/pause.

Which seems to point at a certain Service/session start/stop responsibility misalignment again. I know exactly when I want the Service and the session to start and when I want them to stop. But it's really the media API that starts everything, sometimes, and stops some things, at some other times. I want to control starting and stopping everything, always, so I think I need to carefully negotiate with the media API about both starting and stopping. Unless I decide to assume full control of all this (notifications, service start/stop, foregrounding) myself again, which I don't really want, either.

rohitjoins pushed a commit that referenced this issue Aug 2, 2023
The `MediaNotificationManager` registers an internal controller
to each session. This change marks this controller through its
connection hints and provides an API for apps to hide
implementation details of the marking.

Issue: #389
PiperOrigin-RevId: 549712768
@wongcain
Copy link

I have just run into this issue myself. I am glad to read here confirmation that stopSelf() needs to be called explicitly. I don't have a problem with it working this way, but can I request that the documentation be updated?

@marcbaechinger
Copy link
Contributor

True! Sorry to make you dig. I updated the documentation.

I added a section about implementing the service lifecycle on the DAC page about 'Background playback with a MediaSessionService'. This includes calling stopSelf in onTaskRemoved

@colorgold
Copy link

I have the opposite problem. I am not able to continue playing when the app is swiped away.
My service looks like this:

public class PlayerService extends MediaSessionService {
    private static final String TAG = "PlayerService";
    private MediaSession mediaSession = null;

    @Override
    public void onCreate() {
        super.onCreate();
        ExoPlayer player = new ExoPlayer.Builder(this).build();
        mediaSession = new MediaSession.Builder(this, player)
                .setSessionActivity(getMainIntent()).build();
        

    }

    @Override
    public int onStartCommand(@Nullable Intent intent, int flags, int startId) {
        return super.onStartCommand(intent, flags, startId);
    }

    private PendingIntent getMainIntent() {
        Intent intent = new Intent(this, MainActivity.class);
        intent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP);
        intent.putExtra(Common.isPlaying, mediaSession != null && mediaSession.getPlayer().isPlaying());
        return PendingIntent.getActivity(this, 0,
                intent, PendingIntent.FLAG_UPDATE_CURRENT | (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M ?
                        PendingIntent.FLAG_IMMUTABLE : PendingIntent.FLAG_UPDATE_CURRENT));
    }

    @Nullable
    @Override
    public MediaSession onGetSession(@NonNull MediaSession.ControllerInfo controllerInfo) {
        return mediaSession;
    }

    @Override
    public void onTaskRemoved(Intent rootIntent) {
        super.onTaskRemoved(rootIntent);
        Player player = mediaSession.getPlayer();
        if (!player.getPlayWhenReady()
                || player.getMediaItemCount() == 0
                || player.getPlaybackState() == Player.STATE_ENDED) {
            stopSelf();
        }
    }

    @Override
    public void onDestroy() {
        if (mediaSession != null) {
            mediaSession.getPlayer().release();
            mediaSession.release();
            mediaSession = null;
        }
        super.onDestroy();
    }
}

My MainAcitivty:


    @Override
    protected void onStart() {
        super.onStart();
        SessionToken sessionToken = new SessionToken(this,
                new ComponentName(this, PlayerService.class));
        controllerListenableFuture = new MediaController.Builder(this, sessionToken).buildAsync();
        controllerListenableFuture.addListener(() -> {
            try {
                player = controllerListenableFuture.get();
                player.addMediaItem(media); //media created from uri
                player.prepare();
                player.play();
            } catch (ExecutionException | InterruptedException e) {
                throw new RuntimeException(e);
            }
        }, MoreExecutors.directExecutor());
    }

Whenever I close the app, playback stops and the notification disappears. How do I keep the player running in the background?

@icbaker
Copy link
Collaborator

icbaker commented Jun 27, 2024

I have the opposite problem

@colorgold: i suggest you file a new issue, to avoid this one getting too confusing, since you might need an opposite resolution for an opposite problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants