Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Section 4 Review Comments #204

Closed
aboba opened this issue May 21, 2015 · 1 comment
Closed

Section 4 Review Comments #204

aboba opened this issue May 21, 2015 · 1 comment

Comments

@aboba
Copy link
Contributor

aboba commented May 21, 2015

From Philipp Hancke:

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

@aboba aboba added the 1.1 label May 21, 2015
@aboba
Copy link
Contributor Author

aboba commented May 24, 2015

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

  2. Fixed.

  3. Cleaned up the conditions; now using if/else.

  4. Cleaned up the syntax; hopefully it is correct now.

  5. Are there other places where you think logging would be useful?

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

@aboba aboba closed this as completed Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant