search

ryanheise / just_audio

Audio Player
1.03k stars 652 forks source link

Enabling shuffle mode on a large ConcatenatingAudioSource critically degrades just_audio_background performance #872

Closed agersant closed 1 year ago

agersant commented 1 year ago

Which API doesn't behave as documented, and how does it misbehave? When calling setShuffleModeEnabled(true) on an audio player which is currently playing a ConcatenatingAudioSource with several hundreds/thousands of children, all audio player operations like play/pause take multiple seconds to complete while blocking the UI thread.

Minimal reproduction project https://github.com/agersant/just_audio/

To Reproduce (i.e. user steps, not code) Steps to reproduce the behavior:

  1. Clone the repository linked above
  2. Checkout the shuffle branch
  3. Run the example app under just_audio_background\example using an Android device or emulator
  4. Observe the the app has satisfying performance when toggling playback or tapping individual playlist entries
  5. Tap the shuffle button
  6. Observe that the app becomes very unresponsive, with each interaction taking several seconds to complete

Error messages

N/A

Expected behavior App performance is not significantly degraded when enabling shuffle mode.

Screenshots

Video demonstration: https://user-images.githubusercontent.com/817256/201009999-fbf6b984-ce31-45f5-bed7-6d17c1c03d32.mp4

Desktop (please complete the following information):

Smartphone (please complete the following information):

Flutter SDK version

[√] Flutter (Channel stable, 3.0.2, on Microsoft Windows [Version 10.0.19044.2251], locale en-US)
[!] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
    X cmdline-tools component is missing
      Run `path/to/sdkmanager --install "cmdline-tools;latest"`
      See https://developer.android.com/studio/command-line for more details.
    X Android license status unknown.
      Run `flutter doctor --android-licenses` to accept the SDK licenses.
      See https://flutter.dev/docs/get-started/install/windows#android-setup for more details.
[√] Chrome - develop for the web
[√] Visual Studio - develop for Windows (Visual Studio Build Tools 2019 16.11.16)
[√] Android Studio (version 2021.2)
[√] VS Code (version 1.73.1)
[√] Connected device (4 available)
[√] HTTP Host Availability

Additional context Pausing execution during the very long hitches reveals that the time is being spent in get shuffleIndices within just_audio_background.dart. Example callstack:

new _GrowableList._ofEfficientLengthIterable (dart:core-patch/growable_array.dart:183)
new _GrowableList.of (dart:core-patch/growable_array.dart:150)
new List.of (dart:core-patch/array_patch.dart:51)
ListIterable.toList (dart:_internal/iterable.dart:213)
AudioSourceExtension.shuffleIndices (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:808)
_PlayerAudioHandler.shuffleIndices (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:500)
_PlayerAudioHandler.effectiveIndices (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:502)
_PlayerAudioHandler.shuffleIndicesInv (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:506)
_PlayerAudioHandler.effectiveIndicesInv (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:514)
_PlayerAudioHandler.getRelativeIndex (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:525)
_PlayerAudioHandler.previousIndex (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:517)
_PlayerAudioHandler.hasPrevious (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:519)
_PlayerAudioHandler._broadcastState (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:688)
_PlayerAudioHandler._broadcastStateIfActive (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:681)
_PlayerAudioHandler.setShuffleMode (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:617)
CompositeAudioHandler.setShuffleMode (c:\Users\agersant\AppData\Local\Pub\Cache\hosted\pub.dartlang.org\audio_service-0.18.7\lib\audio_service.dart:2141)
_JustAudioPlayer.setShuffleMode (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\lib\just_audio_background.dart:277)
AudioPlayer.setShuffleModeEnabled (c:\Users\agersant\Documents\GitHub\just_audio\just_audio\lib\just_audio.dart:1051)
<asynchronous gap> (Unknown Source:0)
MyAppState.build.<anonymous closure>.<anonymous closure> (c:\Users\agersant\Documents\GitHub\just_audio\just_audio_background\example\lib\main.dart:218)
rupinderjeet commented 1 year ago

This seems related to https://github.com/ryanheise/just_audio/issues/294.

A solution their suggests to keep your own list of playing queue, and insert items on the go.

Initialise a ConcatenatingAudioSource with an initial sublist of children small enough to load without issue, and then write your own application logic to lazily add more children as needed (just-in-time). If you want to create the illusion that the list of children is complete from the beginning, you will need to write your UI around some other more complete list of metadata, separate from ConcatenatingAudioSource's own internal list of items.

