bluefireteam / audioplayers

A Flutter package to play multiple audio files simultaneously (Android/iOS/web/Linux/Windows/macOS)
https://pub.dartlang.org/packages/audioplayers
MIT License
2k stars 844 forks source link

Race condition when playing/pausing audio #1687

Closed filiph closed 11 months ago

filiph commented 11 months ago

Checklist

Current bug behaviour

When AudioPlayer.pause() is called a short time after AudioPlayers.play() is called, the pause will not take effect. The sound file will play in its entirety.

Same goes for AudioPlayer.stop() instead of .pause().

Expected behaviour

When pause() is the latest call to an AudioPlayer, the audio doesn't play.

Steps to reproduce

  1. Execute flutter run on the code sample
  2. Play any sound from the Sources tab. You'd expect the audio won't play since it's paused 5 milliseconds after being played. But in fact, it plays to completion.

Code sample

Code sample Go to the official example's [`lib/tabs/sources.dart` file](https://github.com/bluefireteam/audioplayers/blob/main/packages/audioplayers/example/lib/tabs/sources.dart#L64-L71) and replace the `_play()` method with the following: ```dart Future _play(Source source) async { await player.stop(); Timer(const Duration(milliseconds: 5), () { // Imagine this is a handler to some event that // happens independently. player.pause(); }); await player.play(source); toast( 'Set and playing source.', textKey: const Key('toast-set-play'), ); } ```

Affected platforms

Android, iOS, web, Windows, Linux, macOS

Platform details

No response

AudioPlayers Version

main, 5.2.0

Build mode

debug

Audio Files/URLs/Sources

No response

Screenshots

No response

Logs

No response

Related issues / more information

Note that everything works as expected if you do something like:

await player.play(source);
await player.pause();

But in an app or game, events come from different sources. For example, settings can be updated (read from file) on startup, at almost the exact same moment as music is started. This can lead to the following:

  1. Game starts
  2. Game asks for settings (async)
  3. Music starts (because that's the default) --- play(...)
  4. Settings are finished reading from something like shared_preferences, and they say "no audio"
  5. Music is immediately asked to stop --- stop()
  6. Music continues to play indefinitely because the stop() finishes before the play() can get to the resume() step

FWIW, I couldn't replicate the behavior when using resume() (i.e. the source is already loaded). So I think the problem stems from the steps included in the play() method, which is not atomic.

Related Discord discussion: https://discord.com/channels/509714518008528896/533299043686940692/1168866344742162463

Working on PR

no way

Gustl22 commented 11 months ago

Despite my arguments on Discord (no network, error, etc.) I think it's more consistent to set the state at the beginning before any async processing is happening (as you proposed). If an error occurs the the playing state should be reset. Tests for both of the cases should be included, if possible.

Gustl22 commented 11 months ago

FWIW, I couldn't replicate the behavior when using resume() (i.e. the source is already loaded). So I think the problem stems from the steps included in the play() method, which is not atomic.

Now that I think about: setting the source simply takes much longer than just doing the resume, so pause is called before finishing setting the source in the event loop. So it might not even help to move setting to playing state at the beginning of the resume method. So we may introduce another variable, which stores the intention to play and check it if it's still valid right before the native resume call.

filiph commented 11 months ago

That works for me, thanks!

FWIW, my current workaround is something like this:

  Future<void> _playCurrentSongInPlaylist() async {
    _log.info(() => 'Playing ${_playlist.first} now.');
    try {
      await _musicPlayer.play(AssetSource('music/${_playlist.first.filename}'));
    } catch (e) {
      _log.severe('Could not play song ${_playlist.first}', e);
    }

    // Settings can change while the music player is preparing
    // to play a song (i.e. during the `await` above).
    if (!_settings!.audioOn.value || !_settings!.musicOn.value) {
      try {
        _log.fine('Settings changed while preparing to play song. '
            'Pausing music.');
        await _musicPlayer.pause();
      } catch (e) {
        _log.severe('Could not pause music player', e);
      }
    }
  }
Gustl22 commented 11 months ago

@filiph can you confirm that #1705 fixes your issue? Thx ;D

Edit: As a note, one can also avoid this issue by using setSource and resume separately instead of play, which also avoids setting the source each time for a single player.

filiph commented 11 months ago

I'm not able to run this at this time to confirm, but looking at the code and at the test — yes, this will solve my issue.