ryanheise / just_audio

Audio Player
1.05k stars 675 forks source link

Memory leak while using StreamAudioSource (Even after calling dispose) #1201

Open sibias opened 8 months ago

sibias commented 8 months ago

Which API doesn't behave as documented, and how does it misbehave? GC failed to remove instance of StreamAudioSource following the execution of audio play. This is confirmed using flutter dev tools (memory). So if the user executes audio play 50 times there will be 50 instances of AudioStreamSource in below example.

    ByteData res = await rootBundle.load('assets/test.mp3');
    player = AudioPlayer();  
    await player.setAudioSource(AudioStreamSource(res.buffer.asUint8List()));            
    await player.play();
    player.dispose();

// Implementation of AudioStreamSource given below

    class AudioStreamSource extends StreamAudioSource {
      final Uint8List _buffer;

      AudioStreamSource(this._buffer) : super(tag: 'MyAudioSource');

      @override
      Future<StreamAudioResponse> request([int? start, int? end]) async {
        // Returning the stream audio response with the parameters
        return StreamAudioResponse(
          sourceLength: _buffer.length,
          contentLength: (end ?? _buffer.length) - (start ?? 0),
          offset: start ?? 0,
          stream: Stream.fromIterable([_buffer.sublist(start ?? 0, end)]),
          contentType: 'audio/mpeg',
        );
      }

Minimal reproduction project Provide a link here using one of two options: Link to example

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

  1. Run the example in above link (main.dart) and press "Play Byte Stream" button.
  2. Open Dev tools in browser and watch memory tab
  3. Watch the instances count of AudioStreamSource or TestStreamAudioSource or LockCachingAudioSource after each audio play

Error messages

N/A

Expected behavior All instances of classes derived from StreamAudioSource should be removed from memory by clearing all retention paths following dispose or based on scope

Screenshots image

Retention paths image

Desktop (please complete the following information): Not running on Desktop / Web

Smartphone (please complete the following information):

Flutter SDK version

Flutter SDK - 3.19

Additional context N/A

ryanheise commented 8 months ago

Minimal reproduction project Provide a link here using one of two options: "The example".

You can't say "The example" when the example doesn't use StreamAudioSource. This is your second strike. Please get the issue submission right on the 3rd time.

sibias commented 8 months ago

Hi @ryanheise, updated the "Minimal reproduction project" with link to forked project with modifications in example.

gwhizofss commented 7 months ago

Minimal reproduction project Provide a link here using one of two options: "The example".

You can't say "The example" when the example doesn't use StreamAudioSource. This is your second strike. Please get the issue submission right on the 3rd time.

It looks like the sample code has been updated to use the StreamAudioSource starting at line 247. We are wanting to use this player, but need to use HLS streaming. Is this bug a showstopper for us in that case? @ryanheise @sibias

class AudioStreamSource extends StreamAudioSource {
  final Uint8List _buffer;

  AudioStreamSource(this._buffer) : super(tag: 'MyAudioSource');

  @override
  Future<StreamAudioResponse> request([int? start, int? end]) async {
    // Returning the stream audio response with the parameters
    return StreamAudioResponse(
      sourceLength: _buffer.length,
      contentLength: (end ?? _buffer.length) - (start ?? 0),
      offset: start ?? 0,
      stream: Stream.fromIterable([_buffer.sublist(start ?? 0, end)]),
      contentType: 'audio/mpeg',
    );
  }
}

class TestStreamAudioSource extends StreamAudioSource {
  TestStreamAudioSource({dynamic tag}) : super(tag: tag);

  @override
  Future<StreamAudioResponse> request([int? start, int? end]) async {
    return StreamAudioResponse(
      contentType: 'audio/mock',
      stream: Stream.value(byteRangeData.sublist(start ?? 0, end)),
      contentLength: (end ?? byteRangeData.length) - (start ?? 0),
      offset: start ?? 0,
      sourceLength: byteRangeData.length,
    );
  }
}
Colton127 commented 1 month ago

This issue isn't exclusive to StreamAudioSource, but it is the most troublesome because it opens streams that consume a lot more resources/RAM than other audio sources.

Once set, AudioSources are stored in this map: https://github.com/ryanheise/just_audio/blob/b5fad794dee004a9cb92804fce6fcb12d025a0f8/just_audio/lib/just_audio.dart#L103

This map is only cleared when dispose() is called: https://github.com/ryanheise/just_audio/blob/b5fad794dee004a9cb92804fce6fcb12d025a0f8/just_audio/lib/just_audio.dart#L1247

Thus, when you set multiple audio sources, each AudioSource is kept alive and Dart GC isn't able to dispose the object correctly.

My proposed solution is to remove the Map<String, AudioSource> altogether, and simply dispose the existing AudioSource when a new AudioSource is set. The previous AudioSource can probably be disposed here without issue:

https://github.com/ryanheise/just_audio/blob/b5fad794dee004a9cb92804fce6fcb12d025a0f8/just_audio/lib/just_audio.dart#L769

Additionally, it would be helpful to expose dispose and even setup methods for custom audio sources. This way we can manually clean up any open streams, if necessary.

Edit: The memory overhead of storing all AudioSource's in this map is very minimal. As ryanheise pointed out, this doesn't keep any audio buffers alive. Just the AudioSource object itself. The only way this would have an impact of significance is if hundreds of different AudioSource's were set overtime.

ryanheise commented 1 month ago

The Dart side doesn't affect memory management, since the native side is what ultimately holds the audio buffers in memory via its own caching. It may be a consideration in the future to have a dispose method on audio sources to instruct the native side to free memory for just one audio source, but at the moment, there is only a dispose method on the player itself. There are also the various platform specific buffering parameters you can pass into the AudioPlayer constructor to tell just_audio how much to hold in memory.

eivihnd commented 1 month ago

In other words, the current recommended practice/workaround is to dispose the player after each (or x) playback(s) in order to avoid this memory leak?

ryanheise commented 1 month ago

It would be a case-by-case basis, because there appear to be different scenarios being described, and I don't think they are all the same type of thing. For example, there is a memory issue in the implementation of the lock caching audio source specifically, which is on the Dart side. Outside of that, it has been reported that StreamAudioSource implementations besides that one have their own issues. And then outside of that, it has been reported that audio sources besides StreamAudioSource have an issue. These are 3 different issues with not necessarily the same workaround.

It would help we had a separate issue submission for each of these. In your case, which of the 3 cases is it?

eivihnd commented 1 month ago

In my case it is the StreamAudioSource.