video_stream_opencv icon indicating copy to clipboard operation
video_stream_opencv copied to clipboard

Published 20 hours ago • verified publisher icon ros-drivers

Almost a major refactor

Open kunaltyagi opened this issue 7 years ago • 2 comments
trafficstars

This code uses C++11 and CMake 3.1. I don't know what the compatibility requirements are. If you are not able to merge this, no issues.

Modified the code to have the following:

  • reuse of ImageCapture logic elsewhere (not dependent on ROS)
  • looping to enable endless debugging of videos, images and image_sequences
  • error count to allow for resetting streams in case of loss of connection (to be used together with looping)
  • some code cleanup (and addition of dirty macro)
  • allow for easier support for ROS2 in future (only utility.hpp needs to be changed for that)
  • removed usage of experimental boost thread sync_queue

kunaltyagi avatar Jun 26 '18 09:06 kunaltyagi

Hello. Sorry for the late reply, I was away for a few weeks. Thank you very much for your efforts.

As you say... this is mainly a complete rewrite. I don't feel confident merging it. At least we'd need to test every old possible use case to make sure it still works as before. Said that, I have the following concerns:

  • About CMake 3.1, Ubuntu 14.04 has version 2.8.12.2, and this package is supported on ROS Indigo, so I would rather not merge that. Is it really necessary?

  • About C++11, I'm not a C++ expert, so I don't know if that has any backwards compatibility problems either.

I do like the capability of looping and also pausing the play. Couldn't that be implemented in the previous code base?

awesomebytes avatar Jul 11 '18 03:07 awesomebytes

Regarding the versions

  • CMake 3.1 is required for setting -std=c++11 in the best practices. We can always use set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
  • I've removed Boost as a dependency, hence the need for C++11. AFAIK, it requires a fairly recent compiler (though as per this, the compilers have been around for a long time)
  • Indigo support has long gone, but I understand your wish to keep it in place. Would a nomenclature like other ROS repositories work? (indigo-devel for indigo, and master for the rest?)

Regarding the refactor:

  • No, it's not requried. I'm using the innards for my work and I refactored the code anyways. Thought the refactor might be useful to the community as well.
  • It's pretty simple to add the changes I made without refactoring

TLDR: This PR is just a refactor with minor changes. Makes it easier for developers, but not required to be merged upstream. The minor changes can be cherry-picked and merged.

kunaltyagi avatar Jul 11 '18 10:07 kunaltyagi