-
Notifications
You must be signed in to change notification settings - Fork 115
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
No way to observe DataChannel-only transport events in initial negotiation #2899
Comments
This change would require either making maxMessageSize nullable or just initializing it to 0. |
The suggestion would be a big downgrade to me, let me explain: I consider it good API design that a) know that SCTP has been negotiated, and If we were to switch this around and make the With the benefits of the status quo logic in mind, if we have some other property where the information is relevant prior to negotiation, then it does not belong on Edit: We also discussed a while ago that SCTP is different to the other transports. It does not allow for any kind of renegotiation by use of SDP, so this design remains plausible for the lifetime of the peer connection. (See #2839 and some other discussion I couldn't find where we discussed what should happen in case |
I don't see a "bunch", only interface RTCSctpTransport : EventTarget {
readonly attribute RTCDtlsTransport transport;
readonly attribute RTCSctpTransportState state;
readonly attribute unrestricted double maxMessageSize;
readonly attribute unsigned short? maxChannels;
attribute EventHandler onstatechange;
}; maxChannels is already nullable, and "will be null until the SCTP transport goes into the
No,
Since this is already true for
I believe SCTP can be added in renegotiation, just not removed? In any case, what difference does that make toward solving the OP problem? |
Sure, the design does not align well with my expectations in the case of If I were to spec this API freely, I would have preferred to assign each state its own set of properties and make it more friendly to static analysis as a side effect, e.g. something like the following (TS syntax): type SctpTransportState =
| {
name: 'staged';
}
| {
name: 'negotiated';
maxMessageSize: number;
}
| {
name: 'connected';
maxMessageSize: number;
maxChannels: number;
};
interface SctpTransport {
state: SctpTransportState;
onstatechange: ...;
} Therefore, I guess my view of the design does not align with your/the spec authors view of the design and the result just aligned by accident to some degree. |
Thanks for the discriminated union example. That's something to consider in future APIs for sure. Any change this late probably has to be surgical to minimize breakage. Every design comes with tradeoffs. E.g. if we were to add a type RTCSctpTransport =
| null | {
state: "new";
transport: object;
onstatechange: object;
} | {
state: "connecting";
transport: object;
maxMessageSize: number;
onstatechange: object;
} | {
state: "connected";
transport: object;
maxMessageSize: number;
maxChannels: number;
onstatechange: object;
} | {
state: "closed";
transport: object;
maxMessageSize: number; // present or not?
maxChannels: number; // present or not?
onstatechange: object;
};
interface RTCPeerConnection {
sctp: RTCSctpTransport;
} Whether maxMessageSize, maxChannels, or both are present in |
I wouldn't mind having a way to get the transport that worked for all cases independent of whether there was SCTP or not; a bunch of tests are doing sctp.transport or senders[0].transport at random. The sticker here is really what to return for non-bundled connections. |
In the TPAC meeting Henrik mentioned a variant of proposal B - adding a member to the pc.oniceconnectionstatechange data that is the iceTransport that it relates to - this seems cleaner and simpler than adding an (array of) iceTransports properties to the Peer connection. I don't like surfacing a (potentially ghost) sctpTransport who's values are wrong or non-existent just to get an iceTransport. |
Agree this might've worked if "max-bundle" was all we needed to support. But the current API surface shape also supports "balanced" and "max-compat".
This has the same problem Harald raises: what to return for non-bundled connections? I also thought we introduced the ORTC-like transport objects to get away from relying on the aggregated pc states, so building on the aggregated state feels like the wrong direction.
Aren't transceivers also ghosts by this definition? I don't think its values are more wrong than those of DtlsTransport: they reflect what the local offer is about to negotiate. |
I was imagining a getter that returns all the transports currently active for the PeerConnection. When using max-bundle, there would be only one; when there are more than one, apps that care will have to find some way to sort out which is which. (Comparing the transport objects to those on the transceivers should do it). I don't know how you define "ghost" when you say that "transceivers are ghosts" - the scenario for the sctpTransport is that when the responder rejects the "m=application" media section in the remote answer, no sctpTransport underlying object will ever have been created. So the sctpTransport object that would be visible after createOffer will never correspond to a real sctpTransport - unlike rejected transceivers, which "just" transition to state "stopped" (if I remember rightly). That said, as I said in the meeting, I won't stand in the way of resolving this issue by mandating the creation of the ghost. |
Not to branch out too many new topics, but how would an app do that (in the API?)
This would match how the other transports work.
Rejected transceivers eventually disappear from getTransceivers() — we just kludged it to require an offer from the remote side first to avoid apps accidentally nuking the offerer-tagged BUNDLE transport.
Same as with the other transports. They stop being referenced by the pc, so the app can no longer find them. But of course JS can hold any object alive, so its states are still observable and need to make sense. It's state would be closed I imagine. |
by not being a WebRTC implementation. We in fact had exactly this issue interoperating with another company's server (I suspect it was a SIP server).
No - the ICETransport underlying object is created on SLD, I think the DTLSTransport object is too. Then they are destroyed again when they turn out not to be needed. But that is really an implementation detail - we could have created ghosts.
But if you hang on to a reference to them, they don't disappear at all....
That makes sense. |
@steely-glint introduced the term "ghost", which I interpreted to mean an object that, if rejected or BUNDLED away by the other side, only exists during negotiation (or in the case of transceivers, until a negotiation initiated by the other side). The proposal here is to align the sctpTransport's lifetime with that of its iceTransport, which acts as @alvestrand describes, which I find quite ghostly, but regardless. I wrote this fiddle to reject m=application and found quite inconsistent behavior in browsers (bugs maybe?) But all of the browsers seem to fire ice candidates on initial negotiation only, so we might only need to expose this Datachannel rejection seems like an edge-case. But how are apps to learn of it? Might early exposure help here? E.g. pc.createDataChannel("");
await pc.setLocalDescription();
pc.sctp.onstatechange = () => pc.sctp.state == "closed" && console.log("m=application rejected!");
// continue negotiation I find support for this in the description of the "closed" state: "the SCTP association has been closed intentionally, such as by closing the peer connection or applying a remote description that rejects data or changes the SCTP port." |
As mentioned in #2898, whereas
transceiver.transport
s are surfaced in sLD,pc.sctp
isn't surfaced until the answer.Inconsistency aside, this renders the following event handlers kinda useless on initial negotiation:
pc.sctp.onstatechange
pc.sctp.transport.onstatechange
pc.sctp.transport.iceTransport.ongatheringstatechange
pc.sctp.transport.iceTransport.onselectedcandidatepairchange
...because it's not possible to register handlers early enough to catch events reliably. It also means they work differently on initial vs. subsequent negotiations, which isn't great.
There's no other way to get dtls or ice-transport-object events for DataChannel-only connections.
Workarounds
pc.oniceconnectionstatechange
andpc.onicegatheringstatechange
instead, or"max-bundle"
and add a dummy audio transceiver to surface the DTLS and ICE transport earlierThis seems broken. We should probably surface the sctp transport in sLD (and handle it in rollback) like the others.
The text was updated successfully, but these errors were encountered: