RE: Issue 198: Philipp Hancke's Review Comments (part II)

Since Philipp's review was so thorough, I had to go over the comments several times.  Here are some additional (and in some cases, revised) responses:


6) page 5, section 2.3.1

This event MUST be fired on reception of a DTLS alert.

does it expose the level and description of the alert? (e.g. 'fatal' + handshake failure)

[BA] It is possible for the implementation to put information on the DTLS alert into the error message text.  Not sure we need to mandate this, though.

7) page 6, section 2.3.2

Stops and closes the DTLS transport object. Since stop() is final, if
start() is called afterwards, throw an InvalidStateError exception.

what happens when calling getStats after this? There was some discussion of that case for the PC

[BA] Here is some revised text for Section 13.1, dealing with objects in the "closed" state:
getStats
Gathers stats for the given object and reports the result asynchronously.
When the getStats() method is invoked, the user agent must queue a task to run the following steps:
1.    Let p be a new promise.
2.    For RTCDtlsTransport.getStats(), check whether RTCDtlsTransport.state is "closed"; if so, reject p with an InvalidStateError. ForRTCIceGatherer.getStats(), check whether RTCIceGatherer.state is "closed"; if so, reject p with an InvalidStateError. For RTCIceTransport.getStats(), check whether RTCIceTransport.state is "closed"; if so, reject p with an InvalidStateError.
3.    Return, but continue the following steps in the background.
4.    Start gathering the stats.
5.    When the relevant stats have been gathered, return a new RTCStatsReport<http://internaut.com:8080/~baboba/ortc/ortc-5-21-2015.html#idl-def-RTCStatsReport> object, representing the gathered stats.

8) page 7, section 2.6

sequence fingerprints;

SDP can only deal with a single fingerprint (surprise...)

[BA] Opened Issues 210 (https://github.com/openpeer/ortc/issues/210) and 211 (https://github.com/openpeer/ortc/issues/211) to deal with this and associated problems.

9) page 7, section 2.6.1

role of type RTCDtlsRole, defaulting to "auto"

move up to match order of attributes in spec.
[BA] Done.  In general, the problem is created because respec.js compulsively alphabetizes everything.  So far, we haven't figured out how to turn this off.


11) either do "ufrag and pwd" or "usernameFragment and password" for consistency

[BA] It's probably best to use the same terminology as in RFC 5245 (usernameFragment and password).  Changed text to reflect this.


12) page 10, section 3.5.1

3.5.1 Dictionary RTCIceParameters Members

order of attributes doesn't match description here

[BA] For this one, it seems like alpha-betizing the order would impact understanding.  It would be better to turn off alpha-betizing in respec.js but we haven't figured out how yet.

13) page 10, 3.7

enum RTCIceTransportState

this is missing my favorite "failed" state from RTCPeerConnection

[BA] Opened Issue 199 (https://github.com/openpeer/ortc/issues/199) to deal with this.   We now have a proposed definition of "failed" as well as a proposed state diagram (non-normative) with "failed" in it.



22) page 14, section 3.14
[first comment in that section is invalid]

gatherOptions.iceservers = ... ;

Just make it []

23) page 14, section 3.14

get tracks and RTP objects from other example

cross-ref would be helpful

24) page 15, section 3.14

}, function(remote) {

this example seems to make some assumptions about the signaling protocol. For Jingle, the callback might be triggered when receiving an which does not imply the session has been accepted, just that the offer has been received.

25) page 15, section 3.14

gatherOptions.iceservers = ... ;

[] is shorter

[BA] There are indeed a lot of issues with the example in Section 3.14.  Here is a revision, trying to fix them:

// Example to demonstrate forking when RTP and RTCP are not multiplexed,
// so that both RTP and RTCP IceGatherer and IceTransport objects are needed.
// Create ICE gather options
var gatherOptions = new RTCIceGatherOptions();
gatherOptions.gatherPolicy = RTCIceGatherPolicy.relay;
gatherOptions.iceservers = [ { urls: "stun:stun1.example.net" } , { urls:"turn:turn.example.org", username: "user", credential:"myPassword"} ];
// Create ICE gatherer objects
var iceRtpGatherer = new RTCIceGatherer(gatherOptions);
var iceRtcpGatherer = iceRtpGatherer.createAssociatedGatherer();
// Prepare to signal local candidates
iceRtpGatherer.onlocalcandidate = function (event) {mySendLocalCandidate(RTCIceComponent.RTP, event.candidate)};
iceRtcpGatherer.onlocalcandidate = function (event) {mySendLocalCandidate(RTCIceComponent.RTCP, event.candidate)};
// Set up response function
mySignaller.onResponse = function(responseSignaller,response) {
// We may get N responses
//
// Create the ICE RTP and RTCP transports
var iceRtpTransport = new RTCIceTransport(iceRtpGatherer);
var iceRtcpTransport = iceRtpTransport.createAssociatedTransport();
// Start the RTP and RTCP ICE transports so that outgoing ICE connectivity checks can begin
iceRtpTransport.start(iceRtpGatherer, response.icertp, RTCIceRole.controlling);
iceRtcpTransport.start(iceRtcpGatherer, response.icertcp, RTCIceRole.controlling);
 // Prepare to add ICE candidates signalled by the remote peer
responseSignaller.onRemoteCandidate = function(remote) {
  if (remote.component === RTCIceComponent.RTP){
   iceRtpTransport.addRemoteCandidate(remote.candidate);
   } else {
   iceRtcpTransport.addRemoteCandidate(remote.candidate);
  }
};

mySignaller.send({
   "icertp": iceRtpGatherer.getLocalParameters(),
   "icertcp": iceRtcpGatherer.getLocalParameters()
});
// ... setup DTLS, RTP, SCTP, etc.

function mySendLocalCandidate(component, candidate){
   mySignaller.mySendLocalCandidate({
   "candidate": candidate,
   "component": component
   });
}


27) page 16, section 4.4
[first comment in that section is invalid]

As well as

s/As/as

28) page 16, section 4.4

if (answer.bundle) {

I have a note here about rewriting these conditions to make them clearer, but can't remember how I wanted them to be more clear.

29) page 16 + 17, section 4.4 (not in PDF)

};

unnecessary semicolon (jshint nitpick mode)

// Check if the responder does not want BUNDLE
// and does not want RTP/RTCP multiplexing
if (!answer.rtcpMux) {

The indent seems wrong here which makes this confusing. Probably what I had in mind with comment 28.

videoRecvParams.rtcp.mux = false;
};

unnecessary semicolon. There seems to be a closing '}' missing, indent makes it hard to read.

30) page 18, section 4.4

log('Error encountered: ' + error.name);
that is the only place where the log helper is used afaics

[BA] Here is a revised example that hopefully fixes several of the issues (though it still doesn't handle forking or responses to incoming connectivity checks properly):


// This is an example of how to utilize distinct ICE transports for Audio and Video

// as well as for RTP and RTCP.  If both sides can multiplex audio/video

// and RTP/RTCP then the multiplexing will occur.

//

// Assume we have an audioTrack and a videoTrack to send.

//

// Create the ICE gather options

var gatherOptions = new RTCIceGatherOptions;

gatherOptions.gatherPolicy = RTCIceGatherPolicy.all;

gatherOptions.iceservers = [ { urls: "stun:stun1.example.net" } , { urls:"turn:turn.example.org", username: "user", credential:"myPassword"} ];



// Create the RTP and RTCP ICE gatherers for audio and video

var audioRtpIceGatherer = new RTCIceGatherer(gatherOptions);

var audioRtcpIceGatherer = audioRtpIceGatherer.createAssociatedGatherer();

var videoRtpIceGatherer = new RTCIceGatherer(gatherOptions);

var videoRtcpIceGatherer = videoRtpIceGatherer.createAssociatedGatherer();



// Set up the ICE gatherer error handlers

audioRtpIceGatherer.onerror = errorHandler;

audioRtcpIceGatherer.onerror = errorHandler;

videoRtpIceGatherer.onerror = errorHandler;

videoRtcpIceGatherer.onerror = errorHandler;



// Create the RTP and RTCP ICE transports for audio and video

var audioRtpIceTransport = new RTCIceTransport(audioRtpIceGatherer);

var audioRtcpIceTransport = audioRtpIceTransport.createAssociatedTransport();

var videoRtpIceTransport = new RTCIceTransport(videoRtpIceGatherer);

var videoRtcpIceTransport = audioRtpIceTransport.createAssociatedTransport();



// Enable local ICE candidates to be signaled to the remote peer.

audioRtpIceGatherer.onlocalcandidate = function (event)  {mySendLocalCandidate(audioRtpIceTransport,event.candidate,"audio")};

audioRtcpIceGatherer.onlocalcandidate = function (event) {mySendLocalCandidate(audioRtcpIceTransport,event.candidate,"audio")};

videoRtpIceGatherer.onlocalcandidate = function (event)  {mySendLocalCandidate(videoRtpIceTransport,event.candidate,"video")};

videoRtcpIceGatherer.onlocalcandidate = function (event) {mySendLocalCandidate(videoRtcpIceTransport,event.candidate,"video")};



// Set up the ICE state change event handlers

audioRtpIceTransport.onicestatechange = ... ;

audioRtcpIceTransport.onicestatechange = ... ;

videoRtpIceTransport.onicestatechange = ... ;

videoRtcpIceTransport.onicestatechange = ... ;



// Prepare to add ICE candidates signaled by the remote peer on any of the ICE transports

mySignaller.onRemoteCandidate = function(remote) {

  switch (remote.kind) {

     case "audio":

       if (remote.component === RTCIceComponent.RTP){

          audioRtpIceTransport.addRemoteCandidate(remote.candidate);

       } else {

          audioRtcpIceTransport.addRemoteCandidate(remote.candidate);

       }

       break;

     case "video":

       if (remote.component === RTCIceComponent.RTP){

          videoRtpIceTransport.addRemoteCandidate(remote.candidate);

       } else {

          videoRtcpIceTransport.addRemoteCandidate(remote.candidate);

       }

       break;

     default:

       log('Invalid media type received');

  };

}



// Create the DTLS transports

var audioRtpDtlsTransport = new RTCDtlsTransport(audioRtpIceTransport);

var audioRtcpDtlsTransport = new RTCDtlsTransport(audioRtcpIceTransport);

var videoRtpDtlsTransport = new RTCDtlsTransport(videoRtpIceTransport);

var videoRtcpDtlsTransport = new RTCDtlsTransport(videoRtcpIceTransport);



// Create the sender and receiver objects

var audioSender = new RtpSender(audioTrack, audioRtpDtlsTransport, audioRtcpDtlsTransport);

var videoSender = new RtpSender(videoTrack, videoRtpDtlsTransport, videoRtcpDtlsTransport);

var audioReceiver = new RtpReceiver(audioRtpDtlsTransport, audioRtcpDtlsTransport);

var videoReceiver = new RtpReceiver(videoRtpDtlsTransport, videoRtcpDtlsTransport);



// Retrieve the receiver and sender capabilities

var recvAudioCaps = RTCRtpReceiver.getCapabilities("audio");

var recvVideoCaps = RTCRtpReceiver.getCapabilities("video");

var sendAudioCaps = RTCRtpSender.getCapabilities("audio");

var sendVideoCaps = RTCRtpSender.getCapabilities("video");



// Exchange ICE/DTLS parameters and Send/Receive capabilities



mySignaller.myOfferTracks({

   // Indicate that the initiator would prefer to multiplex both A/V and RTP/RTCP

   "bundle": true,

   // Indicate that the initiator is willing to multiplex RTP/RTCP without A/V mux

   "rtcpMux": true,

   // Offer the ICE parameters

   "audioRtpIce": audioRtpIceGatherer.getLocalParameters(),

   "audioRtcpIce": audioRtcpIceGatherer.getLocalParameters(),

   "videoRtpIce": videoRtpIceGatherer.getLocalParameters(),

   "videoRtcpIce": videoRtcpIceGatherer.getLocalParameters(),

   // Offer the DTLS parameters

   "audioRtpDtls": audioRtpDtlsTransport.getLocalParameters(),

   "audioRtcpDtls": audioRtcpDtlsTransport.getLocalParameters(),

   "videoRtpDtls": videoRtpDtlsTransport.getLocalParameters(),

   "videoRtcpDtls": videoRtcpDtlsTransport.getLocalParameters(),

   // Offer the receiver and sender audio and video capabilities.

   "recvAudioCaps": recvAudioCaps,

   "recvVideoCaps": recvVideoCaps,

   "sendAudioCaps": sendAudioCaps,

   "sendVideoCaps": sendVideoCaps

  }, function(answer) {

   // The responder answers with its preferences, parameters and capabilities

   //

   // Derive the send and receive parameters, assuming that RTP/RTCP mux will be enabled.

   var audioSendParams = myCapsToSendParams(sendAudioCaps, answer.recvAudioCaps);

   var videoSendParams = myCapsToSendParams(sendVideoCaps, answer.recvVideoCaps);

   var audioRecvParams = myCapsToRecvParams(recvAudioCaps, answer.sendAudioCaps);

   var videoRecvParams = myCapsToRecvParams(recvVideoCaps, answer.sendVideoCaps);

   //

   // If the responder wishes to enable bundle, we will enable it

     if (answer.bundle) {

        // Since bundle implies RTP/RTCP multiplexing, we only need a single

        // ICE transport and DTLS transport.  No need for the ICE transport controller.

        audioRtpIceTransport.start(audioRtpIceGatherer,answer.audioRtpIce, RTCIceRole.controlling);

        audioRtpDtlsTransport.start(remote.audioRtpDtls);

        //

        // Replace the transport on the Sender and Receiver objects

        //

        audioSender.setTransport(audioRtpDtlsTransport);

        videoSender.setTransport(audioRtpDtlsTransport);

        audioReceiver.setTransport(audioRtpDtlsTransport);

        videoReceiver.setTransport(audioRtpDtlsTransport);

        // If BUNDLE was requested, then also assume RTP/RTCP mux

        answer.rtcpMux = true;

      } else {

        if (answer.rtcpMux){

          // The peer doesn't want BUNDLE, but it does want to multiplex RTP/RTCP

          // Now we need audio and video ICE transports

          // as well as an ICE Transport Controller object

          var controller = new RTCIceTransportController();

          controller.addTransport(audioRtpIceTransport);

          controller.addTransport(videoRtpIceTransport);

          // Start the audio and video ICE transports

          audioRtpIceTransport.start(audioRtpIceGatherer,answer.audioRtpIce, RTCIceRole.controlling);

          videoRtpIceTransport.start(videoRtpIceGatherer,answer.videoRtpIce, RTCIceRole.controlling);

          // Start the audio and video DTLS transports

          audioRtpDtlsTransport.onerror = errorHandler;

          audioRtpDtlsTransport.start(answer.audioRtpDtls);

          videoRtpDtlsTransport.onerror = errorHandler;

          videoRtpDtlsTransport.start(answer.videoRtpDtls);

          // Replace the transport on the Sender and Receiver objects

          //

          audioSender.setTransport(audioRtpDtlsTransport);

          videoSender.setTransport(videoRtpDtlsTransport);

          audioReceiver.setTransport(audioRtpDtlsTransport);

          videoReceiver.setTransport(videoRtpDtlsTransport);

         } else {

          // We arrive here if the responder does not want BUNDLE

          // or RTP/RTCP multiplexing

          //

          // Now we need all the audio and video RTP and RTCP ICE transports

          // as well as an ICE Transport Controller object

          var controller = new RTCIceTransportController();

          controller.addTransport(audioRtpIceTransport);

          controller.addTransport(videoRtpIceTransport);

          // Start the ICE transports

          audioRtpIceTransport.start(audioRtpIceGatherer,answer.audioRtpIce,RTCIceRole.controlling);

          audioRtcpIceTransport.start(audioRtcpIceGatherer,answer.audioRtcpIce,RTCIceRole.controlling);

          videoRtpIceTransport.start(videoRtpIceGatherer,answer.videoRtpIce,RTCIceRole.controlling);

          videoRtcpIceTransport.start(videoRtcpIceGatherer,answer.videoRtcpIce,RTCIceRole.controlling);

          // Start the DTLS transports that are needed

          audioRtpDtlsTransport.start(answer.audioRtpDtls);

          audioRtcpDtlsTransport.start(answer.audioRtcpDtls);

          videoRtpDtlsTransport.start(answer.videoRtpDtls);

          videoRtcpDtlsTransport.start(answer.videoRtcpDtls);

          // Disable RTP/RTCP multiplexing

          audioSendParams.rtcp.mux = false;

          videoSendParams.rtcp.mux = false;

          audioRecvParams.rtcp.mux = false;

          videoRecvParams.rtcp.mux = false;

         }

       }

   // Set the audio and video send and receive parameters.

   audioSender.send(audioSendParams);

   videoSender.send(videoSendParams);

   audioReceiver.receive(audioRecvParams);

   videoReceiver.receive(videoRecvParams);

  });



// Now we can render/play audioReceiver.track and videoReceiver.track



// Helper functions

function log(text) {

  console.log('Time: ' + (performance.now() / 1000).toFixed(3) + ' ' + text);

}



function errorHandler (error) {

    log('Error encountered: ' + error.name);

}



function mySendLocalCandidate(transport, candidate, kind){

   mySignaller.mySendLocalCandidate({

   "candidate": candidate,

   "kind": kind,

   "component": transport.component

   });

}




31) page 18, section 5.1

an incoming connectivity check utilizes the remote ufrag

doesn't it concat local and remote separated by a colon? I always forget in which order...

[BA] Yes. Here is an update to Section 2.1, which hopefully is more clear:

2.1 Overview

An RTCIceGatherer instance can be associated to multiple RTCIceTransport objects. The RTCIceGatherer does not prune local candidates until at least oneRTCIceTransport object has become associated and all associated RTCIceTransport<http://internaut.com:8080/~baboba/ortc/ortc-5-21-2015.html#idl-def-RTCIceTransport> objects are in the "completed" or "failed" state.

As noted in [RFC5245] Section 7.1.2.2, an incoming connectivity check contains an ICE-CONTROLLING or ICE-CONTROLLED attribute, depending on the role of the ICE agent initiating the check. Since an RTCIceGatherer object does not have a role, it cannot determine whether to respond to an incoming connectivity check with a 487 (Role Conflict) error; however, it can validate that an incoming connectivity check utilizes the correct local username fragment and password, and if not, can respond with an 401 (Unauthorized) error, as described in [RFC5389] Section 10.1.2.

For incoming connectivity checks that pass validation, the RTCIceGatherer must buffer the incoming connectivity checks so as to be able to provide them to associatedRTCIceTransport objects so that they can respond and initiate their own connectivity checks once iceTransport.start() is called.

32) page 20, section 5.8

can be used to reduce leakage of IP addresses in certain use cases.

add a note about setting rel-addr to 0.0.0.0 then

[BA] I think this would occur when the browser is set explicitly to reduce leakage - but otherwise not sure how 1.0 backward compatibility could be maintained.

33) page 21, section 5.9

Example to demonstrate forking when RTP and RTCP are not multiplexed.

I don't see that

34) page 21, section 5.9

iceRtpGatherer.onlocalcandidate = function (event)
{mySendLocalCandidate(RTCIceComponent.RTP, event.candidate)};
iceRtcpGatherer.onlocalcandidate = function (event)
{mySendLocalCandidate(RTCIceComponent.RTCP, event.candidate)};

move below the definition of mySendLocalCandidate

35) page 21, section 5.9

function(response) {
// We may get N responses

this seems very odd semantics for a callback...

[BA] Here is a corrected example, which may be better:
EXAMPLE 7

// Assume we already have a way to signal, a transport
// (RTCDtlsTransport), and audio and video tracks. This is an example
// of  how to offer them  and get back an answer with audio and
// video tracks, and begin sending and receiving them.
// The example assumes that RTP and RTCP are multiplexed.
function myInitiate(mySignaller, transport, audioTrack, videoTrack) {
  var audioSender = new RTCRtpSender(audioTrack, transport);
  var videoSender = new RTCRtpSender(videoTrack, transport);
  var audioReceiver = new RTCRtpReceiver(transport);
  var videoReceiver = new RTCRtpReceiver(transport);

// Retrieve the audio and video receiver capabilities
  var recvAudioCaps = RTCRtpReceiver.getCapabilities("audio");
  var recvVideoCaps = RTCRtpReceiver.getCapabilities("video");
// Retrieve the audio and video sender capabilities
  var sendAudioCaps = RTCRtpSender.getCapabilities("audio");
  var sendVideoCaps = RTCRtpSender.getCapabilities("video");

  mySignaller.myOfferTracks({
    // The initiator offers its receiver and sender capabilities.
    "recvAudioCaps": recvAudioCaps,
    "recvVideoCaps": recvVideoCaps,
    "sendAudioCaps": sendAudioCaps,
    "sendVideoCaps": sendVideoCaps
  }, function(answer) {
    // The responder answers with its receiver capabilities

    // Derive the send and receive parameters
    var audioSendParams = myCapsToSendParams(sendAudioCaps, answer.recvAudioCaps);
    var videoSendParams = myCapsToSendParams(sendVideoCaps, answer.recvVideoCaps);
    var audioRecvParams = myCapsToRecvParams(recvAudioCaps, answer.sendAudioCaps);
    var videoRecvParams = myCapsToRecvParams(recvVideoCaps, answer.sendVideoCaps);
    audioSender.send(audioSendParams);
    videoSender.send(videoSendParams);
    audioReceiver.receive(audioRecvParams);
    videoReceiver.receive(videoRecvParams);

    // Now we can render/play
    // audioReceiver.track and videoReceiver.track.
  });
}

EXAMPLE 8

// Assume we already have a way to signal, a transport (RTCDtlsTransport)
// and audio and video tracks. This is an example of how to answer an
// offer with audio and video tracks, and begin sending and receiving them.
// The example assumes that RTP and RTCP are multiplexed.
function myAccept(
  mySignaller, remote, transport, audioTrack, videoTrack) {
  var audioSender = new RTCRtpSender(audioTrack, transport);
  var videoSender = new RTCRtpSender(videoTrack, transport);
  var audioReceiver = new RTCRtpReceiver(transport);
  var videoReceiver = new RTCRtpReceiver(transport);

// Retrieve the send and receive capabilities
  var recvAudioCaps = RTCRtpReceiver.getCapabilities("audio");
  var recvVideoCaps = RTCRtpReceiver.getCapabilities("video");
  var sendAudioCaps = RTCRtpSender.getCapabilities("audio");
  var sendVideoCaps = RTCRtpSender.getCapabilities("video");

  mySignaller.myAnswerTracks({
    "recvAudioCaps": recvAudioCaps,
    "recvVideoCaps": recvVideoCaps,
    "sendAudioCaps": sendAudioCaps,
    "sendVideoCaps": sendVideoCaps
  });

    // Derive the send and receive parameters using Javascript functions defined in Section 17.2.
    var audioSendParams = myCapsToSendParams(sendAudioCaps, remote.recvAudioCaps);
    var videoSendParams = myCapsToSendParams(sendVideoCaps, remote.recvVideoCaps);
    var audioRecvParams = myCapsToRecvParams(recvAudioCaps, remote.sendAudioCaps);
    var videoRecvParams = myCapsToRecvParams(recvVideoCaps, remote.sendVideoCaps);
    audioSender.send(audioSendParams);
    videoSender.send(videoSendParams);
    audioReceiver.receive(audioRecvParams);
    videoReceiver.receive(videoRecvParams);

  // Now we can render/play
  // audioReceiver.track and videoReceiver.track.
}





Original issue text: https://github.com/openpeer/ortc/issues/198