Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leaks caused by ReaderConverterCallback? #5

Open
mlejva opened this issue Mar 12, 2019 · 9 comments
Open

Memory leaks caused by ReaderConverterCallback? #5

mlejva opened this issue Mar 12, 2019 · 9 comments

Comments

@mlejva
Copy link

mlejva commented Mar 12, 2019

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.

@NamanVaishnav
Copy link

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?

@mlejva
Copy link
Author

mlejva commented Mar 27, 2019

Not yet.

EzimetYusup added a commit to EzimetYusup/AudioStreamer that referenced this issue Jun 27, 2019
rurza added a commit to rurza/AudioStreamer that referenced this issue Jul 9, 2019
fix issue syedhali#5 memory leak while in paused state
@pattypatpat2632
Copy link

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

@mlejva
Copy link
Author

mlejva commented Apr 29, 2020

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

@FlorianAtNacamar
Copy link

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

@jacklandrin
Copy link

The issue seems fixed in Ventura.

@AdamBCo
Copy link

AdamBCo commented Nov 10, 2022

@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?

@alexwhb
Copy link

alexwhb commented Sep 13, 2023

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.

@Roninside
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants