Conversation
| if (needs_restart && result.successful) { | ||
| stop_pipeline(); | ||
| start_pipeline(); | ||
| } |
There was a problem hiding this comment.
If we do need a restart, but "result.successful" is false, we ignore the restart?
There was a problem hiding this comment.
Also, result.successful is never modified and is set to true at the start of this function call? Do we even need to check it?
| int bits_per_frame = video_bitrate / target_framerate_; | ||
| if (bits_per_frame < 50000) { | ||
| set_resolution(640, 360); | ||
| } else if (bits_per_frame < 100000) { | ||
| set_resolution(854, 480); | ||
| } else if (bits_per_frame < 200000) { | ||
| set_resolution(1280, 720); | ||
| } else { | ||
| set_resolution(1920, 1080); |
There was a problem hiding this comment.
Consider parameterizing this and allowing the "resolution-bitrate ramp" to be defined at runtime.
There was a problem hiding this comment.
Yeah I am probably changing this whole setup. bits_per_frame is a terrible measurement for h265. Lower framerates compress way worse per frame than higher framerates. Also I don't like hard lines that it might bounce back and forth over. I would say overall this simple logic is fine for the next few weeks of testing.
| GError *err = nullptr; | ||
| GstElement *p = gst_parse_launch(desc.c_str(), &err); | ||
|
|
||
| if (err || !p) { |
There was a problem hiding this comment.
It might be standard to do nullptr checks like this in modern C++, IDK. But I think explicitly presenting the comparison is generally better.
i.e.
p == nullptr
There was a problem hiding this comment.
Modern C++ you typically explicitly compare to nullptr. This is very much a c idiom. This whole node follows the c style since gstreamer is a c library.
|
|
||
| void RtpNode::on_iframe_trigger(const std_msgs::msg::Empty::SharedPtr msg) { | ||
| (void)msg; | ||
| if (h265_encoder_) { |
There was a problem hiding this comment.
If casting to void to avoid compiler warning, C++17 lets you do the following,
void RtpNode::on_iframe_trigger([[maybe_unused]] const std_msgs::msg::Empty::SharedPtr msg)
Or just omitting the parameter name and inline commenting it, (pretty much any C++ version)
void RtpNode::on_iframe_trigger(const std_msgs::msg::Empty::SharedPtr /*msg*/)
Doing the void cast is an old way to do it i guess.
There was a problem hiding this comment.
void cast is again a c idiom which is just kind of habit for me at this point. I spend most of my day in c and rust. I'll switch to [[maybe_unused]] since we are using c++17.
3 participants
This pull request introduces RTP (Real-time Transport Protocol) streaming support to the video streaming package, alongside improvements to the pipeline management and launch configuration. The main changes add new RTP nodes for both server and client functionality, update launch files to use these nodes, and enhance dynamic parameter handling and pipeline control.
RTP Streaming Support
RtpNodeandRtpClientNodeclasses for RTP server and client functionality, including pipeline creation, parameter handling, and dynamic bitrate/resolution adjustment (src/Cameras/video_streaming/include/video_streaming/rtp_node.hpp,src/Cameras/video_streaming/include/video_streaming/rtp_client_node.hpp,src/Cameras/video_streaming/src/rtp_node.cpp,src/Cameras/video_streaming/src/rtp_client_node.cpp). [1] [2] [3] [4]src/Cameras/video_streaming/CMakeLists.txt). [1] [2]Launch Configuration
src/Cameras/video_streaming/launch/rtp_client.launch.py,src/Cameras/video_streaming/launch/video_streaming.launch.py). [1] [2] [3]Dynamic Pipeline and Parameter Handling
src/Cameras/video_streaming/src/rtp_node.cpp,src/Cameras/video_streaming/src/rtp_client_node.cpp). [1] [2]Input Node Improvements
InputNodeto track the current video request and ensure proper callback invocation when the pipeline is created or successfully started (src/Cameras/video_streaming/include/video_streaming/input_node.hpp,src/Cameras/video_streaming/src/input_node.cpp). [1] [2] [3]