-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
grahamking
wants to merge
2
commits into
master
Choose a base branch
from
faq-patch-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.