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

From: Adam Roach <[email-masked]>
Date: Wed, 19 Apr 2017 00:42:37 -0500

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

---

From: Adam Roach <[email-masked]>
Date: Wed, 19 Apr 2017 11:06:57 -0500

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:
> document is of poor quality. Rather, the number of comments reflects 
> on the breath, depth, and size of the document itself.
>
> 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.
>
> 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).
>
> 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 
> if an implementation creates SDP, verifies that it has not changed 
> since its creation, and then has difficulty processing it, that is a 
> 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.
>
> 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.
>
> names used in the document text ("have-remote-offer"). Please make 
> these consistent.
>
> *only* gathered for m= sections referenced in BUNDLE tags. Section 
> 3.5.4 talks about gathering more candidates than that under certain 
> 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=".
>
> 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.
>
> formats that were negotiated away in previous offer/answer exchanges. 
> My recollection is that this conflicts with the decision taken by 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 
> not." Also, I *think* this is true of bundle-only sections, not 
> bundled sections in general.
>
> Page 41 contains a statement reading "This promotes readability," 
> which might be read by some as promoting machine parsing rather than 
> 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..."
>
> 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".
>
> 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).
>
> 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 
>   * "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 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.
>
> 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..."
>
> 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 
> 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.
>
> 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.)
>
> mechanisms that were not present in the corresponding m= section in 
> the offer, this indicates a negotiation problem and MUST result in an 
> BLOCKER: The final bullet of page 68 refers to answer directionality 
> of "sendrecv" and "sendonly." These are incorrect when applying remote 
> 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.
>
> specified in [draft-ietf-rtcweb-video], section 5. None of the 
> examples comply with this normative requirement.
>
> 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
> [email-masked]
> https://www.ietf.org/mailman/listinfo/rtcweb

---

From: Justin Uberti <[email-masked]>
Date: Wed, 19 Apr 2017 22:27:21 -0700

Thanks for the detailed review. https://github.com/rtcweb-wg/jsep/issues is
tracking these 50 issues and their blocker status.

On Wed, Apr 19, 2017 at 9:06 AM, Adam Roach <[email-masked]> wrote:

> '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
>
>
> 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.
>
> 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
> 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.
>
> 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.
>
> 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.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.
>
> Minimally, it needs text to explain why  x=0/y=0 would ever make more sense
> than using "a=sendonly/recvonly/inactive."
>
> MUST scale to match imageattr. The same section, paragraph seven, says it
> SHOULD scale to match imageattr. There needs to be only one normative
> 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.
>
> 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.
>
> which contradicts the behavior required of BUNDLE-only sections (for which
> port 0 MUST be used). Clarify.
>
> 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".
>
> 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.
>
> 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..."
>
> 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 52, bullet starting "An 'a=setup' line": add the words "when allowed"
> to the end of the bullet.
>
> 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).
>
> implementations may know and process attributes other than those described
> in this section.
>
> '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
>    - "If RTCP mux is not indicated, but was previously negotiated, i.e.,
>    - "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.
>
> 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.
>
> 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.
>
> 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.)
>
> 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:
>
> 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.
>
> 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.
>
> <https://www.iab.org/2016/11/07/iab-statement-on-ipv6/>
> 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.
>
> 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.
>
> 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.
> rtcweb mailing [email-masked]://www.ietf.org/mailman/listinfo/rtcweb
>
>
>