MixinNetwork / kraken

🐙 High performance WebRTC SFU implemented with pure Go.
Apache License 2.0
335 stars 49 forks source link

Added functionality for listen only #7

Open sethkimmel3 opened 4 years ago

sethkimmel3 commented 4 years ago

Hey @cedricfung - here is the PR for the listen only methods as promised.

The client will call the RPC method 'registerListenOnlyPeer' to create a peer which has a cid set to a listen only identifier. This is in lieu of calling the publish method. The user can then proceed as normal, repeatedly calling the subscribe method.

This code seems to be working but is not substantially tested yet. If you have any suggestions of how to do so, please let me know.

sethkimmel3 commented 4 years ago

Hey @cedricfung - I just added the changes to reflect the design you had suggested. It's very different (and much simpler) than my last iteration. However, I'm having quite a hard time setting up an environment to make sure it functionally works. I figured I would add the changes for now anyway so we can have a dialogue on it while I'm working on getting it tested.

sethkimmel3 commented 4 years ago

Hey @cedricfung - I finally got my test environment working and am making some changes. One gnarly issue I'm running into is with the proposal you made here to have a peer not have an SDP.

I tried both:

  1. Adding an empty webrtc.SessionDescription which resulted in a 500305 error from kraken.
  2. Changing the create method so that it skips adding the remote description for a listen only peer, which resulted in a 500306 error from kraken.

Here are some options I see of fixing this:

  1. Going back to the design of creating a separate registerListenOnlyPeer before the subscribe method so that the client can supply a valid session description as a parameter.
  2. Having a new optional subscribe parameter which takes a valid session description as a parameter.
  3. Doing some clever webrtc workaround which I am not experienced to know about.

Let me know what you think if you can in the next couple of days, otherwise I'll just proceed with the plan I see as most sensible.

sethkimmel3 commented 4 years ago

I decided to revert back to the original design of having a separate registerListenOnlyPeer method, as I believe that is the simplest/cleanest way of adding the functionality without changing too much (almost anything) or what is already written. I was able to test this code in production and it appears to be working just fine.

Please let me know if there are any changes you would like to see here before approving.

sethkimmel3 commented 4 years ago

Hey @cedricfung - do you have any updates here? I've been using my forked version to test my app for about a month now and everything appears to be working fine with listen-only mode. Is there anything you'd like me to change before approving?

cedricfung commented 4 years ago

Sorry for the delay. I have tried to merge this and do some code refactor. But found it would make code too complicated. However I am still going to make progress on this, so leave this open.

sethkimmel3 commented 4 years ago

No worries @cedricfung - certainly don't want to mess up your codebase. But this functionality is critical for my specific use-case, so I'm going to just use my forked branch until it gets rolled in. Let me know when you take another look at it and if you'd like any further help from me.

jbenguira commented 3 years ago

Hey @cedricfung any news about this? The feature is super hot right now (clubhouse effect) 🥳

cedricfung commented 3 years ago

@jbenguira I'm working on this feature.