Re: Issue 198: Philipp Hancke's Review Comments

1) - 4)

Abstract and Section 1

How about this?

"Object Real-Time Communications (ORTC) provides a powerful API for the development of WebRTC based applications. ORTC does not utilize Session Description Protocol (SDP), nor does it mandate support for the Offer/Answer state machine. Instead, ORTC uses "sender", "receiver" and "transport" objects, which have "capabilities" describing what they are capable of doing, as well as "parameters" which define what they are configured to do. "Tracks" are encoded by senders and sent over transports, then decoded by receivers while "data channels" are sent over transports directly."

In terms of describing the figure, as well as the flow from lower level to higher level objects, is this better?

"In the figure above, the RTCRtpSender (Section 5) encodes the track provided as input, which is transported over a RTCDtlsTransport (Section 4). An RTCDataChannel (Section 11) utilizes an RTCSctpTransport (Section 12) which can also be multiplexed over the RTCDtlsTransport. Sending of Dual Tone Multi Frequency (DTMF) tones is supported via the RTCDtmfSender (Section 10).

The RTCDtlsTransport utilizes an RTCIceTransport (Section 3) to select a communication path to reach the receiving peer's RTCIceTransport, which is in turn associated with an RTCDtlsTransport which de-multiplexes media to the RTCRtpReceiver (Section 6) and data to the RTCSctpTransport and RTCDataChannel. The RTCRtpReceiver then decodes media, producing a track which is rendered by an audio or video tag.

Several other objects also play a role. The RTCIceGatherer (Section 2) gathers local ICE candidates for use by one or more RTCIceTransport objects, enabling forking scenarios. The RTCIceTransportController (Section 7) manages freezing/unfreezing (defined in [RFC5245]) and bandwidth estimation. The RTCRtpListener (Section 8) detects whether an RTP stream is received that cannot be delivered to any existing RTCRtpReceiver, providing an onunhandledrtp event handler that the application can use to correct the situation.

Remaining sections of the specification fill in details relating to RTP capabilities and parameters, operational statistics, media authentication via Identity Providers (IdP) and compatibility with the WebRTC 1.0 API. RTP dictionaries are described in Section 9, the Statistics API is described in Section 13, the Identity API is described in Section 14, an event summary is provided in Section 15, and WebRTC 1.0 compatibility issues are discussed in Section 16. "


5) In Section 1, add:

"In the figure above, the RTCRtpSender (Section 5) encodes the track provided as input, which is transported over a RTCDtlsTransport (Section 4). An RTCDataChannel (Section 11) utilizes an RTCSctpTransport (Section 12) which can also be multiplexed over the RTCDtlsTransport. Sending of Dual Tone Multi Frequency (DTMF) tones is supported via the RTCDtmfSender (Section 10).

The RTCDtlsTransport utilizes an RTCIceTransport (Section 3) to select a communication path to reach the receiving peer's RTCIceTransport, which is in turn associated with an RTCDtlsTransport which de-multiplexes media to the RTCRtpReceiver (Section 6) and data to the RTCSctpTransport and RTCDataChannel. The RTCRtpReceiver then decodes media, producing a track which is rendered by an audio or video tag.

Several other objects also play a role. The RTCIceGatherer (Section 2) gathers local ICE candidates for use by one or more RTCIceTransport objects, enabling forking scenarios. The RTCIceTransportController (Section 7) manages freezing/unfreezing (defined in [RFC5245]) and bandwidth estimation. The RTCRtpListener (Section 8) detects whether an RTP stream is received that cannot be delivered to any existing RTCRtpReceiver, providing an onunhandledrtp event handler that the application can use to correct the situation.

Remaining sections of the specification fill in details relating to RTP capabilities and parameters, operational statistics, media authentication via Identity Providers (IdP) and compatibility with the WebRTC 1.0 API. RTP dictionaries are described in Section 9, the Statistics API is described in Section 13, the Identity API is described in Section 14, an event summary is provided in Section 15, and WebRTC 1.0 compatibility issues are discussed in Section 16."

7) In Section 13.1, change:

"For RTCDtlsTransport.getStats(), check whether RTCDtlsTransport.start() has been called; if not, throw an InvalidStateError exception. For RTCIceTransport.getStats(), check whether RTCIceTransport.start() has been called; if not, or if RTCIceTransport.stop() has been called, throw an InvalidStateError exception. For RTCRtpSender.getStats(), check whether RTCRtpSender.send(parameters) has been called; if not, throw an InvalidStateError exception. For RTCRtpReceiver.getStats(), check whether RTCRtpReceiver.receive(parameters) has been called; if not, throw an InvalidStateError exception."

To:

"For RTCDtlsTransport.getStats(), check whether RTCDtlsTransport.state is "closed"; if so, throw an InvalidStateError exception. For RTCIceTransport.getStats(), check whether RTCIceTransport.state is "closed"; if so, or if RTCIceTransport.stop() has been called, throw an InvalidStateError exception. For RTCRtpSender.getStats(), check whether RTCRtpSender.send(parameters) has been called; if not, throw an InvalidStateError exception. For RTCRtpReceiver.getStats(), check whether RTCRtpReceiver.receive(parameters) has been called; if not, throw an InvalidStateError exception."

8) This won't affect the ability to emulate WebRTC 1.0 over ORTC, only vice-versa.

9) Fixed.

10) the order of sections here is somewhat confusing... first we are doing DTLS, then ICE, then RTP. Whereas on the network, DTLS is run on top of ICE.

[BA] Agree that it could be improved. How about the following sequencing for the first 7 chapters:
1.Overview
2.The RTCIceGatherer Object
3.The RTCIceTransport Object
4.The RTCDtlsTransport Object
5.The RTCRtpSender Object
6.The RTCRtpReceiver Object
7.The RTCIceTransportController Object

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

[BA] How about if we write it out in full the first time (with abbreviations in parenthesis), then use ufrag/pwd after that?

12) order of attributes doesn't match description here

[BA] Fixed.

13) page 10, 3.7

enum RTCIceTransportState
this is missing my favorite "failed" state from RTCPeerConnection

[BA] Will do (see Issue 199).

14) missing " [after .net --] bug was fixed in webrtc-pc IIRC

[BA] Fixed.

15) I found it surprising that urls can be a string. But it also can in PC

[BA] Yes. Kept it the same in ORTC for consistency with 1.0.

16) move down to match order
[BA] Done.

17) page 12, section 3.11

WebIDL
how does this deal with the extensibility defined in RFC 5245 --
https://tools.ietf.org/html/rfc5245#section-15.1 (extension-att-name etc)

[BA] It is possible to add variables to the dictionary. so it is possible to add new attributes for extension-att-name/extension-att-value. Do you think there is something missing in particular that needs to be added?

18) page 12, section 3.11

DOMString relatedAddress = "";
unsigned short relatedPort;
I don't think those attributes are useful, just a potential leak of ip addresses when forcing turn-only relays. So I would not expose them.

[BA] These attributes are included so as to allow formulation of an SDP offer/answer as described in RFC 5245, and required in WebRTC 1.0 SDP. So I understand your concern - but am not sure how to address it without creating a backward compatibility problem.

19) page 13, section 3.11

The protocol of the candidate (UDP/TCP). is this case-sensitive?

[BA] Most enum values are lower-case; an exception is component, which is RTP/RTCP.

20) page 13, section 3.11

host candidate that these are derived from.
for TURN, this is the srflx candidate this is derived from

[BA] deleted "host".

21) page 13, section 3.11.2

"udp",
see case question above (19). Jingle and SDP and consequently chrome and firefox disagree here

[BA] We do use "udp"/"tcp" as well as "RTP"/"RTCP".  Do you think that is a problem?

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

gatherOptions.iceservers = ... ;
Just make it []

[BA] Will either do that or use the example ICE servers.

23) page 14, section 3.14

get tracks and RTP objects from other example
cross-ref would be helpful

[BA] Done.

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.

[BA] The example does assume that there is an offer and an answer, though not using SDP semantics.  So probably worthwhile to note that.

25) page 15, section 3.14

gatherOptions.iceservers = ... ;
[] is shorter

[BA] Will either do that or include the example ICE servers.

26) Added text: "An RTCIceTransportController object provides methods to add and retrieve RTCIceTransport objects with a component of "RTP" (associated RTCIceTransport objects with a component of "RTCP" are included implicitly)."

27) Fixed.

28) Cleaned up the conditions; now using if/else.

29) Cleaned up the syntax; hopefully it is correct now.

30) Are there other places where you think logging would be useful?

31) Changed to:

"As noted in [RFC5245] Section 7.1.2.3, an incoming connectivity check utilizes the local/remote ufrag and the local pwd, whereas an outgoing connectivity check utilizes the local/remote ufrag and the remote pwd. Since start() provides role information, an RTCIceTransport object can respond to incoming connectivity checks and initiate connectivity checks."

33) Added "so that both RTP and RTCP IceGatherer and IceTransport objects are needed."

34) Seems odd to have the function definition at the top.

35) What makes the callback odd is that a distinct IceTransport isn't created for each fork.  There are other things wrong too as noted in Issue 170.  So will wait to revise it until Issue 170 is resolved as well.

36) Change reference to Section 17.2.

37) Change text to:

// Send a thumbnail along with regular size, prioritizing the thumbnail (ssrc: 2)
var encodings = [{ ssrc: 1, priority: 1.0 }]
 var encodings = [{ ssrc: 2, priority: 10.0 }];


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