[Community assistance needed] libavcodec multi-version compatibility

[twdragon]

Dear collaborators! Due to the Linux distributions being stick to different versions of libavcodec software stack, it is needed now to refactor a bit of the code and make it more modular and compatible with both old and new versions of libavcodec.

It is strongly encouraged to test the package on the different libavcodec versions and to create issues with feedback and bug reports. It is also encouraged to create pull requests with the introduction of the compiler rules making the code compatible with different versions of libavcodec API!

[twdragon]

#202 looks to have a root in FFMPEG version difference between Ubuntu 18.04 and 20.04

[flynneva]

@twdragon if we are smart about this redesign we could re-use the same libavcodec library wrapper for both ROS 1 and ROS 2, no?

  graph TD;
      ROS1_wrapper_node-->libavcodec_wrapper_lib;
      ROS2_wrapper_node-->libavcodec_wrapper_lib;
      libavcodec_wrapper_lib-->libavcodec

Loading

And then the user could swap in and out libavcodec versions if the libavcodec_wrapper_lib supports it without changing the ROS 1 / ROS 2 sides of things.

[twdragon]

@flynneva great offer! I will review this code a bit later. However, there could be an issue with passing data via pointers through the library API. I will investigate this and then, probably, we can provide a wrapper together. But anyhow, I will prefer the templatized C++ API header wrapper over the library. What do you think about the kind of implementation?

Also, we need to look at the changes in swscaler API because in his description of #202 @mistyk mentioned that the sws_getContext() function was not found either.

[flynneva]

@twdragon fyi I have a test branch and PR open on my fork that might be useful to you / us testing this concept of merging the two code bases (ROS 1 and ROS 2 branches).

[flynneva]

@twdragon I think I've got the ROS 1 and ROS 2 branches merged and compiling: flynneva#2

I dont have an Ubuntu 20.04 machine but if you or someone else could test to make sure the ROS 1 node still works using that branch maybe we could go ahead and merge it in.

[flynneva]

@knorth55 @twdragon so what is the plan here? I see you guys are doing a lot on the develop branch of this repo for ROS 1....and seem to be duplicating a lot of the work already done on the ros2 branch.

Can we add a ROS 1 node wrapper to the ros2 branch and deprecate the develop branch then instead of maintaining two different branches here?

What is your guys' strategy?

[knorth55]

@flynneva I think this is a kind of backport PR from ros2 branch to ros1.
Merging ros1 and ros2 branch is quite good idea, but it will need a lot of works.
There are two strategies to merge ros1 and ros2 branch

1. Use C++ macros
2. Use ros1-ros2 bridge
3. Write good CMakelists.txt

My plan is the first one or the third one.
I don't want to do the second one, because the ROS1 user needs to install ROS2.
Because there are a lot of API changes between ROS1 and ROS2, the first one will be difficult, too.
In order to do either 1. or 3., this backport PR is a good first-step for the plan.
(because ROS1 and ROS2 has different target platform, we need to use macros, though)

So the only problem is who will do the merging.
I don't use ros2, and have no benefits to do the task because there is ros1-ros2 bridge.
(Also, some parts of ROS2 are harder to use than ROS1, which I don't like ROS2)
The merging costs us a lot, so the plan is really a long path for me.

[knorth55]

@flynneva Oh, I didn't see flynneva#2 .
You did some jobs. Good job!
I will check the PR.

I implement ROS1 CI, so please check that, too.
We need to pay attention to merge the PR.
https://github.com/ros-drivers/usb_cam/blob/develop/.github/workflows/main.yml

BTW, why don't you use industrial_ci for CI?
Do you need some special build for CI?

[twdragon]

@flynneva @knorth55 guys, as I see, we all did a great job dividing ROS and V4L APIs inside the package in parallel. Thank you very much! In fact, we now have two variations of internal API splitting V4L and ROS parts. I had a look at the code and this is what I can say:

• @flynneva's part keeps track of perfect backward compatibility in terms of parameters and ROS interaction, so absolutely no changes from the existing configuration are required from the end user. In any case, it is a great job, thanks, @flynneva. At the same time, this approach keeps the model of access the old package provides for V4L parameters. This is what sometimes provokes errors on the cameras supported by V4L drivers but exposes the parameters in a way that cannot be handled with constraints implemented in the old access model.
• My variation of re-implementation of the internal API, thoroughly inspected by @knorth55 (thanks a lot!) strives to split all V4L/FFMPEG-related parts into a dedicated camera_driver library. It also removes the usage of the C-style shared pointer to the large singular object through the program in favour of a singleton camera controller object and one device = one node policy. The internal API is implemented via the C++ simple inheritance mechanism, so I hope that it can ease the division between ROS1 and ROS2 branches. However, as it suggests the new V4L parameters access model based on querying the driver directly, it may require the end user to reconfigure his old node after the update.

Since as @knorth55 mentioned, the merge is labour-consuming, I can suggest starting with thinking about what kind of internal API would be easier to use, as we have now two viable variations. What do you think?

[knorth55]

However, as it suggests the new V4L parameters access model based on querying the driver directly, it may require the end user to reconfigure his old node after the update.

I thought #231 is a back-porting PR, but it introduces new and different structure from ros2.
I didn't know that.
Hmm, IMO, we should keep the backward compatiblitty as far as possible.
So, I think it is better to split #231 into back-porting from ros2 PR and new feature PR.

How about applying @twdragon 's strutcture change first to ros2 branch?
My suggestion is:

1. Apply @twdragon 's structure change into ros2 branch, and leave ros1 branch now
2. Merge ros1 and ros2 branch

[twdragon]

@knorth55 as I mentioned here before:

@knorth55 No, you are not totally right. It involves some code from ros2 branch but actually, it is derived from a rewritten ROS1 version of this package I made for our robot with a customized ROS1 build. It is tested and I can prove it works properly in its current state. It implements the new model of working with V4L parameters and sets up constant addresses for most of the data structures used. This is not about the feature-based renewal but about the change of the implementation technique. So, I did not split the PR into the features, because they are the same

Thus, it is not actually a backport, but it proposes encapsulation of the all V4L/FFMPEG-related stuff in the dedicated library.

@flynneva @knorth55 do you think it is possible to make CMakeLists.txt and the build system able to handle properly package build on both ROS1 and ROS2?

[knorth55]

@twdragon Sorry, I didn't ask you deeply, especially about backward compatiblity.
but if you break the backward compatibility, it is a big change and problem.
The change affects many other projects, so we should be careful.
Also rosparameter's change is hard to realize, because there is no error when you set useless ros parameter.

@twdragon
I think it is possible, but I want to find other projects which merge ros1 and ros2 branch.

1. Apply @twdragon 's structure change into ros2 branch, and leave ros1 branch now

2. Merge ros1 and ros2 branch

Also, my suggestion order can be inversed.
So we can merge ros1 and ros2 branch first, and apply @twdragon 's structure change.

[flynneva]

do you think it is possible to make CMakeLists.txt and the build system able to handle properly package build on both ROS1 and ROS2?

@twdragon yes. That is exactly what my branch / PR above proves is possible: flynneva#2

So we can merge ros1 and ros2 branch first, and apply @twdragon 's structure change.

@knorth55 @twdragon feel free to take the changes I have in flynneva#2 and make an MR to the ros2 branch on this repo. We can then work together to enhance the underlying usb_cam lib and it will benefit both ROS 1 and ROS 2 👍🏼

[twdragon]

@flynneva excellent! I will examine your MR and try to prepare it for merge. Can I rely on your experience and consider a successful build on ROS2 is already done? I should ask because I do not use ROS2 in my work

[twdragon]

Also, my suggestion order can be inverted.
So we can merge ros1 and ros2 branch first, and apply @twdragon 's structure change.

Optimal way, I see, @knorth55. I agree

[twdragon]

@twdragon Sorry, I didn't ask you deeply, especially about backward compatiblity.
but if you break the backward compatibility, it is a big change and problem.
The change affects many other projects, so we should be careful.
Also rosparameter's change is hard to realize, because there is no error when you set useless ros parameter.

In fact, the backward compatibility is maintained almost automatically because if you do not set any parameters the camera is configured automatically by the V4L driver with defaults determined via USB profile data exchange. I decided to leave then the camera "as is" in case the user configuration is not feasible because this autoconfiguration rarely provokes errors (excluding some built-in laptop cameras).

About understanding the new parameter handling model: I discovered that on some cameras (for example, on Dell Latitude 3470 built-in embedded USB webcam) some parameters (white_balance_temperature_auto, exposure_auto_priority) are implemented as so-called V4L menus, which disables them from being set in the old usb_cam node configuration. Thus, I decided to query V4L2 driver infrastructure before running a node, to retrieve the list of parameters and their types directly from the kernel. Also, I discovered in my robotic project with a heterogenous setup with multiple cameras, that there are huge differences in the lists of supported controls between the camera models. In #231 you can see that the node now generates ROS parameter names automatically from the retrieved list of available V4L controls, and handles their types (INT, INT64, MENU or BOOLEAN) in a proper way. To avoid then the unpredictable behaviour with wrong values, it also has an ignore list, and when the parameter is listed there, it will not be set, otherwise, it will be set to the value specified by the user or the default value (retrieved again from the driver) if the user-specified value is not feasible. This is why, as @knorth55 mentioned, it does not stop in the error state when a useless parameter name is configured: the system just ignores it leaving the other parameters with default values. Also there is a dedicated namespace needed for V4L controls, otherwise we have a risk of unexpected parameter name match, then I decided to group V4L control-mapped ROS parameters under intrinsic_controls YAML namespace.

[knorth55]

This week, i cannot make time to work on it.
So next week, i will check the ROS1 and ROS2 merging PR.
the first step is setting CI.

@flynneva i want to ask you a question about CI.
you are using custom ci instead of industrial_ci for ros2, but can you tell me the reason?
im setting industrial_ci for ros1, but i want to know the difference between the current ros2 ci and industrial_ci.

[twdragon]

@flynneva @knorth55 as I did before, I can also suggest after merging ROS1 and ROS2 branches announcing a new parameter handling method on discourse.ros.org when we will decide to do.

[flynneva]

@knorth55 what is industrial CI? I've never heard of it.

The PR I linked to above also included a rewrite for the CI to also run ROS 1 and ROS 2 in CI for each pull request. Check it out and let me know if you have any questions.

Feel free to copy+paste it as well and make changes as you see fit.

[knorth55]

@flynneva
industrial_ci is a useful ROS CI package.
https://github.com/ros-industrial/industrial_ci
the package will install all dependencies in package.xml, and run build and test.
I use industrial_ci for develop branch for usb_cam, and it is really easy to use.

I use industrial_ci for ROS1 CI of usb_cam.
https://github.com/ros-drivers/usb_cam/blob/develop/.github/workflows/main.yml

In audio_common package, I use industrial_ci for ROS2 CI.
https://github.com/ros-drivers/audio_common/blob/ros2/.github/workflows/ros2.yml

[flynneva]

@knorth55 I really like the solution that I came up with on the PR I mentioned above: flynneva#2

Have you looked at it yet?

I am hesitant to migrate to industrial_ci since there exists an official ROS tooling alternative: https://github.com/ros-tooling/action-ros-ci that we currently use today.

Are there features that are on industrial_ci that are not already available in action-ros-ci?

[knorth55]

@flynneva I checked flynneva#2.
the parsing active distro is quite good idea!
One point is that melodic is still active.

the big difference is that industrial_ci is available for github actions, travis CI, gitlab CI, jenkins and so on.
industial_ci exists since many years ago, and developed originally for travis CI and jenkins.
action-ros-ci, which I didn't know, is a new tool and only for github actions.
I'm not familiar with action-ros-ci, but if it can build and test package, it is almost same as industrial_ci.

BTW, can you also check this issue ros-gbp/usb_cam-release#2 ?
I want to release usb_cam with current develop branch.
can you give me write permission to release repo?

[flynneva]

I'm not familiar with action-ros-ci, but if it can build and test package, it is almost same as industrial_ci.

@knorth55 it can and does. I think we should stick with the official ros-tooling provided tooling.

can you give me write permission to release repo?

@knorth55 this isn't really related to this thread at all, but sure. It is all managed via terraform, so all you need to do is add yourself here and make a PR. Tag me in the PR and I can approve in case they need my approval.

[flynneva]

@knorth55 @twdragon can either one of you take what is in flynneva#2 and make a PR to this repo's ros2 branch? We can start combining the ROS 1 and ROS 2 branches that way and then work together to close out the open issues.

[knorth55]

it can and does. I think we should stick with the official ros-tooling provided tooling.

i agree.

It is all managed via terraform, so all you need to do is add yourself here and make a PR. Tag me in the PR and I can approve in case they need my approval.

I didnt know that ros2 has the good management system.
i will make a PR

[knorth55]

can either one of you take what is in flynneva#2 and make a PR to this repo's ros2 branch? We can start combining the ROS 1 and ROS 2 branches that way and then work together to close out the open issues.

@flynneva
I will check it but im busy in next week, so i will do it next next week.
before merging the branch i want to make a release for ros1, so please wait for the release.
i will do the release after i get the permission of usb_cam-release