search

johanhelsing / matchbox

Painless peer-to-peer WebRTC networking for rust wasm (and native!)
Apache License 2.0
869 stars 72 forks source link

Add missing RTCIceCredentialType to the RTCIceServer #320

Closed tedsteen closed 11 months ago

tedsteen commented 12 months ago

Problem: The default RTCIceCredentialType is Unspecified and that will eventually end up crashing when used here because the return value will be Err since this

This PR will default to RTCIceCredentialType::Password. I don't know if that's what you would prefer, but it fixes my problem :)

simbleau commented 12 months ago

In my opinion, I think it makes more sense to submit a PR to webrtc-rs (to do nothing) when the credential is Unspecified. As you've shown, it returns an error by default. Not good.

They do nothing for RTCIceCredentialType::Oauth, I don't see why they can't the same (nothing) for Unspecified.

ref: https://github.com/webrtc-rs/webrtc/blob/71157ba2153a891a8cfd819f3cf1441a7a0808d8/webrtc/src/ice_transport/ice_server.rs#L44

tedsteen commented 12 months ago

In my opinion, I think it makes more sense to submit a PR to webrtc-rs (to do nothing) when the credential is Unspecified. As you've shown, it returns an error by default. Not good.

They do nothing for RTCIceCredentialType::Oauth, I don't see why they can't the same (nothing) for Unspecified.

ref: https://github.com/webrtc-rs/webrtc/blob/71157ba2153a891a8cfd819f3cf1441a7a0808d8/webrtc/src/ice_transport/ice_server.rs#L44

Doing nothing would also be a problem because then the password wouldn't be passed along, right?

simbleau commented 12 months ago

Oh, I see now. Yeah this PR makes sense to me.

johanhelsing commented 11 months ago

I'm still not completely sure if what webrtc-rs does is a good idea. I took the liberty of adding a commit that explains the issue and the workaround as I see it. Let me know if this matches/don't matches your understanding.