But, this is about inserting items slowly and as needed. Your issue is about shuffle, which rearranges all of audio sources. This means you might have to handle shuffle yourselves on your own playing queue. Although, if you go on this path like me, there might be more work you will need to put in to support other features like: next item, previous item, skip to item, shuffle-mode, repeat-mode, etc. Perhaps, move these features outside of the library too.

My playing queue is generally 1000s of songs, but actual audio-sources that are queued in to audio-player are about 5, keeping the current-index at 2.

I have never encountered it though (slowness), but this is a discussion I am interested in as well.

agersant commented 1 year ago

Thanks for chiming in! I did look at this issue too, but I think it is an independent problem.

The slowdown in this one is not related to creating the concatenating audio source but specifically in just_audio_background code that cares about the shuffling order. It behaves well until shuffle mode is enabled.

rupinderjeet commented 1 year ago

My bad, I apologize.

ryanheise commented 1 year ago

Can you confirm that it happens only with the use of just_audio_background, and works fine with vanilla just_audio?

agersant commented 1 year ago

I just verified, this issue does not happen when using vanilla just_audio. 😮

Video recording with vanilla just_audio (this also has a very large concatenating audio source): https://user-images.githubusercontent.com/817256/201025844-4fb32490-a8c4-4456-a676-eb4d65e5205b.mp4

I just pushed this test in the shuffle branch of https://github.com/agersant/just_audio. I also just realized I had forgotten to push the initial example using just_audio_background, but it is now in there too.

Relevant commits:

ryanheise commented 1 year ago

Something very strange seems to be happening in the FlutterEngine. Normally Dart will execute lines of code continuously until it gets to an await, which yields to the next coroutine. However, it seems like Dart is actually pausing at a line of code where there is no await, and nothing intensive is happening on that line.

Without understanding that side of things fully, my guess is that audio_service might be doing some intensive work on the main thread (although it shouldn't be), such as broadcasting this very large queue. I don't actually know if Android was designed to broadcast such a large sized queue.

Another thing you could test to confirm this is modifying the audio_service example in a similar way.

agersant commented 1 year ago

Thank you for investigating ♥

I took a stab at reproducing this with the audio_service playlist example. The repro code is over there: https://github.com/agersant/audio_service/commit/7294f9072f16712d1c880b73e9d550c9ec338e42 This did not exhibit the large performance degradation either - so it seems the problem is limited to just_audio_background.

This lines up with my theory that get shuffleIndices from just_audio_background is the offending function that takes a lot of time and is called repeatedly.

ryanheise commented 1 year ago

Hmm, that wouldn't explain how it is that the main Flutter isolate is being interrupted or paused at points where there is no await. I was thinking that maybe audio_service was also doing something on the main thread (like loading artwork for the media session). In the past this has been a source of issues although I also think that has since been fixed.

It's truly a mystery. I am curious to know what's really happening that could cause an isolate to be interrupted like this, but I suspect it's not going to be easy to track down.

agersant commented 1 year ago

I am also unsure what darkness you peered into within Flutter Engine, but I found a trivial optimization which fixes this problem. It turns out get shuffleIndices was being called way more times than necessary 🥳

ryanheise commented 1 year ago

Oh, right! That code is too inefficient. In just_audio core, the inverted indices are cached and recomputed only when necessary in _updateShuffleIndices, and that same approach should probably also be done in just_audio_background.

ryanheise commented 1 year ago

There is still a slight delay when pressing the shuffle button, so I also think that shuffle() should probably also be async, and the default implementation should probably try to do the shuffling in an isolate on platforms where isolates are available.

agersant commented 1 year ago

In just_audio core, the inverted indices are cached and recomputed only when necessary in _updateShuffleIndices, and that same approach should probably also be done in just_audio_background.

I can try to update the PR in this direction. However, I'm not sure when the appropriate time to update the shuffle indices is. Would it be everywhere that _updateQueue() is called and from customSetShuffleOrder (after it calls shuffle on the player?)? I'm very unclear on what the correct order of operation is.

so I also think that shuffle() should probably also be async

Which shuffle method is this about? Is that the one on Audiosource? 😔 (and does this belong in the same PR?)

ryanheise commented 1 year ago

In just_audio core, the inverted indices are cached and recomputed only when necessary in _updateShuffleIndices, and that same approach should probably also be done in just_audio_background.

I can try to update the PR in this direction. However, I'm not sure when the appropriate time to update the shuffle indices is. Would it be everywhere that _updateQueue() is called and from customSetShuffleOrder (after it calls shuffle on the player?)? I'm very unclear on what the correct order of operation is.

That sounds reasonable. In terms of the correct order, I think it should be done immediately after queue.add and after _source = request.audioSourceMessage.

so I also think that shuffle() should probably also be async

Which shuffle method is this about? Is that the one on Audiosource? 😔 (and does this belong in the same PR?)

That would be separate from this PR, but yes I'm thinking of AudioSource.shuffle and also the one in ShuffleOrder itself.

agersant commented 1 year ago

Thanks! I updated the PR with your suggestions. It is working great as far as I can tell (performance and functionality).

One thing that puzzles me is I would have thought the indices need to be recalculated before calling queue.add. My reasoning was that app UIs could have callbacks firing in response to the queue broadcast, and these callbacks might read values like nextIndex that depend on the shuffle indices. I wasn't able to trigger any bug along those lines so my mental model must have been incorrect.

ryanheise commented 1 year ago

That's a good point. Now, nothing consequential actually listens to the queue stream here, rather, _broadcastState is called at appropriate times to update the notification based on hasNext/hasPrevious, so it is a matter of ensuring the recalculation is done before calling _broadcastState. The reason you did not trigger any bug is likely because _broadcastState is only called in the first of the following methods when it should probably be called in all of them:

  Future<SetShuffleOrderResponse> customSetShuffleOrder(
      SetShuffleOrderRequest request) async {
    _source = request.audioSourceMessage;
    _broadcastStateIfActive();
    return await (await _player).setShuffleOrder(SetShuffleOrderRequest(
      audioSourceMessage: _source!,
    ));
  }

  Future<ConcatenatingInsertAllResponse> customConcatenatingInsertAll(
      ConcatenatingInsertAllRequest request) async {
    final cat = _source!.findCat(request.id)!;
    cat.children.insertAll(request.index, request.children);
    _updateQueue();
    return await (await _player).concatenatingInsertAll(request);
  }

  Future<ConcatenatingRemoveRangeResponse> customConcatenatingRemoveRange(
      ConcatenatingRemoveRangeRequest request) async {
    final cat = _source!.findCat(request.id)!;
    cat.children.removeRange(request.startIndex, request.endIndex);
    _updateQueue();
    return await (await _player).concatenatingRemoveRange(request);
  }

  Future<ConcatenatingMoveResponse> customConcatenatingMove(
      ConcatenatingMoveRequest request) async {
    final cat = _source!.findCat(request.id)!;
    cat.children
        .insert(request.newIndex, cat.children.removeAt(request.currentIndex));
    _updateQueue();
    return await (await _player).concatenatingMove(request);
  }

I guess technically it need only be called if the hasNext/hasPrevious state has changed. I don't know how much a gain there is from such an optimisation, but to pick one example, you could do this:

  Future<ConcatenatingInsertAllResponse> customConcatenatingInsertAll(
      ConcatenatingInsertAllRequest request) async {
    final hadPrevious = hasPrevious, hadNext = hasNext;
    final cat = _source!.findCat(request.id)!;
    cat.children.insertAll(request.index, request.children);
    _updateShuffleIndices(); // new
    if (hasPrevious != hadPrevious || hasNext != hadNext) _broadcastStateIfActive(); // new
    _updateQueue(); // and also undo the changes here since we update the indices above
    return await (await _player).concatenatingInsertAll(request);
  }
agersant commented 1 year ago

Thanks a lot for the clarification, I definitely needed it.

I updated the PR again with the suggested changes, modulo:

ryanheise commented 1 year ago

Sorry for the delay, however this is looking good :+1: . I will do some testing and hopefully merge soon.

ryanheise commented 1 year ago

I found one more case which is the initial case when the audio is first loaded. This is added in my copy of your branch: agersant-shuffle-fix.

If that works for you, I'd be happy to go ahead and merge/publishg.

agersant commented 1 year ago

Oh nice catch on the initial load. Works for me indeed!

Thanks for the continued hard work ♥

ryanheise commented 1 year ago

Your pull request is now released on pub.dev as 0.0.1-beta.8 :+1: Thank you for the contribution!

github-actions[bot] commented 1 year 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 just_audio.