AudioStreamer icon indicating copy to clipboard operation
AudioStreamer copied to clipboard

Memory leaks caused by ReaderConverterCallback?

Open mlejva opened this issue 5 years ago • 9 comments

Hi, first I'd like to say how great this framework is. It saved me a lot of time, thank you.

I've noticed that once I called play() method on the Streamer class, the memory usage was growing rapidly in my app. To check whether it's a problem on my side I downloaded your example project and profiled it. It seems there are memory leaks in ReaderConverterCallback caused by UnsafeMutablePointer that is never deallocated.

I tried to set the streamer's URL to a longer audio file (>1 hour) and the used memory was more than 500MB in about a minute even though the audio file was only about 50MB big.

Steps to reproduce

  1. Download the example project
  2. Run profiling on memory leaks
  3. Hit play before the audio file is downloaded

EDIT

I think I might understand the problem here. The memory usage is growing even though the audio was paused - the read() method is reading empty packets and allocating new memory for the buffer here and here

I'm not really sure how and when should I deallocate the pointers though.

mlejva avatar Mar 12 '19 12:03 mlejva

have you found any solution for it?

Hi, first I'd like to say how great this framework is. It saved me a lot of time, thank you.

I've noticed that once I called play() method on the Streamer class, the memory usage was growing rapidly in my app. To check whether it's a problem on my side I downloaded your example project and profiled it. It seems there are memory leaks in ReaderConverterCallback caused by UnsafeMutablePointer that is never deallocated.

I tried to set the streamer's URL to a longer audio file (>1 hour) and the used memory was more than 500MB in about a minute even though the audio file was only about 50MB big.

Steps to reproduce

  1. Download the example project
  2. Run profiling on memory leaks
  3. Hit play before the audio file is downloaded

EDIT

I think I might understand the problem here. The memory usage is growing even though the audio was paused - the read() method is reading empty packets and allocating new memory for the buffer here and here

I'm not really sure how and when should I deallocate the pointers though.

Have you found any solution?

NamanVaishnav avatar Mar 19 '19 17:03 NamanVaishnav

Not yet.

mlejva avatar Mar 27 '19 15:03 mlejva

Did you ever find a way to deallocate the pointers here? I've been playing around with this a bit and have run into the same issue

Hi, first I'd like to say how great this framework is. It saved me a lot of time, thank you.

I've noticed that once I called play() method on the Streamer class, the memory usage was growing rapidly in my app. To check whether it's a problem on my side I downloaded your example project and profiled it. It seems there are memory leaks in ReaderConverterCallback caused by UnsafeMutablePointer that is never deallocated.

I tried to set the streamer's URL to a longer audio file (>1 hour) and the used memory was more than 500MB in about a minute even though the audio file was only about 50MB big.

Steps to reproduce

  1. Download the example project
  2. Run profiling on memory leaks
  3. Hit play before the audio file is downloaded

EDIT

I think I might understand the problem here. The memory usage is growing even though the audio was paused - the read() method is reading empty packets and allocating new memory for the buffer here and here

I'm not really sure how and when should I deallocate the pointers though.

Did you ever find a way to deallocate the pointers here? I've been playing around with this a bit and have run into the same issue

pattypatpat2632 avatar Apr 29 '20 15:04 pattypatpat2632

Nope, I haven't looked into it since then.

mlejva avatar Apr 29 '20 15:04 mlejva

Hi there, thank you for this nice piece of software. It helped me a lot to get started using AudioToolbox/AudioConverter.

I’ve experienced the same memory leak and would like to tell you about my solution. The idea is to deallocate after converting what you allocated for converting.

  1. There are two pointers for each packet to keep in mind.

I use

var packetDescs:[UnsafeMutablePointer<AudioStreamPacketDescription>?] = []
var packetDatas:[UnsafeMutableRawPointer?] = []
  1. in ReaderConverterCallback

after allocating memory for data I call

packetDatas.append(ioData.pointee.mBuffers.mData) 

and after allocating memory for the description (if there is one) I call

packetDescs.append(outPacketDescriptions?.pointee)
  1. In Reader I clean up

immediately after usage let status = AudioConverterFillComplexBuffer(…) I’m cleaning up with cleanupConverterGarbage() which is

func cleanupConverterGarbage() {
    packetDescs.forEach { (desc) in desc?.deinitialize(count: 1); desc?.deallocate() }
    print("deallocated \(packetDescs.count) packet descriptions")
    packetDescs.removeAll()
    packetDatas.forEach { (data) in data?.deallocate() }
    print("deallocated \(packetDatas.count) packets of data")
    packetDatas.removeAll()
}

I saw memory leaks in profiler without fix. They are gone when fixed. I experienced no threading issues until now.

FlorianAtNacamar

AS_fixed_noLeaks ![Uploading AS_fixed_noLeaks.png…]() AS_orig_showsLeaks

FlorianAtNacamar avatar Mar 18 '21 15:03 FlorianAtNacamar

The issue seems fixed in Ventura.

jacklandrin avatar Jun 20 '22 22:06 jacklandrin

@FlorianAtNacamar Interesting solution for the memory leak. I've added your change, but am running into the following issue with a .mp3 file.

The following line is crashing: packetDescs.forEach { (desc) in desc?.deinitialize(count: 1); desc?.deallocate() }

pointer being freed was not allocated
malloc: *** set a breakpoint in malloc_error_break to debug

How can we fix that?

AdamBCo avatar Nov 10 '22 22:11 AdamBCo

It seems to me that the memory issue is we are downloading the whole file into memory... maybe a better solution would be to only buffer some % of the file ahead (maybe adjusting based on network conditions) and drop all the packets from the reader that have already been played? Or we could make a file caching system if we want that data to persist temporarily. In long audio files the demo app simply won't play because it ran out of memory.

alexwhb avatar Sep 13 '23 16:09 alexwhb

In my implementation what was piling up memory were not a continued downloading but the timer scheduling the buffer. The author indeed commented that it was not an ideal solution. I've managed to use the completion block of the schedule function to pull the next reading instead and it works well for me; the memory issue is gone. So, in order to get there you need to change the scheduleNextBuffer() this way:

` func scheduleNextBuffer() { guard let reader = reader else { os_log("No reader yet...", log: ChannelStreamer.logger, type: .debug) return }

    guard !isFileSchedulingComplete else {
        return
    }
            
    do {
        let nextScheduledBuffer = try reader.read(readBufferSize)
        playerNode.scheduleBuffer(nextScheduledBuffer, completionCallbackType: .dataConsumed, completionHandler: 
                                    { [weak self] _ in
            DispatchQueue.main.async {
                [weak self] in
                self?.handleTimeUpdate()
                self?.notifyTimeUpdated()
                self?.scheduleNextBuffer()
            }
        })
    } catch ReaderError.reachedEndOfFile {
        os_log("Scheduler reached end of file", log: ChannelStreamer.logger, type: .debug)
        isFileSchedulingComplete = true
    } catch {
        os_log("Cannot schedule buffer: %@", log: ChannelStreamer.logger, type: .debug, error.localizedDescription)
    }
}

`
The DispatchQueue.main.asyn is essential to not have the node stuck in the process. Also you have to get rid off the timer instantiation completely and instead launch the first scheduling while setting the reader:

public internal(set) var reader: Reading? { didSet { scheduleNextBuffer() } }

I hope this solution could be of some help. This project is awesome and saved me a lot of time. Thank you to the authors.

Roninside avatar Nov 16 '23 14:11 Roninside