Re: Issue 198: Philipp Hancke's Review Comments

Am 11.05.2015 um 15:28 schrieb Bernard Aboba:
> Philipp Hancke has reviewed the ORTC API and sent comments:
> http://internaut.com:8080/~baboba/ortc/ortc-with-comments.pdf
>

weird... apparently acrobat switched to a new comment format which makes 
them unreadable. Thanks Emil.

Repeating the comments out here, number + page (in the pdf) + section 
followed by quote and comment.


1) page 1, introduction
 > However,unlike the WebRTC 1.0 API, Object RealTime Communications
 > (ORTC) API does not mandate a media signaling protocol or format.

RTCPeerConnection does not mandate a media signaling protocol or format 
either. This text is just going to offend certain people.


2) page 1, introduction
 > "Tracks" and "data channels" are sent over the transports

Sending datachannels sounds odd.


3) page 4, section 1
 > ORTC does not mandate a media signaling protocol or format (as the
 > current WebRTC 1.0 does by mandating SDP Offer/Answer).

No, see comment on page 1


4) page 4, section 1, second paragraph
 > RTCIceTransportController (Section 4), [...]
 > issues are discussed in Section 16.

i'd suggest moving it either after the next paragraph or after the 
picture. Here I have no idea why those objects are defined.


5) page 4, section 2
 > The RTCDtlsTransport object includes information relating to Datagram
 > Transport Layer Security (DTLS) transport.

a high level description of the relationship between the objects would 
be helpful before this.


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)


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


8) page 7, section 2.6
 > sequence<RTCDtlsFingerprint> fingerprints;

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


9) page 7, section 2.6.1
 > role of type RTCDtlsRole, defaulting to "auto"

move up to match order of attributes in spec.


10) page 7, section 3
 > 3. The RTCIceTransport Object

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


11) page 9, section 3.5
 > The RTCIceParameters object includes the ICE ufrag and password.

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


12) page 10, section 3.5.1
 > 3.5.1 Dictionary RTCIceParameters Members

order of attributes doesn't match description here


13) page 10, 3.7
 > enum RTCIceTransportState

this is missing my favorite "failed" state from RTCPeerConnection


14) page 12, section 3.10
 > { urls: "stun:stun1.example.net }

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


15) page 12, section 3.10

 > (DOMString or sequence<DOMString>) urls;

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


16) page 12, section 3.10
 > credential of type DOMString

move down to match order


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)

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.


19) page 13, section 3.11
 > The protocol of the candidate (UDP/TCP).

is this case-sensitive?


20) page 13, section 3.11
 > host candidate that these are derived from.

for TURN, this is the srflx candidate this is derived from


21) page 13, section 3.11.2
 > "udp",

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


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 
<iq type=result/> 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


26) page 15, section 4.1
 > component of "RTP".
worth mentioning that RTCP ones also exist but can not be constructed 
directly.
Also made me wonder about SCTP


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


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...


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


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...


36) page 25, section 7.4
 > Javascript functions defined in Section 15.

Outdated ref [actually section 17 now and empty]


37) page 33, section 9.8.1
 > [{ ssrc: 2,
 > // Prioritize the thumbnail over the main video.
 > priority: 10.0 }];

Indent please


38) page 39, section 10.5
 > Sending the DTMF signal "1234" with 500 ms duration per tone

http://webrtc.github.io/samples/src/content/peerconnection/dtmf/ has a 
much nicer example :-)


That's all.

Received on Thursday, 14 May 2015 02:47:05 UTC