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 leak in usb_cam.cpp #233

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

knorth55
Copy link
Member

@knorth55 knorth55 commented Mar 2, 2023

we should do step by step, not making a big PR.
also this is not a refactoring, but a issue solution.
the priority is higher than refactoring.

with this PR, the memory leaks disappear.
closes #211

usb_cam_fix_memory_leak_compressed.mp4

@knorth55
Copy link
Member Author

knorth55 commented Mar 2, 2023

@twdragon can you check this PR and check there is no memory leak?
I will work on #231 ofcourse, but this PR should be merged first.

I also think we should release the new version after merging this PR and before merging #231.
#231 may cause some errors, so between merging #231 and next-next release, we should take time for issue reporting by source-build users.

@twdragon
Copy link
Collaborator

twdragon commented Mar 2, 2023

@twdragon can you check this PR and check there is no memory leak? I will work on #231 ofcourse, but this PR should be merged first.

I also think we should release the new version after merging this PR and before merging #231. #231 may cause some errors, so between merging #231 and next-next release, we should take time for issue reporting by source-build users.

@knorth55 sure! This is what I am doing right now! I also compare variants with used avcodec functions to ensure that there are no memory leaks.

I agree to release the version with this PR merged in case there are no serious errors, also because now there is no need for end user to change configuration

@knorth55
Copy link
Member Author

knorth55 commented Mar 2, 2023

@twdragon thank you! please test this PR and merge this if there is no more memory leaks.

after this PR is merged, I will move forward for the releasing this package.

@twdragon
Copy link
Collaborator

twdragon commented Mar 2, 2023

@knorth55 I thoroughly check the presence of memory leaks in comparison tests with and without the application of your PR. Let me please enumerate the results here:

  • I forgot to use av_init_packet() when preparing my PR Total refactoring #221, so thank you very much for the notification;
  • I compared the amount of allocated memory with and without av_init_packet(). It appeared only when mjpeg or h264 pixel format was activated but always when av_init_packet() is not used.
  • The amount of leaking memory depends strangely on the version of FFMPEG which was in use. I tested 4.4.2 from apt and bleeding-edge N-108445-ga1bfb5290e. When the bleeding-edge version is used the leak is weaker, and the memory amount oscillates, but it is still present. On the stable version, the amount of allocated memory grows linearly.

After this test, I think. I can merge this PR. I will do it in a couple of hours! Thank you again!

@twdragon twdragon merged commit e713f7a into ros-drivers:develop Mar 2, 2023
@knorth55 knorth55 deleted the fix-memory-leak branch March 3, 2023 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in develop branch, in UsbCam::mjpeg2rgb()
2 participants