-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Default to using Google STUN server for WebRTC player #12084
Conversation
Hi @chriswiggins, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Hi, I don't think we're going to add this dependency on Google here directly, without giving users an option to adjust it. I would assume that would need some options from home assistant core, but we would need to think through the best way to do that, e.g. architecture issue |
const peerConnection = new RTCPeerConnection({ | ||
iceServers: [ | ||
{ | ||
urls: ["stun:stun.l.google.com:19302"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't send traffic to google without warning users about it and getting consent.
WebRTC requires knowing both the "client" and "server" IP addresses. I have verified that just having the server public IP (i.e HomeAssistant) still doesn't make the stream work. WebRTC is a two-way communication channel, even though in this instance the client is only receiving. So this is definitely required to make external communication work. I agree that we shouldn't send traffic to Google without the user's consent, so how can we go about adding that in as an option? HA development is all very new to me so if you point me in the right direction, I can open the relevant PRs required to do so. In my eyes, we should give the users an option to set up STUN, and recommend Google's STUN server, but give them the ability to set it to whatever they like |
I was asking if this actually solved it for you before going into next steps since it's likely not trivial. So, I think if you can confirm that you've solved webrtc remote access then I can help you navigate how to do it. I can walk you through options and brainstorm a bit but first wanted to verify it'd be worth it. (I omitted this from the initial pr intentionally to avoid having to figure out the solution) |
I tried out this change and it didn't make remote access work for me, as far as I can tell. I think its still interesting to explore how to configure this, and this may be /a/ missing piece, but its not the missing piece. |
I wonder if it needs to better handle trickle ice candidates, as described here: https://groups.google.com/g/discuss-webrtc/c/UOnopWJ1l44 |
I was able to get this to work by inserting a blocking step to capture the ice candidates and add them to the offer SDP:
Then below when sending the offer, send the offer with the extra The offer SDP now includes the candidates that include the public IP address resolved via the STUN server. Can you try this out and let me know if it works for you? If so then we can talk about how to properly allow configuration of the stun servers. |
Will do - I run HASS.io on a Pi, so the frontend testing isn't exactly trivial. Will get back shortly |
Thanks, just want to make sure I am not imagining things. I think there is probably a way to make the client include the ice candidates in the offer itself but haven't figured out the right set of calls yet. |
I'm thinking something along the lines of this (waiting for the dev frontend to build): const q = new Promise<void>((resolve) => {
peerConnection.addEventListener("icegatheringstatechange", (event) => {
const connection = event.target;
if (!connection) return;
if (connection.iceGatheringState === 'complete') {
resolve();
}
});
});
await q; But ultimately we really need a way to send ICE candidates through via trickle, because this is a bit of a hack (but I have done similar in the past with GStreamer to get around trickle) |
@hunterjm and I discussed some options for this yesterday and thought this direction would make sense:
I would be happy to work with you if you have any questions about any of this (e.g. feel free to discuss w/ us in the home assistant discord) and can review all the PRs. |
I might be able to help out here as well. Just let us know what you need @chriswiggins. |
@chriswiggins I had some spare time so I played around with this in home-assistant/core#72574 Let me know if you'd like to try to integrate the websocket call into this PR? or I can follow through on this if other priorities are on your plate |
Hi @chriswiggins -- we typically like to only merge the core PR when the frontend PR has confirmed that it has all it needs from core. That is, i think a next step is to have the frontend integrate this new websocket call to get the stun server and confirm it works and has all we need. Are you planning to do this anytime soon or should I take what you have here and send another PR to do it? Happy to have you do it, but just want to set my expectations in case you've moved on |
Hi team, I will endeavour to update this today / tomorrow - will update the PR and get back to you |
Awesome! No rush just wanted to know if this was still on your plate. I am happy to wait until you pick this up, and am pleased to have you push it forward. |
I've sent #13942 to replace this PR. |
Proposed change
Defaults the
ha-web-rtc-player
to using Google's STUN server (stun.l.google.com:19302
) so that external WebRTC connections have a better chance of working.The new WebRTC integration doesn't work for me remotely, so I've gone and opened a PR on both the RTSPtoWeb and stream-addons repos so that these use STUN on the server side, but we also need STUN on the client side for proper two-way NAT traversal. This change doesn't require the other two PRs to be merged, FYI.
Type of change
Example configuration
No config as-of-yet, if someone can point me in the right direction as to where this sort of config should sit, I can implement
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: