ryanheise / audio_service

Flutter plugin to play audio in the background while the screen is off.
806 stars 481 forks source link

Minor Bug[Android]: Pausing and Skipping from notification #140

Closed rohansohonee closed 4 years ago

rohansohonee commented 4 years ago

Describe the bug If we pause the notification and then skip to next song, we are able to clear the notification which is not right. This means if the user pauses song from notification and skips to next song from notification and say quits the app, then song will stop.

Minimal reproduction project https://github.com/rohansohonee/audio_service

To Reproduce Steps to reproduce the behavior:

  1. Start the audio service and play media.
  2. From android notification pause the media. (It is correct from this state only to be able to dismiss or clear the notification. This is working no doubt. Anyhow don't clear notification goto step 3)
  3. Now from android notification skip to next media.
  4. We can now dismiss or clear the notification which is not right. (Media is playing but we can swipe the notification and stop media which is incorrect).

Expected behavior Only from paused state the notification should be dismissed.

Runtime Environment (please complete the following information if relevant):

Flutter SDK version

[√] Flutter (Channel stable, v1.12.13+hotfix.5, on Microsoft Windows [Version 10.0.18363.535], locale en-IN)

[√] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
[√] Android Studio (version 3.5)
[√] VS Code (version 1.41.1)
[√] Connected device (1 available)

• No issues found!
ryanheise commented 4 years ago

Calling onPlay in your background task does not go through the platform side which is what you need to happen. The only way to do this is by invoking play from a client. skipToNext will not in itself cause play to happen, so the client side must sequentially call skipToNext then play if you want this behaviour.

rohansohonee commented 4 years ago

What if client side is disconnected?? Then how do I solve this issue.

ryanheise commented 4 years ago

You have several clients:

  1. The Flutter app
  2. The notification or control center
  3. The headset

In each of these, it is possible to control play/pause as well as skip to next/previous. Since you said that the scenario starts off in a paused state and then the user triggers a skip to next, through one of these 3 clients, the user should be able to use that same client to click the play button.

rohansohonee commented 4 years ago

The scenario is as follows:

Thus I can only write code in dart background isolate. That is why in onSkipToNext I call onPlay(). I hope I am clear.

ryanheise commented 4 years ago

The only part of the code you gave was the onSkipToNext which is the callback for handling a call from the client. I think to understand your issue fully, I need a minimal reproduction project. That is, create a fork of the example and modify it to demonstrate the issue and to allow me to see the bigger picture in code.

rohansohonee commented 4 years ago

See @ryanheise the onSkipToNext is the callback that I can listen to in dart. Cause the app is closed by user. This means UI is disconnected.

(Edited) I have committed and updated the bug issue. Also there are two stop buttons in notification the first one actually performs skip and the second is for performing stop.

ryanheise commented 4 years ago

Thanks for that. Seeing the full context, I can confirm that it is as I said before, and you can solve it in the way I said before. When I get a chance, I'll update the official example to demonstrate this.

Basically:

Calling onPlay in your background task does not go through the platform side which is what you need to happen. The only way to do this is by invoking play from a client. skipToNext will not in itself cause play to happen, so the client side must sequentially call skipToNext then play if you want this behaviour.

rohansohonee commented 4 years ago

Just fixed this in this

rohansohonee commented 4 years ago

Why reopen?? I added the startForeground code since stopForeground was called in onPause()

ryanheise commented 4 years ago

We need to be careful to distinguish between skipping and playing as separate actions. Skipping should not always imply playing, in that some apps will want to have the behaviour where, from a paused state, clicking the skip button will skip tracks and remain in the paused state. Other apps will want to automatically play upon skipping.

So, the code to resume the foreground status of the service does not belong in the skip action at all. I need to think a bit about the right way to support both use cases.

A simple hack would be to have an option to call startForeground in both skip actions, but I'm not sure if that is the right way. Another way is to provide a way for the background task to invoke the platform code for play whenever it wants. And yet another way is to put the responsibility of switching to foreground into the state mechanics rather than the action mechanics.

I would not close the issue until a solution has actually been committed to master.

Edit: However, I am in fact going to bed now, so will need to think more on this tomorrow.

rohansohonee commented 4 years ago

Ok should I remove my pull request??

rohansohonee commented 4 years ago

I will try and implement this tomorrow. Should I have a flag androidStayPausedOnSkip that will allow for both use cases to be met.

Or should I give user the flexibility to start or stop the foreground notification from BackgroundAudioTask in dart.

I think the option two is easier to implement and gives user control over how they would like the behaviour to be.

@ryanheise What are your thoughts?

ryanheise commented 4 years ago

If we go with the second option, then maybe we can also delete the androidStopForegroundOnPause option and have both of these behaviours controlled manually by the app.

Another consideration in favour of this approach is that there is conceivably a way for audio to become paused without a pause being requested by the client, e.g. if app's background audio task decides on its own to pause. In that case, trying to build this logic into onPause on the platform side won't catch the fact that it was paused. The downside is that its more work for the app to manage.

The other alternative is to somehow tie this into the state mechanics. In theory, even when the background audio task initiates a self pause, it is going to call setState to the paused state, and at that moment, the platform side can intercept the state change and automatically move the service in and out of the foreground state for you based on your preferences (where the default is to keep the service in the foreground on pause, and the androidStopForegroundOnPause option causes it to leave foreground whenever transitioning to the paused state)

One other thing related to the stopping and starting of the foreground service which I'm not sure if I mentioned here (although I feel like I may have mentioned it in a separate issue) is that after 10 minutes or so of the service not being in the foreground, Android may kill the service and after longer possibly even the process. In a normal Android app, you would want to still keep the notification alive in these scenarios while your media session is in the paused state. And then when the user clicks the play button to resume playback, your service will be restarted. In this scenario, we need code to fire back up the background isolate since it is no longer running and will not respond to your onPlay event. For now, what happens in this scenario is that after Android kills the service, we close the notification to prevent the user from pressing any buttons that wouldn't respond anyway. But I think for your use case you'll want some way of keeping the notification alive and having some way of restarting the background isolate.

One thing at a time, though, so on the pausing and skipping behaviour, I would go with either your second option or the option of integrating it with the state mechanics. Which of those two options feels like a better API to you?

rohansohonee commented 4 years ago

But I think for your use case you'll want some way of keeping the notification alive and having some way of restarting the background isolate.

My use case follows Google Play Music rules. Rules:

  1. User starts music playback -> an ongoing notification appears i.e user cannot dismiss this notification.
  2. User pauses playback either from UI or from notification -> we allow for the notification to be dismissed by user in this state. This means that the notification is still present but android can kill the service for us or user can by swiping away the notification & stop the music playback.
  3. User pauses initially and then skips next/previous -> the notification should enter dismiss stage first due to pause and should exit dismiss stage due to skip, since user continues to listen to music. The result will be an ongoing notification i.e cannot be dismissed by user, similar to first rule. This case follows rule 2 first & of course android can kill the service but user initiates a skip operation thus keeping the service alive.

Google Play Music follows these rules and I have adopted them as well.

I would go with either your second option or the option of integrating it with the state mechanics

Hard for me to choose just yet. Let's discuss a little more so that I can implement for not just the rules I am using but also for all other rules.

ryanheise commented 4 years ago

Integrating into the state mechanics would mean no extra method calls besides those you're already doing to set state changes. Your media actions would also not need to do anything special to handle the two different skipToNext behaviours. Simply, in an app where you have selected the androidStopForegroundOnPause option and the user clicks the skipToNext media control in the notification while in the paused state, you get to control the behaviour you want in your onSkipToNext by deciding whether or not you call setState to change the state to playing. If you do, the plugin detects that and bring the service back into the foreground, otherwise it leaves the service in the background.

Since you'll already be making these setState calls anyway, this approach wouldn't require any change to the way you write your app.

It is just a question of figuring out which states should trigger the service's transition into and out of the foreground state, because it's not as simple as there being two states, playing and pausing. There's:

enum BasicPlaybackState {
  none,
  stopped,
  paused,
  playing,
  fastForwarding,
  rewinding,
  buffering,
  error,
  connecting,
  skippingToPrevious,
  skippingToNext,
  skippingToQueueItem,
}

One of the problems (which is solved in just_audio's state model) is that the buffering state doesn't actually tell you whether we're playing or paused. But this could perhaps be handled in audio_service by having the plugin remember the state before buffering, and count the transition S1 to buffering to S2 as really a transition from state S1 to S2. Perhaps the same thing applies for rewinding and fastForwarding.

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs, or use StackOverflow if you need help with audio_service.