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

Fix memory leaks in FFmpegController, #1720 #3461

Closed
wants to merge 1 commit into from

Conversation

low-batt
Copy link
Contributor

The method getPeeksForFile in FFmpegController is leaking memory
when generating thumbnails. This commit will:

  • Add a @try-@finally block in the while loop to free the packet

  • Replace av_free by av_frame_free, when freeing an AVFrame

  • Add a call to sws_freeContext to free the SwsContext

  • Replace deprecated method avcodec_close with avcodec_free_context

  • Add nullable annotation to declaration of probeVideoInfoForFile

This is a stopgap fix and does not address all of the potential leaks
in the method. This commit focuses on the leaks that occur during the
normal flow when generating thumbnails. Error flows will still leak
memory. At some point this method should be refactored to always
properly free memory.

  • This change has been discussed with the author.
  • [ x] It implements / fixes issue Memory leaks. #1720.

Description:

The method `getPeeksForFile` in `FFmpegController` is leaking memory
when generating thumbnails. This commit will:

- Add a `@try-@finally` block in the while loop to free the packet

- Replace `av_free` by `av_frame_free`, when freeing an `AVFrame`

- Add a call to `sws_freeContext` to free the `SwsContext`

- Replace deprecated method `avcodec_close` with `avcodec_free_context`

- Add `nullable` annotation to declaration of `probeVideoInfoForFile`

This is a stopgap fix and does not address all of the potential leaks
in the method. This commit focuses on the leaks that occur during the
normal flow when generating thumbnails. Error flows will still leak
memory. At some point this method should be refactored to always
properly free memory.
@low-batt
Copy link
Contributor Author

I'm expecting significant merge conflicts between this PR and #3566. The HDR PR has a lot of changes to FFmpegControler.m including some, but not all of the fixes for memory leaks in this PR. I suggest getting that PR merged first and then having me deal with resolving merge conflicts in this PR.

@lhc70000
Copy link
Member

Hi @CarterLi, I think I went through the hard times; I'm resuming the development and handling outstanding PRs these days. Eight PRs have been merged and I expect a new release soon. Are you still interested in implementing HDR in IINA?

If you could prepare a PR, I will be able to finish the review within three days. In this case, please include only the implementation part (changes to the video layer and probably the inspector), and add a follow-up PR later for the preference/menu/UI part, so that we can have fewer merge conflicts.

It'd also be great if you could add PRs for your fix to #3625 and #3626.

If you are too busy, I can still incorporate your previous code in IINA on my own, but that also requires your permission.

Thank you very much!

@lhc70000
Copy link
Member

Seems that all changes were included in the HDR branch except for the @try block. Merged locally via 8682a54.

@lhc70000 lhc70000 closed this Apr 28, 2022
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.

2 participants