Change UDP video streams to use an address:port #10974
Conversation
|
Anything that affects UI needs screen shots |
|
Nevermind. Simple enough to see minimal UI changed.
Yes, you need to add code to VideoSettings to migrate any old value to the new one and remove the old value. |
I wonder about this part as well. Does RTSP or TCP url setting allow this? If not these should match how those work. Either port only is allowed everywhere or full url is required everywhere. I think consistency is more important here. Might also be nice in all of these URL fields to add ```placeholderText`` which shows the required url format. |
| port_validated = uri.midRef(url.scheme().length() + 3).toInt(); //Defaults to '0' on failure, +3 to account for '://' | ||
| } | ||
|
|
||
| if (port_validated < 1025) { |
There was a problem hiding this comment.
why this assumption?
root can open every port they want.
I see some rare cases when sbd would use existing firewall rules or other "fixed" ports to push stream through for example via port 80.
| "default": 5600 | ||
| "name": "udpUrl", | ||
| "shortDesc": "Video UDP Url", | ||
| "longDesc": "UDP url address and port to bind to for video stream. Example: 192.168.143.200:3001", |
There was a problem hiding this comment.
this is confusing for people that are not familiar with interfaces binding, including myself like 2-3 years ago.
that is: 0.0.0.0:5600 bind to all existing network interfaces
127.0.0.1:5600 bind to loopback - not seen from network, even from LAN
192.168.143.200:5600 bind to this particular itf, so if a device has let say, another itf QGC will not receive stream on that itf.
when I knew nothing about those "rules", I'd think that 192.168.143.200 would be ip of a sender.
I'd also add as an example port 5600 which is a standard, that may also support better understanding of this setting
There was a problem hiding this comment.
I'm still thinkign about this.
Since this is a bit complicated for average user (believe me!), maybe we could add a question mark with tooltip with wider explanation about those "rules". I know they should just read in wiki or elsewhere but I have a feeling that little help might be a great support for our users.
There was a problem hiding this comment.
This change allows to enter ip, but it is not enabling support for multicast.
It needs changes in gstreamer pipeline config to allow use of muticasted receive of stream.
See https://developer.ridgerun.com/wiki/index.php/Using_UDP_Multicast_with_GStreamer
|
Thanks for the feedback zdanek and DonLakeFlyer. I haven't had a chance to circle back on this and clean it up, but I aim to do so at some point in the next few weeks. The GStreamer does not need any adjustments, at least not for recent releases of GStreamer. I have not confirmed specifically with the recommended release, but I can confirm that this patch does enable multicast across a fairly wide range of releases (tested on arm64 and amd64 Linux builds, and a Windows build). If there's additional work to be done ensuring that the GStreamer side is also compatible, then perhaps that's another issue/PR for the "back-end" work. Regarding the needed changes for address/port layout, and the notes on how to actually pick the right bind address, I do agree with the points raised. I think I'll make an adjustment to instead add a new parameter for the bind address. This should make it easier to parse when actually using the setting, as well as being a bit easier to describe in the tooltip. I have not done it in the original patch, but it would be smart here to make sure the new code works for IPv6 as well, I'll throw that into the mix too. Not sure if there is any platform compatibility concerns for IPv6, but it seems unlikely. Regardless, I will check if there's a sane way to do this generally for the targeted platforms, any perhaps the default value for the new bind address parameter can just be blank? That way we could detect and use the right address space for a given platform. This would also mean that people that have no interest in selecting the correct bind port can just clear the parameter and not have to think about it. |
This allows for the following specific benefits:
Provision has been made to allow for just a port number to be entered to keep previous functionality. However, the settings fact has been renamed so perhaps something else must be done to migrate existing settings. Alternative would be to leave the settings fact listed as udpPort but keep the new functionality.