Skip to content

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
Member

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
Member

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
Member

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
Member

Oh cool it just just didn't work

@yameatmeyourdead
Copy link
Contributor Author

kernel device tree modified to work with spi, camera set as device tree overlay with spi being merged into base fdt

@Hermanoid
Copy link
Member

I apparently forgot to push my parameterization. The test stream opens on my machine but I can't connect to it - I get a 403 Service Unavailable. This is dumb but I've reason to suspect it's my machine, since the same test stream appears to work on Zac's machine and the Jetson. @yameatmeyourdead could you confirm that this code works on your machine?

@Hermanoid Hermanoid marked this pull request as ready for review May 1, 2025 02:09
Copy link
Member

@Hermanoid Hermanoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good 👍

@Hermanoid
Copy link
Member

@yameatmeyourdead MERGE IT

@Hermanoid
Copy link
Member

Technically missing the gstreamer-went-kaput concern we mentioned earlier but who needs safety measures?

@Hermanoid Hermanoid merged commit 2991473 into main May 3, 2025
1 check passed
@Hermanoid Hermanoid deleted the camera-4.0 branch May 3, 2025 04:25
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.

4 participants