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

Camera 4.0 #50

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from
Draft

Camera 4.0 #50

wants to merge 30 commits into from

Conversation

yameatmeyourdead
Copy link
Contributor

Rips out rov_cameras and replaces it with new cameras node NAME TBD (old files to be deleted)

Utilizes GST RTSP Server for dynamic streaming requests
Supports multiple sinks for each source stream
Basically 0 latency
Currently only supports the two MIPI ports

ROS API TBD, we still need to have a feature brainstorming for this node I guess, but a basic node has been created.

This will support literally any future camera, just create a new PIPELINE const char def in cameras.cpp, add a mapped port, and create the camera object in the node's constructor.

Love to finally make progress on something thats been nagging me for >1 year
Winning.

camera4_0_reduced.mp4

@yameatmeyourdead yameatmeyourdead added enhancement New feature or request help wanted Extra attention is needed labels Feb 14, 2025
@yameatmeyourdead
Copy link
Contributor Author

looks like some gstreamer development libraries are not installed in the CI's image. Need to bump the CI's tagged image to apt install those

@yameatmeyourdead
Copy link
Contributor Author

Also worth testing if we can remove elements from the pipeline. nvvidconv might be able to be removed. IDK if nvv4l2h264enc can read from NVMM or not. If it can, we can significantly reduce CPU usage spent just copying VRAM to RAM to encode as x264 to then push through a socket.

@Hermanoid
Copy link
Contributor

Relating to your questions from discord:

  1. Do we want to be able to dynamically register cameras?
    1. No! Well, sorta. No reason to do this through services, it's unnecessary complexity.
    2. I think a better solution would be to make the camera list a ROS configuration value
    3. We could technically make a parameter change listener and change the camera steams when the parameters are changed via ROS - but how often are we actually going to need that. Just change the YAML file and restart the node.
  2. Recreate cameras in case something weird happens?
    1. How know when weird thing happen???
  3. We don't really need to ever publish these images with ros. RTSP streams support multiple endpoints so we'd just be using a gstreamer client endpoint for those applications.
    1. If the node is not ever publishing via a ROS topic, we're not using ROS services to change configuration, and we're really only using ROS for it's configuration system - do we really need this to be a node?
    2. To answer my own question: Yes, if only for the convenience of being able to ask ROS stuff like "yo is the node running" and "wait what is the current camera configuration?" We might also come up with stuff we want to report via a topic later

@Hermanoid
Copy link
Contributor

Hermanoid commented Feb 20, 2025

Code itself looks pretty decent, except for three changes I would propose:

  1. Un-hardcode-afy the list of cameras (use configuration file for: Camera ID, Camera Port, GStreamer Pipeline String)
    1. Dynamic changeability is definitely an optional feature - probably not.
  2. Delete rov_cameras, rename the cameras package you added to rov_cameras.
  3. Bump that image so the CI check passes :)

@Hermanoid Hermanoid self-requested a review February 20, 2025 01:46
@yameatmeyourdead
Copy link
Contributor Author

Relating to your questions from discord:

1. Do we want to be able to dynamically register cameras?
   
   1. No! Well, sorta. No reason to do this through services, it's unnecessary complexity.
   2. I think a better solution would be to make the camera list a ROS configuration value
   3. We could technically make a parameter change listener and change the camera steams when the parameters are changed via ROS - but how often are we actually going to need that. Just change the YAML file and restart the node.

2. Recreate cameras in case something weird happens?
   
   1. How know when weird thing happen???

3. We don't really need to ever publish these images with ros. RTSP streams support multiple endpoints so we'd just be using a gstreamer client endpoint for those applications.
   
   1. If the node is not ever publishing via a ROS topic, we're not using ROS services to change configuration, and we're really only using ROS for it's configuration system - do we really need this to be a node?
   2. To answer my own question: Yes, if only for the convenience of being able to ask ROS stuff like "yo is the node running" and "wait what is the current camera configuration?" We might also come up with stuff we want to report via a topic later
  1. agreed that some sort of parameter system or configuration file is probably best here rather than services to get/set values. Don't imagine these changing dynamically while the code is actually running.

  2. Say the camera driver has an oopsie woopsie and causes gstreamer to melt down and die, the thread that runs that camera will not currently handle that. A service to forcibly restart might be a good idea. We don't want to end up in the same scenario as the ESCs where we can't do a reset on the fly when we need to without turning it off and back on. This will require some reconfiguring to store the g_main_loop pointers outside of the std::thread local contexts and pass the pointers in.

  3. It doesnt currently need to be a node, but the alternative is just having a ros package with a non-ros executable that is called from a launch file. I think its fine to keep it as a node. Who cares if a node is just hanging around and only has a couple sliders? Theres also maybe possibility to add dynamic camera configuration controls to allow dynamic changing of stuff like resolution/fps that doesn't involve just modifying the default pipelines

@Hermanoid
Copy link
Contributor

The CI build should be working, huzzah.
Still want to add configuration-ability (of the list of cameras, I think dynamically changing fps/resolution can be a feature add after this PR is merged) and oops-it-broke recover-ability.

@Hermanoid
Copy link
Contributor

Oh cool it just just didn't work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants