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

Change onssrcconflict event in RTCRtpSender by exception #448

Closed
murillo128 opened this issue Apr 1, 2016 · 11 comments
Closed

Change onssrcconflict event in RTCRtpSender by exception #448

murillo128 opened this issue Apr 1, 2016 · 11 comments

Comments

@murillo128
Copy link

murillo128 commented Apr 1, 2016

Currently we have an onssrcconflict in the RTCRtpSender, but I fail to see which situations would cause it to be triggered.

AFAIK the only way of having an ssrc conflict is by assigning the same ssrc to two different RTPSender on same transport, or having two different encodings with same ssrc. Or when changing a sender from one transport to another which already had another RTPSender using that same ssrc.

This can only be triggered by an API method call (either RTCRtpSender.send()or RTCRtpSender.setTransport()) so IMHO it is much better to throw an exception in this case (an InvalidParameter exception for send() and maybe a new SSRConflict exception for setTranspor()), that triggering an event.

@murillo128 murillo128 changed the title Value of onssrcconflict event in RTCRtpSender Change onssrcconflict event in RTCRtpSender by exception Apr 1, 2016
@murillo128
Copy link
Author

This event must be fired if an SSRC conflict is detected. On an SSRC conflict, the RTCRtpSender automatically sends an RTCP BYE on the conflicted SSRC.

More over, if we move to exceptions, this would cause the method call to fail, so we can rollback changes and leave the RTPSender on same state as before the method call, avoiding to call BYE and terminate the stream.

@robin-raymond
Copy link
Contributor

I don't know any implementation (yet) that detects and throws the exception. We do need to be clear when this can happen. I don't want to have specialized exceptions needlessly or different exceptions for the same event needlessly.

This is a very problematic thing for the sender because of multiple reasons:

  1. At time of send(..) it may be possible to detect a conflict of ssrc exists with other senders if both senders are attached to the same transport (and thus fire an invalid params BUT)
  2. Firing an invalid exception on send based on ssrc conflict might require asynchronous processing to determine since knowing this must reach deep into sender transport objects
  3. If a programmer changes the sender's transport to attach to a transport that already has the ssrc then the conflict occurs at that time of setTransport(...) and not at time of calling send(...)

So... we do have some clarifying to do on how / when this is supposed to occur...

@ibc
Copy link
Contributor

ibc commented Apr 1, 2016

If we are in the sender, SSRC conflicts can just happen due to user actions. If they cannot be detected synchronously then having send() return a Promise would be a much better approach than firing a context-less event.

Anyhow:
ssrc-conflict

@robin-raymond
Copy link
Contributor

LOL

then having send() return a Promise would be a much better approach than firing a context-less event.

Except a Promise is not sufficient... This can also happen later after send(...) is called by attaching a sender to a transport after send is called that already has another sender with the same ssrc.

@ibc
Copy link
Contributor

ibc commented Apr 1, 2016

This can also happen later after send(...) is called by attaching a sender to a transport after send is called that already has another sender with the same ssrc.

rtpSender.setTransport() may also return a Promise.

@robin-raymond
Copy link
Contributor

@ibc I would prefer to just event ssrc conflicts; it's such a rare use case and I seriously doubt many are going to implement or concern themselves with it.

@ibc
Copy link
Contributor

ibc commented Apr 1, 2016

If somebody gets a sending SSRC conflict he deserves ugly things to happen.
Yep, there are is more important stuff to improve.

aboba added a commit that referenced this issue Apr 1, 2016
@murillo128
Copy link
Author

Indeed as @aboba adds in the PR, according to the rtp specification, ssrc conflict may happenbetween the local ssrcs and the remote ones.

@aboba
Copy link
Contributor

aboba commented Apr 6, 2016

The next text states that for send() and receive(), "SSRC misusage results in an InvalidParameters exception." The onssrcconflict event handler "is fired if an SSRC conflict is detected within the RTP session." As Robin noted, there are no existing implementations of the onssrcconflict event handler.

@aboba aboba closed this as completed Apr 6, 2016
aboba added a commit that referenced this issue Apr 6, 2016
@aboba aboba reopened this Apr 6, 2016
@aboba
Copy link
Contributor

aboba commented Apr 6, 2016

Went through the specification and found additional areas to clarify. There may be additional clarifications needed.

@robin-raymond
Copy link
Contributor

Resolved and clarified in PR #459

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants