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

RTP header extensions may conflict on transport receiver #803

Closed
robin-raymond opened this issue Jan 15, 2018 · 7 comments
Closed

RTP header extensions may conflict on transport receiver #803

robin-raymond opened this issue Jan 15, 2018 · 7 comments

Comments

@robin-raymond
Copy link
Contributor

@robin-raymond robin-raymond commented Jan 15, 2018

If there are multiple receivers with different header extensions mapped to the same ID this can cause issues in interpreting of the header extension. For example, RID might be mapped to an ID which conflicts in one sender with a different header extension.

This can cause receiver.receive(...) to fail. This can also cause changing transports on a receiver can fail due to attaching to a transport that already has header extensions assigned to different meanings for other receivers.

receiver.setTransport should be async to allow the receiver to interrogate the transport to ensure no conflicts occur with header extensions.

@ibc
Copy link
Contributor

@ibc ibc commented Jan 15, 2018

Yep. This is like "the egg or the chicken". The RID and MID header extensions are intended for demultiplexing RTP streams but they are set at RTCRtpParameters level within the corresponding RTCRtpSender.

receiver.setTransport should be async to allow the receiver to interrogate the transport to ensure no conflicts occur with header extensions.

How is that different from just throwing an exception? "Interrogating the transport" should be sync IMHO.

@robin-raymond
Copy link
Contributor Author

@robin-raymond robin-raymond commented Jan 15, 2018

Not always possible in all implementations to make this synchronous.

@ibc
Copy link
Contributor

@ibc ibc commented Jan 15, 2018

IMHO it makes sense that this is a synchronous operation. Implementations should honor the spec and not the other way around.

@robin-raymond
Copy link
Contributor Author

@robin-raymond robin-raymond commented Jan 15, 2018

@ibc This isn't so easy from experience in this case. The header extensions need to be registered as a transaction against the transport to ensure they take affect as multiple transports can conflict. This can cause contention if you attempt to do this synchronously. I would have a very difficult time making this synchronous in my own code for ortclib for example. There's a reason mechanisms that need to work across multiple objects become asynchronous vs objects than can act independently without interrogation / interaction / transacting with other active objects.

@ibc
Copy link
Contributor

@ibc ibc commented Jan 15, 2018

This is about iterating the RTCRtpParameters of all the RTCRtpSenders and RTCRtpReceivers managed by the given transport and checking whether MID and RID values do not conflict between them. Not sure why this cannot be synchronous. I've already done this in a synchronous way.

@robin-raymond
Copy link
Contributor Author

@robin-raymond robin-raymond commented Jan 15, 2018

It's not enough to check if they conflict, receivers have to be registered on construction and de-registered when they detach. This can and does cause contention issues across objects. When detachment happens this has to be done asynchronously for my implementation (for a long host of reasons as a whole set of pipelines need to be torn down across objects) and if I have certain things happen synchronously then it might be possible a detachment happens after a new attachment which can cause a false failure. Maybe your implementation doesn't have this issue but I can say for 100% certainty that it would be very difficult for me to handle this case synchronously.

@ibc
Copy link
Contributor

@ibc ibc commented Jan 15, 2018

Fair enough.

aboba added a commit that referenced this issue Mar 1, 2018
Partial fix for Issue #803
@aboba aboba self-assigned this Mar 1, 2018
@aboba aboba added the PR exists label Mar 1, 2018
aboba added a commit that referenced this issue Apr 29, 2018
Fix for Issue #803
@aboba aboba added the PR exists label Apr 29, 2018
aboba added a commit that referenced this issue Jun 6, 2018
Fix for Issue #803
aboba added a commit that referenced this issue Jul 10, 2018
Fix for Issue #803
@aboba aboba closed this Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants