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

FAQ entries on how to use ion-sfu #496

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

FAQ entries on how to use ion-sfu #496

wants to merge 2 commits into from

Conversation

grahamking
Copy link
Contributor

All written by @leewardbound

Until now we didn't provide any direction on how to actually use ion-sfu. Now we do!

@grahamking grahamking added the documentation Improvements or additions to documentation label Mar 29, 2021
@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #496 (09d42c7) into master (7be82ab) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #496   +/-   ##
=======================================
  Coverage   41.69%   41.69%           
=======================================
  Files          23       23           
  Lines        2240     2240           
=======================================
  Hits          934      934           
- Misses       1184     1185    +1     
+ Partials      122      121    -1     
Impacted Files Coverage Δ
pkg/sfu/router.go 47.64% <0.00%> (-1.18%) ⬇️
pkg/sfu/subscriber.go 62.41% <0.00%> (+1.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7be82ab...09d42c7. Read the comment docs.

@grahamking
Copy link
Contributor Author

@tarrencev @OrlandoCo Please edit the text as you see fit. This came out of a long discussion on Slack between @leewardbound , @billylindeman and myself.

FAQ.md Outdated

- "All RPC" - package is jsonrpc + grpc at the same time, for convenient testing between a cmd line publisher and a bowser subscriber (or vice-versa).

**None of these** are designed for end-users to connect directly to them. Always build an Application Server, either by using ion-sfu as a library, or using the grpc API to talk to a local ion-sfu. (?? Is using internal GRPC "officially supported"?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tarrence + Orlando, can you 2 confirm whether using the grpc API (internally, after validating SDPs in an upstream application server) should currently be considered a "supported" usecase? Can we safely advise people to use the grpc or jsonrpc docker images internally as long as input is sanitized?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move the included signaling to be "examples" and the batteries included deployment should be "ion"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think that represents a major change needed to the above advice, we will have to reword it to avoid saying jsonrpc/grpc are official interfaces people should build upon, I will mark this as a Draft and we can try to adjust the verbiage this week

Copy link
Collaborator

@tarrencev tarrencev Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. The reason i think this is the right direction is there are a million different ways to signal and it is very dependent on your goals. I would prefer to remove it as a source of distraction. Then it will hopefully be more clear that authors should extend their own implementation rather than try to generalize the examples to other use cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took some time to think about it and I understand why it seems like the best option - and the GoLang API is undoubtedly the strongest one to support. It is also the hardest to use, and adds a new requirement that everyone write GoLang to use ion-sfu.

I have to say, I do see this as breaking support for most existing projects built on ion-sfu -- including mine. Granted, by moving the entrypoints to examples/ you are leaving a backup option, but I feel like you are suggesting we deprecate "official" support for 2 APIs that most people have been building on. Tarrence, you yourself told me I should build a python client to talk to SFU over grpc... I know this means lots of re-thinking and re-structuring and re-work for me and many others.

I tried to maintain my own SFU wrapper in golang, but it was a source of constant frustration and bugs, I abandoned the code after wasting 4mo evenings and weekends on the effort, I much preferred to be able to write an API client in a language I am familiar with. Mainstream support for grpc and jsonrpc has been implied (at least since the early versions of the codebase), by both the name of the folder they live in and their repeat appearance in related SDK project examples, and further reinforced by the prevailing discussions in the slack channel. I'm sad nobody spoke up and said "don't build on this grpc entrypoint, it should not be an official entrypoint" much sooner.

I'm always happy to see ion-sfu become a better project, but as a maintainer who devotes most of his resources to helping teach newcomers and share knowledge, I expect I'll be dealing with frustrated and sad users. We'll be best off to add a section to the FAQ "What happened to cmd/signal?" at the very least. I'll prepare the necessary changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you concern and I definitely don't think that we should kill it. We should still continue maintaining it in its current form. My main point is to move away from it being seen as a signaling layer that competes with i.e. janus or jitsi signaling. Our goal is to move almost everything to happen over webrtc (using datachannels) so the out of bound signaling will be very simple. Anything on top will be application specific. I know it is frustrating to have a moving target, but we are all learning together and make mistakes. I think we would change a lot about how we do signaling if we knew then what we know now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe deprecating this is the wrong move. I do not understand why we're reluctant to say its supported because nearly all the web examples are built on it. From my perspective jsonrpc should be supported as a first class project. We are already doing this at tandem, and we're happy to continue supporting it in our codebase. I'm happy to think through and write up a more formal proposal on how to handle this at the org level, but pushing everyone to ion which is way more complex and heavy than jsonrpc is the wrong move in my opinion.

@tarrencev
Copy link
Collaborator

This is super awesome!

Copy link
Contributor

@leewardbound leewardbound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have to reword it to avoid saying jsonrpc/grpc are official interfaces people should build upon, and clarify they are examples that you can begin your own Application Server from.

https://github.com/pion/ion-sfu/pull/496/files#r603736974

@nkonev nkonev mentioned this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants