Re: [MMUSIC] Review of draft-ietf-mmusic-ice-sip-sdp-10

[Suhas Nandakumar <[e-mail]>]

Hello Adam

  Many thanks for your detailed review and apologies for the delayed
response.

The authors discussed on your Big Picture questions and we need help from
the WG to decide on the next steps. I will be following this email with
separate emails on this topic reaching out for more information.

I have submitted version-12 that incorporates most of editorial and
technical comments. Please let us know if there were any omissions.

However, here are few comments that  need more inputs from you. I have
included them below for ease of readability.

Section 8.1.1, final paragraph: "it's a localized decision" is extremely
informal and doesn't match the general style for RFCs. Please rephrase.

[Suhas] - Could you suggest a viable rephrase.

Section 8.1.2, first paragraph describes a kind of difficult-to-follow
situation in which an INVITE contains an offer, and the answer is provided
in a PRACK response, resulting in an empty "200 OK." Two comments: (1) this
would really benefit from a sequence diagram; and (2) are SBCs and ALGs
likely to do the right thing with empty 200s?

[Suhas]  - On (2), I am not well versed with SBCs and ALGs behavior. Not
sure, what is recommended that we do here. Any thoughts ?

Section 4.2.2.2.3 specifies, during offer processing, that the
implementation "SHOULD wait for [ICE] checks to complete" before sending an
answer. Keeping in mind that these updates typically happen with the SIP
UPDATE method, and given that RFC 3261 specifies "TUs SHOULD respond
immediately to non-INVITE requests" (cf. RFC3261 section 17.1), I'm
concerned about the timing implications here. We should minimally point out
that we're explicitly telling UAs to ignore this normative statement in
RFC3261, and make sure that we've done the analysis to ensure that we're
not going to cause problems for the SIP state machine (I'm concerned, in
particular, about unnecessary SIP retransmissions here).

[Suhas] - I am not aware of any analysis being done. What do you suggest we
need to do ?



Thanks
Suhas



On Thu, Jan 12, 2017 at 2:46 PM, Adam Roach <[e-mail]> wrote:

> I've done a review of draft-ietf-mmusic-ice-sip-sdp-10, and have a
> handful of comments. I've broken these down into three sections:
> big-picture questions, technical comments, and editorial comments. I
> apologize for the lateness in posting this review; the comments are
> substantial, and converting them into email form took far longer than I
> anticipated.
>
>
> Big Picture Questions
>
> The text in section 9, read literally, deprecates RFCs 4091 and 4092. If
> that is the actual intention, the abstract and "obsoletes" metadata need to
> be updated to reflect this fact. If the deprecation is qualified (e.g., the
> RFCs are still okay in other contexts, but should not be used in
> conjunction with ICE), then the text in section 9 needs to be much clearer.
>
> It's not clear to me what the relationship is between this document and
> RFC 5245 (and, for that matter, 6544). Is this supposed to obsolete RFC
> 5245? That appears to be the intention. It's also not clear why we would
> obsolete 5245 without also incorporating 6544 -- it seems that doing so
> leaves the use of ICE TCP with SIP to be kind of in limbo.
>
> Assuming the intention *is* to replace RFC 5245, the structure of this
> document seems to leave non-SIP uses of RFC3264 (e.g., WebRTC) stranded. If
> the intention is *not* to replace RFC 5245, the replication of all of the
> SDP syntax seems to be problematic: the the case of a conflict, which
> specification is intended to govern?
>
> In short, I think the relationship between this document, the contemporary
> work (e.g., ICE-BIS), and the historical work on which it is based needs to
> be made much clearer in the document itself, and we need to have a clear
> picture about how to handle SDP for non-SIP uses of ICE (e.g., by splitting
> this document into an SDP part and a SIP part, similar to how we split 3264
> from 3261).
>
>
> Technical Comments
>
> I believe that the RECOMMENDED-strength normative statement in the final
> paragraph of 4.1.1.1 overstates things a bit. While this guidance makes set
> in certain circumstances. For example, for devices that are known to
> typically known to be on a single network with mutual routability, using a
> relayed candidate as a default will lead to calls being unnecessarily sent
> out of network and back. I strongly suggest that this paragraph be reworked
> to be a considered discussion of the different trade-offs involved in using
> each kind of candidate, with a non-normative suggestion to make the
> behavior configurable in user agents. What I want to avoid here is guidance
> which leads to devices behind a single ALG necessarily going through two
> relays by default when talking to each other (which is what the current
> guidance can lead to -- I can sketch this out in more detail if it's not
> obvious how this happens).
>
> Section 4.1.1.2 uses the word "media stream" in a way that's not
> consistent with the way that term is defined by RFC 7656, which could lead
> to confusion. In RFC 7656 terminology, "media stream" is an alias for "RTP
> stream," which is identified by a single SSRC. With technologies such as
> simulcast. A single m-line can contain multiple media streams. I think you
> want the term "source stream," with a citation of that term in RFC 7656.
>
> Section 4.1.1.2 discusses ice-options attributes in the terms of
> "support", without any discussion of activation. Although this is treated
> (somewhat lightly) later in the document, this smells like the same kind of
> potential morass we ran into with SIP options tags: the patchwork means of
> indicating feature *support* versus feature *activation* made it very
> difficult to specify and implement things in a consistent fashion. I
> strongly recommend that this document spend a bit more text discussing this
> distinction, and maybe even consider a formal syntax for distinguishing
> between supported and activated features.
>
> Section 4.1.3 should probably point out that, in the case of forking, the
> same local candidates are used for all branches of the fork.
>
> Section 4.2, section paragraph: replace "be rejected" with "fail" -- there
> are more ways to fail than rejection, and the action should be the same for
> rejection as for other failures.
>
> Section 4.2.1.2 should add motivation for the requirement imposed by its
> second paragraph, or the requirement should be removed. The need to do this
> is not obvious, and I suspect it may be unnecessary.
>
> Section 4.2.2.2.3 specifies, during offer processing, that the
> implementation "SHOULD wait for [ICE] checks to complete" before sending an
> answer. Keeping in mind that these updates typically happen with the SIP
> UPDATE method, and given that RFC 3261 specifies "TUs SHOULD respond
> immediately to non-INVITE requests" (cf. RFC3261 section 17.1), I'm
> concerned about the timing implications here. We should minimally point out
> that we're explicitly telling UAs to ignore this normative statement in
> RFC3261, and make sure that we've done the analysis to ensure that we're
> not going to cause problems for the SIP state machine (I'm concerned, in
> particular, about unnecessary SIP retransmissions here).
>
> Section 4.2.4.1.4: the final paragraph seems to reiterate behavior
> described in ICE-BIS. It's probably not ideal to describe this in two
> places, as any subtle difference can result in interop problems.
>
> Given the state of ICE today, I think the definition of "candidate" in
> section 5.1 should also include the syntax for TCP transports.
>
> Section 5.1, <connection-address>: the language in here that states "If
> the DNS query returns more than on IP address, on is chosen," implying that
> one is chosen at random. There are specific procedures for how this is done
> (especially with IPv6); the statement should point to the selection
> mechanisms.
>
> Section 11.2.1, final paragraph: We should have text in here addressing
> the fact that user agents that are not willing to receive non-ICE answers
> need to include "Require: ice" in their offer; and that clients that reject
> non-ICE offers should use the 421 response code to do so, and should
> indicate "Require: ice" in their rejections.
>
>
>
> Editorial Comments (of varying severity)
>
> The final sentence of the definition of "Default Destination/Candidate" is
> a bit difficult to follow. Suggest: ' For the RTCP component, the address
> and port are indicated using the "a=rtcp" attribute defined in [RFC3605],
> if present; otherwise, the RTCP component address is the same as the
> address of the RTP component, and its port is one greater than the port of
> the RTP component.'
>
> Section 3, final paragraph: remove "the" before "[ICE-BIS]".
>
> Section 4.1.3, second paragraph: eliminate the space before the first
> comma. Place a comma between "its role" and "and processes"
>
> Section 4.1.3, final paragraph: replace "complaint" with "compliant".
>
> The contents of section 4.1.5 appear to be completely redundant with the
> contents of section 4.1.5.1.1. I would recommend eliminating section 4.1.5.
> Also, the section numbering here is really baroque and unnecessarily deep.
> For example, section 4.1.5.1 contains no text and only one subsection; this
> is also true of section 4.1.5.2. The more I look at this, the more bizarre
> it appears. I think the correct thing to do here is:
> (a) Remove all the text currently in section 4.1.5.
> (b) Move the paragraph in section 4.1.5.1.1 and the two paragraphs in
> 4.1.5.2.1 into section 4.1.5
> (c) Eliminate the (now empty) sections 4.1.5.1, 4.1.5.1.1, 4.1.5.2, and
> 4.1.5.2.1.
>
> Section 4.2.1.2.1, second paragraph says "The agent MAY include additional
> candidates it did not offer previously" before we explain how this can
> happen. I recommend adding a "(see section 4.2.4.1.1)" after "previously".
>
> Section 4.2.2.3, final paragraph: The second line has an "i" that has been
> separated from the rest of its word (which appears on the following line).
>
> Section 4.2.3: add commas: "being ICE-unaware, a subsequent" ... "this is
> done, this could result" ... "ICE-unaware, that answer would not"
>
> Section 4.2.3.1.1 should not rely on the section header exclusively for
> context here. The first sentence should read "If ICE support is indicated
> int he SIP answer and the offer was a restart..." Ditto for sections
> 4.2.3.1.3 (add "and ICE is running") and 4.2.3.1.3 ("and ICE is completed").
>
> Section 5.1 indicates that ABNF is used for "candidate," but this is not
> repeated for (e.g.) sections 5.3. and 5.4. I would suggest moving the
> statement from 5.1 (and 5.2) into section 5 so that it applies to all
> syntax definitions.
>
> In all of section 5, the attributes really should include examples.
>
> Also, section 5 should indicate which exernally-defined ABNF constructs
> are being used in the document. This is done in some locations, but not
> others. Running the ABNF through a validator will highlight all
> externally-defined elements; it will also help uncover any other issues
> with the ABNF.
>
> All attributes are defined with ABNF of the form: "attribute-name" ":" --
> this is unnecessary, and should be shortened to "attribute-name:".
>
> Section 5.1, under <connection-address>: change "using an A" to "using an
> A record."
>
> Section 5.1, under <transport> refers to specification of TCP with ICE in
> the future tense rather than the past. This should be reworked.
>
> Section 5.1, under <foundation>, "...in the Frozen algorithm" should be
> followed by "defined in" and then a citation for where the Frozen algorithm
> is defined.
>
> Section 5.1, <priority> -- this just says it's an integer, without
> describing its meaning. Either explain what it means, or cite something
> that does so.
>
> Section 5.1, <rel-addr> and <rel-port>: replace "<rel-addr> and <rel-port>
> is equal" with "<rel-addr> and <rel-port> are equal".
>
> Section 5.2, in the ABNF for "remote-candidate", indicate that
> component-ID is defined in RFC 4566.
>
> Section 5.4, final paragraph: this indicates that the grammar allows for
> "6 bits of randomness per character" -- this should indicate that it allows
> for "6 bits of information per character".
>
> Section 6: rewrite the first sentence using active voice so as to
> positively identify which entities the requirements apply to.
>
> Section 7.1: "Note that" is an awkward way to start a section; I suggest
> striking it.
>
> Section 7.1, first line: replace "may not" with "might not." I had to read
> this several times to realize that the intended meaning here was "might
> not." In colloquial usage, "may not" is a prohibition (e.g., "you may not
> take more than two cookies.")
>
> Section 8.1, first paragraph refers to "ringing the phone of the called
> party." Old-style telephones are only one use case for SIP; this should
> instead read "alerting the called user agent."
>
> Section 8.1.1 second paragraph is HUGE. Consider how to split it up to be
> less "wall of text"y.
>
> Section 8.1.1, fourth paragraph replace "each candidate" with "every
> candidate."
>
> Section 8.1.1, final paragraph: "it's a localized decision" is extremely
> informal and doesn't match the general style for RFCs. Please rephrase.
>
> Section 8.1.2, first paragraph describes a kind of difficult-to-follow
> situation in which an INVITE contains an offer, and the answer is provided
> in a PRACK response, resulting in an empty "200 OK." Two comments: (1) this
> would really benefit from a sequence diagram; and (2) are SBCs and ALGs
> likely to do the right thing with empty 200s?
>
> Section 8.1.2, second paragraph indicates that placing an offer in an
> INVITE the 2xx can cause substantial PDD and clipping; this is true
> regardless of whether ICE is in use. I think you mean to say that ICE can
> exacerbate PDD and clipping in this case rather than causing it.
>
> Sections 8.4, 9, and 11.1 -- remove the redundant RFC numbers (as in
> "defined in RFC 3312 [RFC3312]").
>
> Section 10 refers to timer Ta without indicating where it is defined.
>
> Section 11.1: Replace "SIPS" with "TLS". As Olle is fond of noting, there
> are circumstances in which TLS is feasible but SIPS is not, and specifying
> "TLS" here is sufficient.
>
> Section 11.2: replace "stun" with "STUN."
>
> Section 11.2.1, final paragraph: replace "if its not used" with "if it's
> not used."
>
> Section 11.2.2 -- expand the acronym "NAT" on first use.
>
> Section 11.2.2, first paragraph: replace "application layer SIP" with
> "application-layer SIP"
>
> Section 12.1: Insert "The" at the very beginning of the first sentence.
>
> Section 12.2: Make "RFC 5245" a reference.
>
> Section 12.2, third paragraph should start "[RFC6679] defines the..." (add
> the word "the")
>
> Section 13: make "RFC 5245" and "RFC 6336" references.
>
>
> /a
>
> _______________________________________________
> mmusic mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/mmusic
>