Reviewer: Robert Sparks Review result: Almost Ready I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair. Please treat these comments just like any other last call comments.
For more information, please see the FAQ at <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. Document: draft-ietf-rtcweb-jsep-21 Reviewer: Robert Sparks Review Date: 2017-08-08 IETF LC End Date: 2017-08-11 IESG Telechat date: Not scheduled for a telechat Summary: Almost ready for publication as a Proposed Standard RFC, but with issues that need to be addressed before publishing Major issues: The last paragraph of 5.1 needs more context. Why woudn't you expect the re-offer to fail, given that you already know the peer is using the older profile strings? It's not clear from the email list and proceedings how the document ended up with this particular text. Minor issues: Second paragraph of 3.2, last sentence: at "these parameters or reject them" - "these" and "them" have ambiguous antecedents. It's not clear if you are trying to point to the parameters that don't follow the earlier stated rule, or all parameters. The sentence is complex. Splitting it up and finding a way to avoid the need for the semicolon will likely make it easier to say what you mean. Last paragraph on page 5 - first sentence: at "One way to mitigate this" - "this" has an unclear antecedent. Did you mean "One way to reduce the complexity of the application's responsibilities"? Last paragraph of 126.96.36.199 - "all of these fields" is ambiguous. Please state precisely which fields you mean. Odd edge: The description createAnswer (in 4.1.7) leaves open the possibility that the application could call createAnswer twice (back-to-back with no intervening calls to other APIs) with different values in the options parameter. Is this ok? If not, should there be language that says not to do that somewhere? Slightly more than minor maybe: The first paragraph of 5.1.2 has at least a reference error. UDP/TLS/RTP/SAVPF is not specified in RFC7850. It's in RFC5764. Assuming that this is only a reference error, then the last sentence seems like a stray. It doesn't seem to be in the right context ("this advertisement" doesn't cast back to the discussion of the two profiles above it, which are both UDP only). Please clarify the sentence, giving it the right context, or remove it. The fourth sentence of the second bullet of section 5.2.1 is confusing. (It starts "It is RECOMMENDED that the <sess-id>".) RFC3265 requires the initial sess-id high-order bit to be zero. As written, this can be read to say that setting that bit to zero is only RECOMMENDED, and that it's possible to have sess-ids that are something other than 64 bits. You mean to say "Do what 3265 says to do and make the lower 63 bits random". Here's one possible replacement for the sentence: "RFC 3265 requires that the high-order bit of the initial <sess-id> be zero. It is RECOMMENDED that the remaining 63 bits be initially set to a cryptographically random value." The last paragraph of 5.3.1 prohibits the same attributes from generated answers that are prohibited in generated offers. Consider adding text for what to do if an offer shows up from a peer with some of those prohibited attributes. The second paragraph of section 5.4 is confusing. A slight restructuring of the sentences would remove much of the potential confusion, I think. I suggest replacing the first sentence (which is very complex) with this: "After calling setLocalDescription, the application MAY modify the SDP to reduce the capabilities in the offer before sending it to the far side, as long as it remains a valid SDP offer and specifies a subset of what was in the original offer. Likewise, an application that has received an offer from a peer MAY modify the received SDP, subject to those same constraints, before calling setRemoteDescription." The first paragraph of section 5.7.1 is simply restating requirements from RFC4566. Restating normative requirements like that has led to interop problems through unintended introduced ambiguity and spec maintenance problems. Please consider rewriting the paragraph to say "RFC4566 says" and avoid the 2119 keywords. On page 69, at "If the application attempt to transmit DTMF when using a media format that does not have a corresponding telephone-event format, this MUST result in an error". This sentence is out-of-place. The section is about applying a remote description. If the document needs to retain the sentence, it belongs off in some "processing media" section. In particular, the current placement of text is confusing about what and when an error could be returned, and it could be misread to say that an error should be returned to the setRemoteDescription call. Nits: Section 1.2 second paragraph - "This was rejected based on a feeling". Can you write something that speaks to consensus here? "Based on a feeling" isn't quite right. Maybe "using the argument"? PeerConnection is used first at the next to last paragraph of section 3. Please add a reference at that point. "LS" (lipsync) is first used at 4.1.2, with no context for what it is. Please add a reference. The concept of a "pending local description" is used in sections 3.5.1, 4.1.6, and 4.1.7 but isn't really explained until much later in the document. Please provide forward references, or move a description of what a pending description is earlier in the document. In 4.1.7 (create Answer) why would calling setLocalDescription cause the answer to be a _pending_ local description. I think it's the full on local description at that point, not a pending one? At 4.1.17's first paragraph: Why is "if parsed successfully" necessary to call out? I suggest removing the phrase. 4.2.3 talks about setting direction properties, but the values that can be set aren't called out (and even then only by inference from a list in an example) in section 4.2.5. Should there be explicit pointers to the api doc here? (And in section 4.2.6?) There are two paragraphs on page 50 that start "The next step is to". They are both referring to the same step. I suggest combining the paragraphs. The 4th bullet on page 64 is ambiguous at "this m= section". It looks like this bullet should just be merged with the previous bullet. The second paragraph of 6.8 starts "Next, ". I think it should start "First, ". It's not clear what the "Or," in the first bullet on page 65 is referring to. Are you trying to say "If the m= section is not new, and the ICE ufrag and password values have changed" or something else? In the last paragraph of section 8 at "programmer MUST recognize". That is not an appropriate use of 2119. I suggest "needs to be aware" instead of "MUST recognize". Micro Nits: Suggested change to the first sentence of 1.1 OLD: The thinking behind WebRTC call setup has been to fully specify and control the media plane, but to leave the signaling plane up to the application as much as possible. New: WebRTC call setup has been designed to focus on controlling the media plane, leaving signaling plane behavior up to the application as much as possible. The first full bullet on page 18 has a sentence that starts "Note that when considering". Please change that to simply "When considering". As written, there is an implication that this is a consequence of something specified somewhere else. The verb "surfaced" appears three times in the document without support or definition. Can you replace them with simpler language? At the second to last sub-bullet at the end of page 66, please remove "note that". The sentence is stronger without it. I noted these sections as particularly hard to read when going through the document linearly. I don't have concrete suggestions for improvement, but perhaps other eyes (if they agree the sections could use improvement) can come up with something: - First sentence of the first paragraph on page 19. - 2nd sentence of the 4th paragraph on page 19. _______________________________________________ Gen-art mailing list Genemail@example.com https://www.ietf.org/mailman/listinfo/gen-art