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 ice trickle support on wasm #16

Closed
wants to merge 4 commits into from
Closed

Add ice trickle support on wasm #16

wants to merge 4 commits into from

Conversation

johanhelsing
Copy link
Owner

@johanhelsing johanhelsing commented Feb 5, 2022

Following the same pattern as the native implementation. Now that both
wasm and native support trickling, we're one step closer to supporting
cross-platform connections.

TODO:

  • Get trickle wasm <-> wasm working
    • with negotiated = false?
  • Get rid of the ugly mutex hack
  • If webrtc-rs 0.5 is released, remove the candidate sdp_m_line_index hack.
  • Figure out why connections on the same machine between firefox and native fails during ice
  • Figure out why no data seems to get through on native <-> wasm, even though peer connection state is set to connected
  • Other todo comments

Issue: #7

@johanhelsing
Copy link
Owner Author

I tried with chrome <-> native, and there I do get a log message saying it's connected (!), and the demo game starts, though I don't get any data. It just seems to freeze and I get some warnings about unsupported extensions on the native side.

Firefox <-> native never changes to connected.

@johanhelsing
Copy link
Owner Author

@sapir I don't suppose you have any hunches as to what might be wrong?

@sapir
Copy link
Contributor

sapir commented Feb 5, 2022

to be honest, my own pr was the result of my staring at the (working) webrtc offer/answer example and comparing with your (practically identical) code and wondering what the difference was. I'll try to take a look later, though.

@sapir
Copy link
Contributor

sapir commented Feb 8, 2022

What I've found so far:

  1. The trickle listening future is being dropped; I'll open a pr with my fix. It still doesn't entirely work, though. I'm not getting communication even between my wasm instances
  2. To make it work more like the native code, the data channel in handshake_accept should probably be retrieved from the on_datachannel callback. I haven't actually tried it, so I don't know if it makes a difference.

@johanhelsing
Copy link
Owner Author

It still doesn't entirely work, though. I'm not getting communication even between my wasm instances

For me neither actually. It seems my last commit, the one that sets negotiated to false on wasm was the one that broke it. The commit before that works for me, though (even with the dropped trickle future).

To make it work more like the native code, the data channel in handshake_accept should probably be retrieved from the on_datachannel callback. I haven't actually tried it, so I don't know if it makes a difference.

Yeah, that seems like a good idea to check!

@johanhelsing
Copy link
Owner Author

Just noting down my progress as I have to put this down for now, and I'm not sure when I get a chance to pick it up again.

To make it work more like the native code, the data channel in handshake_accept should probably be retrieved from the on_datachannel callback. I haven't actually tried it, so I don't know if it makes a difference.

Yeah, that seems like a good idea to check!

I tried this now, and the good news is that I actually get some data coming through cross-platform now! The bad news is performance is horrible even on wasm <-> wasm, and sometimes the outgoing message buffer just fills up. Need to look this over and see if I made a mistake somewhere.

One thing that's different is that the trickle future is dropped as soon as the data channel is ready... So maybe it chooses some poor ice candidate instead of the optimal one? Guess I should read up on how ice trickle actually works. Does it "upgrade" the connection if it gets a "better" candidate? I should probably check the webrtc connection inspection tab in browser and compare against the working version and see if/what the difference is.

Another thing to do is add onerror/onclose callbacks and log there so we can better see what happens.

johanhelsing and others added 4 commits April 24, 2022 17:47
Following the same pattern as the native implementation. Now that both
wasm and native support trickling, we're one step closer to supporting
cross-platform connections.

Issue: #7
Also set it explicitly on native
@johanhelsing
Copy link
Owner Author

Rebased and fixed conflicts.

@johanhelsing
Copy link
Owner Author

johanhelsing commented Sep 8, 2022

I took a stab at this again today.

First, I updated to webrtc-rs 0.5 for the native side, that broke connections with negotiated set to true, so I disabled it on both the wasm and native side.

...and was able to get cross-play between chrome and native working smoothly! as well as chrome-chrome.

Sadly, firefox is giving me all sorts of grief. After connecting firefox-firefox, framerate for the bevy app drops to about 0.7, which I think makes ggrs just freeze indefinitely, so something really horrible is happening there. Connecting firefox to chrome or native give similar horrible results.

I pushed my work today to the wasm-trickle-and-webrtc-5 branch if anyone wants to investigate further.

It would probably also be interesting to see if the behavior is the same on linux and on other machines. And testing firefox-firefox across the network instead of just locally.

@johanhelsing johanhelsing added the enhancement New feature or request label Oct 26, 2022
@johanhelsing
Copy link
Owner Author

johanhelsing commented Oct 26, 2022

First, I updated to webrtc-rs 0.5 for the native side, that broke connections with negotiated set to true, so I disabled it on both the wasm and native side.

Inspecting this more closely, we used to use the API incorrectly, so pretty sure we have never used negotiated channels in the native implementation.

@johanhelsing
Copy link
Owner Author

Fixed in #54 instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants