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 fixes #7

Merged
merged 4 commits into from
Mar 15, 2015
Merged

Memory fixes #7

merged 4 commits into from
Mar 15, 2015

Conversation

i-n-g-o
Copy link

@i-n-g-o i-n-g-o commented Feb 20, 2015

Hi.

I came across this, when i was using the threaded-version of ofxAVFVideoPlayer (ofxAVFThreadedVideoPlayer).

Actually this PR contains two changes. It might be "better" to seperate the to things, but somehow they are related.

First:
I added a switch to control if the audio gets loaded into the internal buffer or not. The default behavior is how it was before the change (loading audio)
getter: shouldLoadAudio, setter: setShouldLoadAudio:

Second:
When i was testing with the threaded version for a longer time i came across a memory leak. i digged in and found the following:

  • when destroying (or closing) the ofxAVFVideoPlayer and if it was never played (but a file was loaded), the moviePlayer object was not getting freed because the timeObserver holds a reference to it.
  • even if cleaning up the timeObserver there was something else going on. so i made sure, that the blocks scheduled by the timeObserver are not holding a reference to the moviePlayer object. Using __block variables and a __block reference to self (in ofxAVFVideoRenderer)
  • i created the dispatch_queue before using it, in order to be able to sync with the queue and to free it on cleanup
  • a memory-leak at the first iteration when reading audio-data. sampleBuffer has to be released.

Ingo Randolf and others added 3 commits February 20, 2015 12:52
fixing issues with non-releasing addPeriodicTimeObserverForInterval ... why?
…io loading)

defaulting to previous behaviour to load audio
@prisonerjohn
Copy link
Collaborator

Cool, thanks for this. I was aware there was a leak in the audio stuff but never had the time to really look into it. A lot of this addon (minus the audio stuff) got merged into the core, maybe we can now add the audio read in as well.

Do you think you could do a pass to fix indentation and remove debug traces before I merge this?

@i-n-g-o
Copy link
Author

i-n-g-o commented Mar 11, 2015

i had a look at the implementation of the AVPlayer in the current master and created a PR which would fix a potential memory leak:
openframeworks/openFrameworks#3672

i suspect also something else going on in this addon, but i cannot name it at the moment. i am a bit out of the loop about this, but will come back to that sometimes soon.

sure, i will have a look at the indentations and the left debug things and create a new PR.

@prisonerjohn
Copy link
Collaborator

Great, FYI you can just push the changes to the same branch and they'll go in this PR.

@i-n-g-o
Copy link
Author

i-n-g-o commented Mar 15, 2015

all right, fixed the indentations and removed the debug logs
also fixed a log whre QTRenderer is mentioned ;)

prisonerjohn added a commit that referenced this pull request Mar 15, 2015
Memory fixes with audio loading
@prisonerjohn prisonerjohn merged commit d2d6dba into obviousjim:master Mar 15, 2015
@prisonerjohn
Copy link
Collaborator

Thanks!

@obviousjim
Copy link
Owner

thx!!

On Sun, Mar 15, 2015 at 10:03 AM, Elie Zananiri [email protected]
wrote:

Thanks!


Reply to this email directly or view it on GitHub
#7 (comment)
.

  • James

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

Successfully merging this pull request may close these issues.

3 participants