Skip to content
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

Add missing RTCIceCredentialType to the RTCIceServer #320

Merged
merged 4 commits into from
Sep 21, 2023
Merged

Add missing RTCIceCredentialType to the RTCIceServer #320

merged 4 commits into from
Sep 21, 2023

Conversation

tedsteen
Copy link
Contributor

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
Copy link
Collaborator

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

@simbleau simbleau added bug Something isn't working blocked Progress cannot be made currently labels Sep 15, 2023
@tedsteen
Copy link
Contributor Author

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
Copy link
Collaborator

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

@simbleau simbleau removed the blocked Progress cannot be made currently label Sep 16, 2023
@johanhelsing
Copy link
Owner

johanhelsing commented Sep 21, 2023

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.

@johanhelsing johanhelsing merged commit f3a228b into johanhelsing:main Sep 21, 2023
PraxTube added a commit to PraxTube/matchbox that referenced this pull request Oct 22, 2023
This is already fixed in johanhelsing#320
but is only on `main`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants