Re: A proposal for solving non-muxed RTCP *and* ICE freezing

On Sat, May 3, 2014 at 6:30 AM, Robin Raymond <robin@hookflash.com> wrote:

>
>
>
> [RR} Generally, I like this approach. I know there are a half dozen other
> approaches we can do which might be equally as good
>

​This problem is a little more thorny than that, I think, which is why it's
taken so long to arrive at a solution that didn't have some major problem.
 I think this is the first I've seen that doesn't.​



>  but we have to "pick one" that makes sense which has the least amount of
> pain for those who don't care to do non-muxed RTCP or don't care about
> freezing other than "it just works". See inline comments.
>

​I believe this proposal has zero pain for someone using bundle and rtcp
mux (which I hope is the normal/default/preferred/dominant usage).​



>
>    Peter Thatcher <pthatcher@google.com>
>  May 2, 2014 at 9:21 PM
> This could probably be two separate proposals, but they are slightly
> related, so the proposal and example here will go together.
>
> Just a reminder about the problems we're solving:
>
> 1. We don't have a way
> ​ ​
> for the app to
> ​ ​
> express that
> ​ i​
> t
> ​ ​
> wants
> ​ ​
> to have RTCP that isn't multiplexed with RTP.  We can create a separate
> IceTransport for RTCP, but we don't have a way to tie it to tie it to the
> RTP transport.
>
> 2.  We don't have a way of expressing that a set of IceTransports are tied
> together in a
> ​way​
>  such that the right ICE freezing behavior happens, and in particular that
> the JS can express which IceTransport(s) should come first.
>
>
> ​So, here's how we can solve these two things:
>
> // From [RFC 5245] 4.1.1.1.
>
> enum IceComponent {
>
>  RTCP = 2
>
> }
>
> [RR] I presume component hasn't been overloaded to have dual purpose other
> than for RTCP (or something else that wouldn't work within an enum set)?
> Just double checking as I'm always amazed at what crazy things are done at
> times...
>
>
I don't think there is, but if it's an enum, it doesn't really matter.


> Minor nit, probably should be:
>
> enum IceComponent {
>   "RTCP"
> };
>

​You can tell I write more C++ than WebIDL :).​



>
> [/RR]
>
>
>
>  interface IceTransport {
>
>  // ... all the existing stuff
>
>
>   // Create ICE transport for RTCP via createAssociatedTransport(RTCP)
>
>  // and associate it with this ICE transport.
>
>  // When the transport for RTP is supplied to RtpSender/Receiver,
>
>  // an RTCP transport, if present, is implicitly associated as well.
>
>  // If called more than once for the same component,
>
>  // the old one is ".stop()"ed, and the new one replaces it
>
>  // (or do we just want to fail?)
>
>  IceTransport createAssociatedTransport(IceComponent component);
>
> }
>
>
>   [RR] I would rather have API fail rather than auto-stop.
>

​I'm happy with just failing.​



>
>  dictionary RtpParameters {
>
>  // ... all the existing stuff
>
>
>   // Defaults to true.  If false in RtpSender.send() or
> RtpReceiver.receive(),
>
>  // the IceTransport must have an associated IceTransport with component
> ==
>
>  // RTCP, in which case RTCP will be sent on the associated IceTransport
> ​.
>
>  bool rtcpMux;
>
> }
>
> [RR] When you said "...the IceTransport must have an associated
> IceTransport with component...", What will happen if they set to false and
> they don't have this component? Silent fail? Fail upon "start" using it?
> What happens if the RTCP related IceTransport was stopped after "start" was
> called but this flag is was set "false" when "start" as called?
>
>
>  ​Fail with invalid parameters, just like any other invalid parameters.

In the case that there is an RTCP transport, but the app called stop, treat
it just as if they called stop on the transport mid-call: RTCP stops
flowing until a new IceTransport is created (or the ICE transport is
restarted).

>  interface TransportController {
>
>  sequence<IceTransport> getTransports();
>
>
>   // Adds a transport to the controller for the purposes of managing
>
>  // ICE freezing and sharing BWE. ICE transports will be unfrozen
>
>  // according to their index.
>
>  // Inserts at |index|, or the end if no index specified.
>
>  // Fails if |index| is greater than the current number of transports.
>
>  // Fails if |transport| is already added to another TransportController.
>
>  // Fails if |transport| is for the RTCP component.
>
>  void addTransport(IceTransport transport, int index = null);
>
> [RR] Alt void "addTransport(IceTransport transport, IceTransport
> insertBeforeTransport = null);". Can be done by relative position to an
> existing object rather than via absolute index. I slightly prefer that than
> an absolute index but I don't have strong preference.
>

​This is an absolute index.​  That's also what I prefer.



> Also, I think there needs to be a rule to fail if adding IceTransports
> that were created from "IceTransport.createAssociatedTransport(...)".
>

​That's what is meant by "Fails if |transport| is for the RTCP component."

>    // Do we need this? Assume not for now.
>
>  // Calling transport.stop() should be sufficient.
>
>  // void removeTransport(IceTransport transport);
>
> }
>
>
>   [RR] I see no reason to need but perhaps others do.
>
>
> ​Here's an example:​
>
>
>
> var audioRtpIceTransport = new RTCIceTransport(...);
>
> var audioRtcpIceTransport = audioRtpIceTransport.createAssociatedTransport(
>
>  IceComponent.RTCP);
>
> var videoRtpIceTransport = new RTCIceTransport(...);
>
> var videoRtcpIceTransport = audioRtpIceTransport.createAssociatedTransport(
>
>  IceComponent.RTCP);
>
> // ... start the ICE and DTLS transports
>
>
>  var controller = new TransportController();
>
> controller.addTransport(audioRtpIceTransport);
>
> controller.addTransport(videoRtpIceTransport);
>
>
>  audioSender = new RtpSender(...);
>
> videoSender = new RtpSender(...);
>
> audioReceiver = new RtpSender(...);
>
> videoReceiver = new RtpSender(...);
>
> var audioParams = RtpSender.createParams();
>
> var videoParams = RtpSender.createParams();
>
> // Sender can disable RTCP mux to start,
>
> // to see if responder will enable it.
>
> audioParams.rtcpMux = false;
>
> videoParams.rtcpMux = false;
>
> // At this point, the sender can send the params to the receiver.
>
> // If the receiver leaves it disable, or enables it, either way it works.
>
> audioSender.send(audioParams);
>
> videoSender.send(videoParams);
>
> audioReceiver.receive(audioParams);
>
> videoReceiver.receive(videoParams);
>
> // If the receiver does enable RTCP
> ​-​
> mux, we need to remember to
>
> // stop the unused ICE transports, or else they will go to the
>
> // failed state when the remote endpoint blows theirs away.
>
> if (audioParams.rtcpMux) {
>
>  audioRtcpIceTransport.stop();
>
> }
>
> if (videoParams.rtcpMux) {
>
>  videoRtcpIceTransport.stop();
>
> }
>
>
>
>  ​This has the following benefits:
>
>  ​1. It allows non-muxed RTCP
>
> 2. It allows proper ICE freezing behavior.
>
> 3. The additional complexity added to the API is small.
>
>  4. The burden on users who don't care about non-muxed RTCP or ICE
> freezing order is basically zero.
>
> 5. The users who do care about non-muxed RTCP and ICE freezing have a
> pretty clean API to work with, and the only tricky part is remembering to
> stop transports you don't use anymore.
>
>
> ​
>
> [RR] Looks good to me. Question: should ice freezing should still work
> between Alice and Bob even when they haven't setup an "IceController", e.g.
> an implicit freezing rather than explicit? I can see benefits and drawbacks
> to both answers.
>

​Yes, there  should be freezing if there is no "TransportController".  But
you don't get to control the order.​