Philipp Hancke's Review Comments #198

Closed
aboba opened this Issue May 11, 2015 · 5 comments

Projects

None yet

2 participants

@aboba
Contributor
aboba commented May 11, 2015

Review comments from Philipp Hancke are available here:
http://internaut.com:8080/~baboba/ortc/ortc-with-comments.pdf

@aboba aboba added the 1.1 label May 11, 2015
@aboba aboba changed the title from Phillip Hancke's Review Comments to Phillipp Hancke's Review Comments May 11, 2015
@aboba aboba changed the title from Phillipp Hancke's Review Comments to Philipp Hancke's Review Comments May 11, 2015
@elagerway
Contributor

Thanks Bernard. I have sent an email to &yet requesting that Phillipp break out these comments so we can all read them in the issue tracker without the need of Adobe Acrobat.

@aboba
Contributor
aboba commented May 16, 2015

From Philipp:

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.

  1. page 1, introduction

"Tracks" and "data channels" are sent over the transports

Sending datachannels sounds odd.

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

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

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

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

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

  1. page 7, section 2.6

sequence fingerprints;

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

  1. page 7, section 2.6.1

role of type RTCDtlsRole, defaulting to "auto"

move up to match order of attributes in spec.

  1. page 7, section 3
  1. 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 on top of ICE.

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

  1. page 10, section 3.5.1

3.5.1 Dictionary RTCIceParameters Members

order of attributes doesn't match description here

  1. page 10, 3.7

enum RTCIceTransportState

this is missing my favorite "failed" state from RTCPeerConnection

  1. page 12, section 3.10

{ urls: "stun:stun1.example.net }

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

  1. page 12, section 3.10

(DOMString or sequence) urls;

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

  1. page 12, section 3.10

credential of type DOMString

move down to match order

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

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

  1. page 13, section 3.11

The protocol of the candidate (UDP/TCP).

is this case-sensitive?

  1. page 13, section 3.11

host candidate that these are derived from.

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

  1. page 13, section 3.11.2

"udp",

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

  1. page 14, section 3.14
    [first comment in that section is invalid]

gatherOptions.iceservers = ... ;

Just make it []

  1. page 14, section 3.14

get tracks and RTP objects from other example

cross-ref would be helpful

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

  1. page 15, section 3.14

gatherOptions.iceservers = ... ;

[] is shorter

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

  1. page 16, section 4.4
    [first comment in that section is invalid]

As well as

s/As/as

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

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

  1. page 18, section 4.4

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

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

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

  1. page 21, section 5.9

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

I don't see that

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

  1. page 21, section 5.9

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

this seems very odd semantics for a callback...

  1. page 25, section 7.4

Javascript functions defined in Section 15.

Outdated ref [actually section 17 now and empty]

  1. page 33, section 9.8.1

[{ ssrc: 2,
// Prioritize the thumbnail over the main video.
priority: 10.0 }];

Indent please

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

@aboba aboba added the 1.0 label May 16, 2015
@aboba
Contributor
aboba commented May 18, 2015

This review is now divided up into Issues 201 - 206, so I will flag this Issue as a duplicate.

@aboba aboba added the duplicate label May 21, 2015
@elagerway
Contributor

In order for these changes to be accepted Philipp Hancke will need to join the CG, agree to the CLA and preferably subscribe to the mail list, if he is not already doing so. Erik will email &yet to try and attempt to get this sorted out. Until then, we will have to wait on vetting and integrating these proposed changes.

@aboba
Contributor
aboba commented Jun 11, 2015

I have broken up Philipp's comments into individual issues, so I am closing this "big" issue.

@aboba aboba closed this Jun 11, 2015
@robin-raymond robin-raymond pushed a commit that referenced this issue Jun 22, 2015
Robin Raymond Addressed Philipp Hancke's review comments, as noted in: Issue #198
Added the "failed" state to RTCIceTransportState, as noted in: Issue #199
Added text relating to handling of incoming media packets prior to remote fingerprint verification, as noted in: Issue #200
Added a complete attribute to the RTCIceCandidateComplete dictionary, as noted in: Issue #207
Updated the description of RTCIceGatherer.close() and the "closed" state, as noted in: Issue #208
Updated Statistics API error handling to reflect proposed changes to the WebRTC 1.0 API, as noted in: Issue #214
Updated Section 10 (RTCDtmfSender) to reflect changes in the WebRTC 1.0 API, as noted in: Issue #215
Clarified state transitions due to consent failure, as noted in: Issue #216
Added a reference to [FEC], as noted in: Issue #217
894b08b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment