kraken icon indicating copy to clipboard operation
kraken copied to clipboard

Added functionality for listen only

Open sethkimmel3 opened this issue 5 years ago • 8 comments

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 avatar Sep 04 '20 04:09 sethkimmel3

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 avatar Sep 13 '20 03:09 sethkimmel3

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 avatar Sep 18 '20 02:09 sethkimmel3

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 avatar Sep 21 '20 01:09 sethkimmel3

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?

sethkimmel3 avatar Oct 15 '20 03:10 sethkimmel3

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.

cedricfung avatar Oct 17 '20 17:10 cedricfung

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.

sethkimmel3 avatar Oct 18 '20 19:10 sethkimmel3

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

jbenguira avatar Feb 27 '21 18:02 jbenguira

@jbenguira I'm working on this feature.

cedricfung avatar Feb 27 '21 19:02 cedricfung