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

Comments

Projects
None yet
4 participants
@murillo128

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 from Value of onssrcconflict event in RTCRtpSender to Change onssrcconflict event in RTCRtpSender by exception Apr 1, 2016

@murillo128

This comment has been minimized.

Show comment
Hide comment
@murillo128

murillo128 Apr 1, 2016

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.

murillo128 commented Apr 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 1, 2016

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...

Contributor

robin-raymond commented Apr 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Apr 1, 2016

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 1, 2016

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.

Contributor

robin-raymond commented Apr 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Apr 1, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 1, 2016

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.

Contributor

robin-raymond commented Apr 1, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Apr 1, 2016

Contributor

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

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

@aboba aboba added PR exists and removed publication-blocker labels Apr 1, 2016

@murillo128

This comment has been minimized.

Show comment
Hide comment
@murillo128

murillo128 Apr 1, 2016

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

murillo128 commented Apr 1, 2016

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

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Apr 6, 2016

Contributor

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.

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 Apr 6, 2016

aboba added a commit that referenced this issue Apr 6, 2016

@aboba aboba reopened this Apr 6, 2016

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Apr 6, 2016

Contributor

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

Contributor

aboba commented Apr 6, 2016

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

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 7, 2016

Contributor

Resolved and clarified in PR #459

Contributor

robin-raymond commented Apr 7, 2016

Resolved and clarified in PR #459

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