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 camera_info publishing #165

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

Eruvae
Copy link

@Eruvae Eruvae commented Jun 20, 2023

Currently, camera_info only gets published if there is a subscriber for image_raw, since they are bundled with a camera publisher. However, I have an application that only needs the rectified image, but also the camera info. In this case, the camera info is not received.

I changed the node so that the camera_info is published separately if there is no image_raw subscriber, but a camera_info subscriber. Additionally, the subscriber count is now only retrieved once before grabbing the image, because otherwise, there could potentially be a race condition when a subscriber is added after checking if an image needs to be grabbed.

Eruvae added 2 commits June 20, 2023 20:52
- Publish camera_info separately if no image_raw subscriber
- Read subscriber numbers only once to avoid race conditions
- allows access to publishers without casting to access private members
@Eruvae
Copy link
Author

Eruvae commented Jun 21, 2023

I added another commit to completely separate the image and camera info publishers. That has the advantage that there's no need to cast the camera publisher to access its private members, which could cause runtime errors if the implementation of the camera publisher is changed.

There are some side effects of that change. One thing the camera publisher does is to automatically remap the camera_info topic if the image topic is remapped. This would no longer happen. If that behavior is wanted, you'd need to figure out the camera_info topic like the camera publisher. I don't think that is necessary, since the automatic remapping can also have negative impact, like overriding manual remappings (see e.g. this issue). Also, you would usually just use a namespace for the camera node anyway. However, if the old behavior is preferred, this could be added, or you could just merge the first commit.

@FrancoisPicardDTI
Copy link
Collaborator

Hi @Eruvae
Thanks for your contributions and these explanations.
Before looking at your changes more thoroughly, do you confirm that the 2 commits are actually 2 different implementations and that I need to choose between one of them?

@Eruvae
Copy link
Author

Eruvae commented Sep 5, 2023

Yes, they are. The first commit keeps more of the old implementation, which used the kind of hacky way to retrieve the private members of the camera publisher. The second commit removes the camera publisher in favor of separate image and camera info publishers, which removes the need for casting (with the mentioned side effect, which could also be handled if required).
I'd personally prefer the second one, because the purpose of the camera publisher is to always publish image and camera info together. You could also do that, but then I would still remove the casting and just always publish both if either of the topic is being subscribed to (using the getNumSubscribers method of CameraPublisher directly).

@FrancoisPicardDTI FrancoisPicardDTI merged commit 2b4b5b6 into basler:master Oct 17, 2023
@FrancoisPicardDTI
Copy link
Collaborator

Hello @Eruvae
I merged your contribution. I took the second option, the one you recommended, which makes sense to me. Thank you again!

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.

None yet

2 participants