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

Add RTCSctpTransport.setTransport #775

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lgrahl
Copy link
Contributor

@lgrahl lgrahl commented Sep 24, 2017

These changes add a setTransport to the SCTP transport to allow seemless changing of the underlying DTLS transport. It also adds a missing state check for the DTLS transport instance on construction.

Some questions are left:

1. DTLS transport state on construction/setTransport

When constructing an instance that requires a DTLS transport, the description is:

If an attempt is made to construct an RTCRtpSender object with transport.state or rtcpTransport.state closed

Shouldn't the failed state be covered by the description, too? (The same would need to be applied to the description of setTransport.)

Answer: No.

2. state vs. ObjectNameState

I've seen that in the RTCSctpTransport to reference the current active state, RTCSctpTransportState is being used instead of just state (to refer the local attribute) as in the RTCQuicTransport.start method. Should we unify this for consistency (and by which method)?

3. DTLS transport component check

Do we need a dtlsTransport.transport.component check on construction of the SCTP transport (and on calling setTransport)? If the component is not rtp, this should be an error, right?

@lgrahl
Copy link
Contributor Author

lgrahl commented Sep 24, 2017

Also fixes a minor compatibility issue raised in #755.

@taylor-b
Copy link

taylor-b commented Sep 25, 2017

If the component is not rtp, this should be an error, right?

Why? If you're not RTCP-muxing, you need separate DTLS associations for RTP and RTCP.

Somehow missed that we're talking about SctpTransport, not RtpSender. Nevermind, you're correct.

@aboba aboba added the 1.1 label Sep 28, 2017
@lgrahl
Copy link
Contributor Author

lgrahl commented Sep 29, 2017

@aboba @robin-raymond Any comments to 3? Do you agree with @taylor-b and me?

@lgrahl
Copy link
Contributor Author

lgrahl commented Apr 13, 2018

Any action needed from my side to get this merged (other than the conflict that appeared in the meantime 🙂)?

@lgrahl
Copy link
Contributor Author

lgrahl commented Oct 6, 2018

Ping.

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

Successfully merging this pull request may close these issues.

3 participants