-
Notifications
You must be signed in to change notification settings - Fork 598
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
[refactored version of #221] Total refactoring #231
Conversation
knorth55
commented
Mar 1, 2023
•
edited by twdragon
Loading
edited by twdragon
- The node implemented as singleton with monopolistic device access model (one node per device)
- Moved and separated into different blocks the the code using V4L2, ROS and FFMPEG APIs
- Removed message pointers completely, the node uses runtime-constant shared object addresses
- Debugged FFMPEG decoder algorithm, removed memory leak. Closes Memory leak in develop branch, in UsbCam::mjpeg2rgb() #211
- Added (using @flynneva's code) the conditional compilation directives to exploit properly versioned libavcodec API. Closes Fix compiler warnings, replace deprecated code #207
- Closes [Community assistance needed] libavcodec multi-version compatibility #203
- Added separated launch files to run the node and to test it using image_view. Removed image_view from ROS dependencies.
- Node parameters with convenient defaults moved from the launch file to YML file for the parameter server
- Documented parameter names and default values
@knorth55 I do not want to mess up with the commits, so we can leave this PR as is and start reviewing here. If you agree with the current state, I will be very happy to hear from you! |
@knorth55 I built and tested the package in the state of this PR, so we can proceed |
@tfoote thank you! |
@tfoote and in this repo setting, there are already several collaborators who are not listed in I'm also in |
I can run my Jabra Panacast camera with
|
I was running wrong command. - rosrun usb_cam usb_cam_node _video_device:=/dev/video2 _image_width:=3840 _image_height:=1080 pixel_format:=mjpeg _color_format:=yuv420p
+ rosrun usb_cam usb_cam_node _video_device:=/dev/video2 _image_width:=3840 _image_height:=1080 _pixel_format:=mjpeg _color_format:=yuv420p
|
@twdragon I continued running this PR and the memory usage got larger and larger. usb_cam_memory_leak_compressed.mp4 |
I found this memory leak issue also occurs in the video is the case with usb_cam_0.3.6_memory_compressed.mp4@twdragon |
I fixed the memory leak probrem in now the memory usage stops at usb_cam_pr_fix_memory_leak_compressed.mp4I trusted the commit message that #211 is fixed in this PR, but actually #211 was not solved in this PR... |
Thank you! @knorth55 I will test thoroughly the memory issues dedicated to FFMPEG functions because it occurs literally because only of them. Thank you for notifying me and for your fix! I completely agree with you, we should merge #233 first because I had used the The question: if I discover that the node works normally in all accessible decoder modes, can I merge #233 by myself? I can test for all: YUV, MJPEG, H.264. And do we need to rebase again, what do you think? |
Yes, you can merge #233 when you finish tests and find no error. |
d9b7ac9
to
6d8948b
Compare
@knorth55 thank you! Can we now prepare the release of the package in its current state and then announce the release of the new version after this PR would be merged? What do you think? |
@twdragon |
- The node implemented as singleton with monopolistic device access model (one node per device) - Moved and separated into different blocks the the code using V4L2, ROS and FFMPEG APIs - Removed message pointers completely, the node uses runtime- constant shared object addresses - Debugged FFMPEG decoder algorithm, removed memory leak. Closes ros-drivers#211 - Added (using @flynneva's code) the conditional compilation directives to exploit properly versioned libavcodec API. Closes ros-drivers#207 - Closes ros-drivers#203 - Added separated launch files to run the node and to test it using image_view. Removed image_view from ROS dependencies. - Node parameters with convenient defaults moved from the launch file to YML file for the parameter server - Documented parameter names and default values
- Implemented copyless publishing to avoid blinks on some slow sensors
Co-authored-by: Shingo Kitagawa <[email protected]>
6d8948b
to
3c7405c
Compare
@twdragon No worries. I rebased. |
@tfoote Hi, sorry for bothering you many times. |
@knorth55 @tfoote I am a little late to the party here (sorry it has been busy start to the year for me) but I am pretty confused by this PR and the other ROS 1 ones you guys are targeting to the From #203 my understanding was the solution was:
That would mean all we would need to do is write small / thin wrapper for ROS 1 and add it to the This PR is copying over a lot of code from the |
@flynneva I think this is a kind of backport PR from
My plan is the first one or the third one. So the only problem is who will do the merging. |
@knorth55 @flynneva can I ask you guys for an opinion? As I prepared the version of this package with essential changes in user workflow and configuration, maybe it will be more convenient to fork and document the entire package and release it separately, let the users test it, and then merge after. What do you think? |
@twdragon i don't see why you can't just merge it here if @knorth55 approves of the changes. Just I've already shared my thoughts about these changes above, and I think a lot of it is duplicated work from the ROS2 branch already. I think it'd be better if we combine our efforts and work together on the same branch / code instead of developing the same features side by side. |
@twdragon Sorry for waiting you. I'm preparing the ROS1 release, so please wait to merge this PR. |
@twdragon I did the release of |
@twdragon i havent checked ros1 and ros2 merged pr. |
@knorth55 thank you! I also tested this version again and it does not seem to have major issues. So, if you and @flynneva agree, I can merge this PR next week, and then prepare a new README.md or wiki article to let the users try the new configuration workflow. Then we can think about refreshing the idea of merging ROS1/ROS2 branches in a more modular way. What do you think? |
@twdragon I agree. We can move forward to mege this PR to |
@knorth55 @flynneva, as I said before, I am ready to merge. I will merge this PR today and update the documentation on the ROS wiki (the old version of the documentation is already preserved) |
@knorth55 documentation http://wiki.ros.org/usb_cam updated |
@twdragon good! thank you! |