RobotWebTools / webrtc_ros

Streaming of ROS Image Topics using WebRTC
Other
131 stars 52 forks source link

add ICE to server #44

Closed mjsobrep closed 4 years ago

mjsobrep commented 4 years ago

In reference to #39 and #24. This adds the default google stun servers. Combined with something like this You can get the system to work over the web in most situations.

I'm putting this in as a draft for now. Hardcoding in the stun servers is probably not what anyone wants. I also don't have TURN in there yet. I think for TURN what I am going to do is allow users to specify a service to call to get TURN credentials via whatever means the user asks for (REST, permanent, etc.). I will allow a flag to be passed into the webrtc server nodes whether to use that or not.

The alternative would be to pass all of the ICE info from the client to the server through the existing messaging architecture. That actually seems nicer for my applications, but quite a bit harder to implement and not as flexible for other users.

Let me know your thoughts.

A few important links:

mjsobrep commented 4 years ago

I think this is ready to review. It works for me, but I have admittedly not tested all edge cases. In fact, the only test I have run is through this launchfile with the setup described in that repository using coturn as the turn server and the remote endpoint programmed here.

I did not add ICE servers to the example app since I can't imagine how that would be accessed in such a way as to need any sort of NAT traversal. But that wouldn't be hard to do if desired.

I tested the TURN functionality using this trick

mjsobrep commented 4 years ago

Splitting that logic out allows someone else to easily replace the extra node with whatever they want and just make their own launch file. I implemented it using a rest api to get credentials. That is probably the most general purpose approach, but it is not the only approach that is viable. Someone else might have a database on their turn server with fixed usernames and passwords or a rest api that has a different authentication method, JWT for example. I didn't want to make people have to drop into the webrtc server to make those changes.

roehling commented 4 years ago

Understood.

Is this ready to merge, or do you still want to work on this PR?

mjsobrep commented 4 years ago

Ready to merge

roehling commented 4 years ago

Thank you for your work!

mjsobrep commented 4 years ago

Happy to help out. Is it possible to update the built version installed via package managers?

roehling commented 4 years ago

Yes, I will push a new release to ROS soon(ish).

roehling commented 4 years ago

https://github.com/ros/rosdistro/pull/24639 https://github.com/ros/rosdistro/pull/24640