Re: [rtcweb] AD Review: draft-ietf-rtcweb-jsep

One quick follow-up -- as was discussed in MMUSIC in Chicago, the 
'dtls-id' attribute defined in draft-ietf-mmusic-dtls-sdp has been 
renamed to 'tls-id' as of the -23 version that was submitted yesterday. 
This necessitates changes in the JSEP spec.

/a

On 4/19/17 00:42, Adam Roach wrote:
> RTCWEB working group --
>
> I have completed my AD review for JSEP. I want to start by saying that 
> this is an impressive body of work, and that everyone who has 
> contributed to getting it to its current state deserves a round of 
> applause. The volume of comments below does not represent that the 
> document is of poor quality. Rather, the number of comments reflects 
> on the breath, depth, and size of the document itself.
>
> All of that said, I'm going to need to see a new version of this 
> document before I put it in front of the IETF for last call. The 
> document has a number of issues that I suspect will trip people up in 
> their LC reviews. Most of these are feedback that the document authors 
> should treat as normal last call comments. The feedback that I 
> consider to block progressing the document, in my role as AD, is 
> explicitly marked with the prefix "BLOCKER", and these will need to be 
> resolved in a new version of the document before progressing it 
> further. Note that it is entirely possible that something I have 
> marked "BLOCKER" may stem from an error on my part; so recognize that 
> these are not demands for change, as much as a need to have things 
> either fixed in the document or explained to me.
>
> Some general, top-level comments:
>
> BLOCKER: The nits tool reports (and a cursory hand check supports 
> this) that six of the references are unused, and RFC4572 has been 
> (recently) obsoleted. The remaining references are largely out of date 
> by many versions, which I believe the current tools will automatically 
> handle if version numbers are simply pulled out of the source. 
> Finally, a large number of the referenced working group documents 
> still use the individual name (e.g., draft-nandakumar-rtcweb-sdp 
> instead of draft-ietf-rtcweb-sdp).
>
> There is a lot of text that should have been removed when the decision 
> to prohibit SDP munging was taken. I will call these out individually, 
> but wanted to call it out here at the top since it has impacts in many 
> places. (In this sentence and elsewhere, I use "munging" exclusively 
> to refer to changing SDP between a createOffer or createAnswer and the 
> corresponding setLocalDescription. In particular, I *do* *not* use it 
> to refer to changes applied to SDP sent to or received from the remote 
> party). Although I repeat it below, I want to be clear in my position: 
> if an implementation creates SDP, verifies that it has not changed 
> since its creation, and then has difficulty processing it, that is a 
> programming error, not a protocol error. It is not appropriate to call 
> out how to recover from programming errors in a protocol document.
>
> BLOCKER: Several of the SDP-building procedures in this document call 
> out addition of "a=fingerprint" to individual media sections, while 
> the canonical definition of "a=fingerprint" allows these to be added 
> at either the session level or the media level. The same is true of 
> ice-pwd and ice-ufrag. While the process of SDP building in this 
> document appears to be prescriptive on purpose, one assumes that it is 
> allowed some variation so as to accommodate deployment of future SDP 
> extensions. Because of this state of affairs, it is unclear whether 
> implementations that consolidate these attributes at the session level 
> when they would otherwise be identical among all media sections is 
> such an allowable variation. Ambiguity in specifications is never a 
> good thing, so I believe that the draft needs to do one of: (a) 
> specifically call out this kind of aggregation as allowed; (b) 
> specifically call out this kind of aggregation as disallowed; or (c) 
> include a blanket statement that implementations are allowed to emit 
> SDP that is semantically equivalent to that which is generated by the 
> procedures in this document, but which may vary in exact structure. I 
> have a strong preference for (c) -- implementations will be required 
> to deal with such variations anyway to interop with legacy SDP, so 
> prohibiting JSEP clients from generating them serves no goal.
>
> BLOCKER: The text in the third paragraph of section 5.7.1 
> intentionally contradicts language in RFC 5245; as such, this document 
> needs to formally update that RFC. Note that updated RFCs are expected 
> to be mentioned in the Abstract as well as the document metadata.
>
> The rest of my comments are in document order.
>
> Section 1.1 talks about Jingle as a potential transport, but the 
> remainder of the document calls out a slew of MTI SDP features that 
> don't have analogs for signaling in Jingle. I recommend either 
> qualifying the discussion of Jingle with some mention of it requiring 
> as-yet undefined extensions, or removing mention of Jingle altogether.
>
> Section 1.2 uses the term "glare" without describing it. This term may 
> be unfamiliar to readers, and should be explained or cited.
>
> Section 3.2 has a paragraph starting "Lastly, while the actual media 
> parameters are only know after an offer..."; the entire contents of 
> this paragraph appear to be actually false if both ICE and DTLS are in 
> use. I know there's conversation underway on MMUSIC and the WebRTC 
> mailing lists on this topic, but none of them have yet described a 
> situation in which this paragraph is actually true. Please remove it 
> or add a sequence diagram showing how it arises.
>
> The state labels in Figure 2 ("Remote-Offer") do not match the state 
> names used in the document text ("have-remote-offer"). Please make 
> these consistent.
>
> Section 3.3, paragraph 3 is an example of text that should have been 
> removed with SDP munging. Please remove it.
>
> The same is true of the final three words in the first paragraph of 
> section 3.4.
>
> Section 3.5.1 talks about a "recycled m= section" without any context 
> for what this means. Please provide a forward reference to the section 
> that discusses m= section recycling.
>
> BLOCKER: The final paragraph in section 3.5.1 says that candidates are 
> *only* gathered for m= sections referenced in BUNDLE tags. Section 
> 3.5.4 talks about gathering more candidates than that under certain 
> circumstances. Please make these consistent with each other.
>
> Since it has been the cause of actual interop issues in the field, it 
> is probably worth mentioning, in section 3.5.2.1, that the 
> IceCandidate strings explicitly *do not* start with "a=".
>
> The second paragraph of section 3.6.1 seems odd when talking about 
> creating answers, and downright incorrect when talking about offers. 
> Minimally, it needs text to explain why  x=0/y=0 would ever make more 
> sense than using "a=sendonly/recvonly/inactive."
>
> Section 3.6.2 talks about "original resolution" where it appears to 
> want to reference the current resolution. This happens in three 
> different places.
>
> BLOCKER: Section 3.6.2, second paragraph, second sentence says a 
> sender MUST scale to match imageattr. The same section, paragraph 
> seven, says it SHOULD scale to match imageattr. There needs to be only 
> one normative statement on this topic, so as to avoid confusion among 
> implementors.
>
> Section 3.6.2 would ideally include a disclaimer, like we have for 
> many other features, indicating that -- despite normative statements 
> in this section -- the remote side might not honor imageattr because 
> of legacy behavior, and that implementations MUST be prepared to 
> handle such situations.
>
> Section 4.1.9 paragraph one includes an enumeration of things that can 
> appear in the "type" field, but omits "rollback" from the list.
>
> Section 4.2.6, second paragraph says that codec preferences cannot add 
> formats that were negotiated away in previous offer/answer exchanges. 
> My recollection is that this conflicts with the decision taken by the 
> WebRTC working group; and a quick read of the description of 
> setCodecPreferences() in their current spec seems to confirm this (at 
> least, by omitting this restriction). Unless I'm mistaken about the 
> state of play, this paragraph should be modified to indicate that 
> codec preferences *can* add back in codecs were not negotiated in 
> previous offer/answer exchanges.
>
> BLOCKER: The first bullet on page 35 has a normative statement about 
> the AVT profile strings that must be accepted, but uses an unspecified 
> grammar to actually specify the string. If this is normative, it needs 
> to be formal. Either specify the string using a formal and cited 
> grammar, or provide an exhaustive list.
>
> Section 5.2.1 contains the statement "both 'o=' and 's=' are 
> meaningless" -- this should be qualified with "in JSEP." These fields 
> are not generally meaningless in all uses of SDP.
>
> BLOCKER: The first bullet on page 37 says that port 9 "MUST be used," 
> which contradicts the behavior required of BUNDLE-only sections (for 
> which port 0 MUST be used). Clarify.
>
> Page 37 contains a statement that "attributes of category IDENTICAL or 
> TRANSPORT should not be repeated in bundled m= sections". Two issues: 
> this reads as normative, so consider "SHOULD NOT" rather than "should 
> not." Also, I *think* this is true of bundle-only sections, not 
> bundled sections in general.
>
> Page 38, second-to-last bullet refers to "an 'a=rtcp-fb' mechanism" 
> where it should say "an 'a=rtcp-fb' line".
>
> Page 41 contains a statement reading "This promotes readability," 
> which might be read by some as promoting machine parsing rather than 
> troubleshooting. I propose replacing "readability" with "troubleshooting."
>
> BLOCKER: The bullet at the bottom of of page 42 ("If any RtpTranceiver 
> has been added...") is normative and redundant with the bullet in the 
> middle of page 44 ("If any RtpTranceiver has been added..."), which is 
> also normative. Remove one.
>
> BLOCKER: The second bullet on page 43 ("If an RtpTranceiver has been 
> stopped and is associated..." is normative and redundant with (and 
> slightly contradictory to) the bullet on page 44 starting "If any 
> RtpTranceiver has been slopped...") which is also normative. Remove one.
>
> The bullets on page 45 use the construct "MUST only" three times. This 
> is confusing, and likely to lead to implementors doing the wrong 
> thing. Please rewrite in the form "The 'a=rtcp' line MUST NOT be added 
> unless..."
>
> The paragraph that spans pages 46 and 47 claims that the 
> "VoiceActivityDetection" parameter has no impact on whether the local 
> endpoint does silence suppression for the audio it sends. The 
> paragraph immediately preceding it says that JSEP implementations MUST 
> NOT emit "CN" codecs if this parameter is set to "false." This only 
> makes sense because RFC 3264 allows SDP to specify codecs that you 
> want to receive by do not plan to generate (i.e., you can send CN even 
> if your offer doesn't have CN in it, as long as the answer *does*). 
> This is slightly counter-intuitive to many implementors, so it 
> probably bears mention.
>
> Page 51, bullet starting "For each supported RTCP feedback...": 
> replace "an 'a=rtcp-fb' mechanism" with "an 'a=rtcp-fb' line".
>
> Page 52, bullet starting "An 'a=setup' line": add the words "when 
> allowed" to the end of the bullet.
>
> Section 5.5, bullet 4 talks about parsing the local session 
> description. This is legacy from when SDP munging was allowed, and 
> should be removed (especially the normative statement about parsing 
> errors: the check that the SDP has not changed will catch changes, and 
> any implementation that cannot parse its own SDP is exhibiting an 
> internal error, not a protocol error).
>
> Section 5.7, first paragraph: similar to above, change 
> "setLocal/RemoteDescription" to "setRemoteDescription."
>
> BLOCKER: Section 5.7.1, first paragraph: this claims that a= lines are 
> order insensitive. Nothing in SDP's base definition makes this true, 
> and nothing precludes specific attributes from assigning meaning to 
> the order in which they appear. See 
> draft-ietf-slim-negotiating-human-language-08 for an example of an SDP 
> extension that *does* apply meaning to the order in which attributes 
> appear (for better or worse).
>
> Section 5.7.2 should include a statement (probably at the end) that 
> implementations may know and process attributes other than those 
> described in this section.
>
> Page 62 has a bullet ending "If this is a local description, the 
> 'ice-lite' attribute MUST NOT be specified." This sentence is leftover 
> from before SDP munging was prohibited and should be removed.
>
> Section 5.8 describes the behavior of an implementation processing its 
> own unmodified SDP. It contains significant error language that should 
> have been removed when SDP munging was removed. In particular, please 
> remove all of the following:
>
>   * "If an error is returned, the session MUST be restored to the
>     state it was in before performing these steps."
>   * "If RTCP mux is not indicated, but was previously negotiated,
>     i.e., the RTCP ICE component no longer exists, this MUST result in
>     an error."
>   * "If any indicated RTP header extension is not supported, this MUST
>     result in an error."
>   * "If any indicated media format is not supported, this MUST result
>     in an error."
>   * "If any referenced primary payload types are not present, this
>     MUST result in an error."
>
> The first bullet of section 5.8 includes: "...unless it has been 
> marked as bundle-only...," and the second includes "...and it has not 
> been marked as bundle-only..." -- in both cases, I believe the 
> indicated behavior is supposed to happen for any bundled section 
> (e.g., in a re-offer or re-answer), regardless of whether it is 
> bundle-only.
>
> Section 5.8 contains the sentence: "If there is no RtpTransceiver 
> associated with this m= section (which will only happen when applying 
> an offer)..." -- it is grammatically ambiguous whether this says that 
> the *absence* of an RtpTranceiver can only happen with an offer, or if 
> the *presence* of an RtpTranceiver can only happen with an offer. 
> Suggest rephrasing as: "If applying an offer and there is no 
> RtpTransceiver associated with this m= section, find one..."
>
> BLOCKER: Section 5.9 discusses remote description handling. This means 
> that the first bullet on page 65 talks about remote description 
> handling in the "have-local-offer" state. This means that the second 
> sentence of that bullet talks about remote offer handling when in the 
> "have-local-offer" state. That... doesn't seem right. I'm particularly 
> concerned that the language regarding ICE restart probably belongs 
> somewhere else, since it doesn't appear to belong where it is, but I 
> can't quickly figure out where it's missing from.
>
> The bullet in the middle of page 66 (starting "For each specified 
> media format...") states that unsupported media formats must be 
> ignored. The following bullet specifies that "rtx" media formats that 
> refer to "primary payload types [that] are not present" MUST result in 
> an error. Read naïvely, implementors may very well discard a payload 
> type for lack of understanding, and then discover an "rtx" for that 
> discarded PT, and generate an error ("it's not present!"). These 
> bullets need additional text regarding how they interact with each other.
>
> It's nice that you describe how you get from AS to TIAS, but as the 
> formula here isn't intended to be tuned, please simplify it for 
> imlementors as "TIAS = (950 * AS) - 16000". Minimally, include 
> parentheses around the first three terms and the final two; while most 
> people will assume proper order of operations, leaving these ungrouped 
> is asking for trouble. (I once had an argument with an implementor 
> reading an RFC who insisted that 96 wasn't an even multiple of 32; 
> they asserted, with great vehemence, that 96 is 32 times three, and 
> three is odd, so 96 is an *odd* multiple of 32. Don't underestimate 
> the ingenuity of some of the people who will end up implement these 
> specifications.)
>
> Page 67 contains the acronym "DTX" without expanding it. Please expand it.
>
> BLOCKER: Page 68 contains a sub-bullet that says "If the m= section 
> references any media formats, RTP header extensions, or RTCP feedback 
> mechanisms that were not present in the corresponding m= section in 
> the offer, this indicates a negotiation problem and MUST result in an 
> error." No. This is a violation of RFC 3264. Section 6.1 of that 
> document, when talking about generating answers, specifically stipulates:
>
>    The stream MAY indicate additional media formats, not listed in the
>    corresponding stream in the offer, that the answerer is willing to
>    send or receive (of course, it will not be able to send them at this
>    time, since it was not listed in the offer).
>
> Note, for example, that this is *exactly* the behavior that the CN 
> handling I describe above relies on. General handling for header 
> extensions and (I believe) feedback mechanisms is intended to operate 
> the same way. This sub-bullet needs to be remove in its entirety.
>
> BLOCKER: The final bullet of page 68 refers to answer directionality 
> of "sendrecv" and "sendonly." These are incorrect when applying remote 
> answers, for which these should be "sendrecv" and "recvonly." Please 
> rephrase in terms of whether media is to be sent, or fix in some other 
> way that doesn't get the sense reversed for remote answers.
>
> BLOCKER: The first bullet on page 70 exhibits the same error.
>
> Section 7 does not acknowledge IPv6. The IAB statement on IPv6 
> <https://www.iab.org/2016/11/07/iab-statement-on-ipv6/> has a 
> recommendation that all protocol specifications have IPv6 examples in 
> them. Since we have a separate example document, and Suhas has 
> indicated that he will be including some IPv6 examples in it, I think 
> you can get away in this document by amending the final paragraph of 
> section 7 as: "More examples of SDP for WebRTC call flows, including 
> examples of IPv6 addresses, can be found in [I-D.ietf-rtcweb-sdp]." I 
> make no promise that other IESG members will find this sufficient, but 
> I'm personally okay with it.
>
> For the examples, please expand the acronyms "JS" and "UA".
>
> BLOCKER: This is an issue with all of the examples. Page 37 contains a 
> bullet that asserts that (unless excluded by rare circumstances which 
> the examples do not stipulate), media formats MUST include those 
> specified in [draft-ietf-rtcweb-video], section 5. None of the 
> examples comply with this normative requirement.
>
> All examples include telephone-event audio codecs, but omit a 
> corresponding "a=fmtp" line. RFC4733 specifies:
>
>    SDP descriptions using the event payload MUST contain an fmtp format
>    attribute that lists the event values that the receiver can process.
>
> While this is a little hinky for implementations that plan to send, 
> but not to receive, DTMF (and that's the only reason I'm not marking 
> this as a blocker), I don't believe that such implementations are 
> exempted from the normative requirement (as SDP recipients may not be 
> expecting such a situation). Including such fmtp lines in the examples 
> is probably the best way to avoid resulting interop issues.
>
> BLOCKER: The example on page 83 includes "a=rid" attributes, but omits 
> a corresponding "a=extmap" attribute to indicate which value is to be 
> used for the associated header extensions.
>
> /a
>
>
>
> _______________________________________________
> rtcweb mailing list
> rtcweb@ietf.org
> https://www.ietf.org/mailman/listinfo/rtcweb