Dale, thanks for your review. Thomas, thanks for your responses. I have entered a No Objection ballot.
Alissa > On Feb 1, 2018, at 5:47 PM, Thomas Stach <[email protected]> wrote: > > Dale, > > first of all apologies for the delay in responding to your thorough review. > > Thanks for your effort. > > more inline > > > On 2018-01-24 04:45, Dale Worley wrote: >> Reviewer: Dale Worley >> Review result: Ready with Issues >> >> 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. >> >> Document: draft-ietf-mmusic-trickle-ice-sip-12 >> Reviewer: Dale R. Worley >> Review Date: 2018-01-23 >> IETF LC End Date: 2018-01-26 >> IESG Telechat date: [unknown] >> >> Summary: >> >> This draft is on the right track but has open issues, described >> in the review. >> >> This draft is technically quite good, and except for various nits, >> quite well written. But there are some significant technical issues >> that need to be properly settled before the protocol is well-defined >> with good advice on robust usage. I don't think settling these issues >> will be difficult. >> >> ** Global/major issues: >> >> * The tokens "end-of-candidate" and "end-of-candidates" seem to be used >> equivalently in this document, with the following number of uses: >> >> 11 end-of-candidate >> 19 end-of-candidates >> >> Which is the correct form? > Hm, as the grammar says "end-of-candidates ", I'll change to the latter. >> * The definition of application/trickle-ice-sdpfrag describes the use of >> "a=charset" as an attribute, but the grammar of sdpfrag in section 9 >> does not permit that attribute. > The grammar allows for using extension attributes. > "a=charset" would be one of these, but might only be needed if non-ASCII > characters are possibly introduced via some potential extension. >> >> * Deployment considerations (section 11) >> >> It might be worth mentioning here what happens if a middlebox deletes >> Trickle ICE INFO requests because it does not understand them, but >> does not delete "Supported: trickle-ice" headers. It seems to me that >> in order to be robust in this situation, a UA should provide a >> globally routable address in the m= lines of the initial offer or >> answer, or if Trickle ICE INFO requests fail, eventually amend the >> addresses to be globally routable addresses. > There are so many ways how middle boxes can break any mechanism, that we > can't do much to cover everything. > RFC 5245 has recommendations (e.g. relayed candidates will always work) on > how to choose the default destination as provided in the m/c-line. > I don't think we need to repeat that reasoning here , and after all the user > agent might not want to use such a globally routable address. >> >> But the WG may have more knowledge about this situation. >> >> * There are various locations where the language has usages that seem to >> me to be excessively informal or prolix. Cases I've noticed are: >> >> 2. Terminology >> >> It is assumed that the reader will be >> familiar with the terminology from both documents. >> >> Probably s/will be/is/. > OK >> >> 3.1. Discovery issues >> >> Such Offers and Answers can >> only be handled meaningfully by agents that actually support >> incremental candidate provisioning, which implies the need to confirm >> such support before actually using it. >> >> It's probably best to omit "actually" here and elsewhere. There is >> also a use of "right from the first Offer". And in some places >> "would" is used where it is not needed or could be replaced by "do"; >> "would" should (IMO) be restricted to situations that are >> counter-factual. There is also a use of "like for example" instead of >> "for example". > I'll have pass through the document and look for these. >> >> I expect the RFC Editor can give guidance on this subject. >> >> ** Detailed/editorial/nits: >> >> A Session Initiation Protocol (SIP) usage for Trickle ICE >> draft-ietf-mmusic-trickle-ice-sip-12 >> >> The current practice is to capitalize "usage" in this situation. >> (I expect the RFC Editor can give guidance on the current >> capitalization style for RFCs.) > We switch to capitalized "Usage" as that seems correct to me. >> >> Abstract >> >> This document defines usage semantics for Trickle ICE with the >> Session Initiation Protocol (SIP) and defines a new SIP Info Package. >> >> Perhaps /a new SIP Info Package/a new SIP Info Package to support this >> usage/. >> >> Table of Contents >> >> The capitalization of section titles seems to be inconsistent. > I'll have a pass through the section titles that are taken over into the ToC >> >> 1. Introduction >> >> It describes how ICE candidates >> are to be exchanged incrementally with SIP INFO requests [RFC6086] >> >> Probably s/exchanged incrementally with/exchanged incrementally >> using/, as "with" can be read to mean that the SIP INFO requests are >> one party which is exchanging. > ok >> >> 4. Incremental Signaling of ICE candidates >> >> The following sections provide further details on how Trickle ICE >> Agents perform the initial Offers/Answers exchange (Section 4.1), >> perform subsequent Offers/Answers exchanges (Section 4.2) and >> establish the INVITE dialog usage (Section 4.3) such that they can >> incrementally trickle candidates (Section 4.4). >> >> I think you want to change "Offers/Answers" to "Offer/Answer". > Yes >> >> 4.3.1. Establishing dialog state through reliable Offer/Answer delivery >> >> It was stated in section 4.1, "Trickle ICE Agents MUST indicate >> support for Trickle ICE by including the SIP option-tag 'trickle-ice' >> in a SIP Supported: header field within all SIP INVITE requests and >> responses.", but it would be helpful to remind the reader in figure 3 >> by showing the Supported: headers: >> >> Alice Bob >> | | >> | INVITE (Offer) | >> | Supported: trickle-ice | >> |--------------------------->| >> | 183 (Answer) | >> | Supported: trickle-ice | >> |<---------------------------| >> >> and similarly in later figures. >> >> Something similar might be done with the "SRFLX Cand." elements to >> make the figures less cramped. >> >> 4.3.2. Establishing dialog state through unreliable Offer/Answer >> >> These retransmissions MUST >> cease on receipt of an INFO request or on transmission of the Answer >> in a 2xx response. >> >> Strictly, "a trickle-ice INFO request", as other INFO requests might >> be received as part of other mechanisms. > OK > I'll change to > "These retransmissions MUST cease on receipt of an INFO request > carrying a 'trickle-ice' Info Package body or on ..." >> >> 4.3.4. Considerations for Third Party Call Control >> >> The peer >> Trickle ICE Agent (the UAC) MUST send a Trickle ICE INFO request as >> soon as they receive an unreliable provisional response (see >> Figure 8). >> >> s/they receive/it receives/ > OK >> >> 4.4. Delivering candidates in INFO messages >> >> Agents MUST include a pseudo "m=" line and an identification tag in a >> "a=mid:" attribute for every "m=" line whose candidate list they >> intend to update. Such "a=mid:" attributes MUST immediately precede >> the list of candidates for that specific "m=" line. All >> "a=candidate" or "a=end-of-candidates" attributes following an >> "a=mid:" attribute, up until (and excluding) the next occurrence of a >> pseudo "m=" line, pertain to the "m=" line identified by that >> identification tag. An "a=end-of-candidates" attribute, preceding >> any pseudo "m=" line, indicates the end of all trickling from that >> agent, as opposed to end of trickling for a specific "m=" line, which >> would be indicated by a media level "a=end-of-candidates" attribute. >> >> This seems to be the first section that involves the detailed >> structure of trickle-ice INFO messages. It would probably be useful >> here to tell the reader that there is a grammar in section 9. > OK >> >> This paragraph is very dense and you should see if you can split it in >> some way. In particular, I think it is possible to separate the first >> part, which discusses the structure of the sdpfrag, from the second >> part, which specifies how the pseudo m= lines in the sdpfrag correlate >> with the m= lines in the offer/answer. > I will have a look into this in order to improve >> >> In addition, it seems to me that there is no requirement that the >> sdpfrag contains as many pseudo m= lines as the offer/answer contains >> m= lines, nor that the pseudo m= lines be in the same order as the m= >> lines that they pertain to (the correspondence being made via the mid >> attributes). Since this is very different from the semantics of SDP >> proper, I think it should be pointed out explicitly. > Makes sense, I'll add a note. >> >> 5. Initial discovery of Trickle ICE support >> >> This makes discovery fairly straightforward for Answerers or for >> cases where Offers need to be generated within existing dialogs >> (i.e., when sending UPDATE or re-INVITE requests). In both scenarios >> prior SDP would have provided the necessary information. >> >> I think you need to expand this to note that INVITE requests are >> required to include "Supported: trickle-ice" if the UA supports >> Trickle-ICE. This is important because a UA may receive an offerless >> re-INVITE as part of initiating a 3PCC -- the peer UA may be >> attempting to connect media between the UA and a third UA, and the >> third UA may not support Trickle ICE, even though the peer UA does. >> (See, e.g., RFC 3725 section 4.1 and RFC 7088 section 2.3. >> In this situation, the offerless re-INVITE would not contain >> "Supported: trickle-ice" unless the peer UA knows that the third UA >> supports Trickle-ICE. The UA, seeing the absence of "Supported: >> trickle-ice", would send a non-Trickle-ICE offer, even if the previous >> SDP had indicated support for Trickle-ICE. > Good point. I'll add an explanation along the lines of your reasoning >> >> 5.1. Provisioning support for Trickle ICE >> >> While the exact mechanism for allowing such provisioning is out of >> scope here, this specification encourages trickle ICE implementations >> to allow the option in the way they find most appropriate. >> >> What is "the option"? Perhaps "such provisioning". > Yes. I'll change accordingly >> >> 5.2. Trickle ICE discovery with Globally Routable User Agent URIs >> >> This circumvents to use of forking >> for that request. >> >> I think you want s/circumvents to/prevents the/. > OK >> >> 5.3. Fall-back to Half Trickle >> >> Figure 11 seems to be incorrect -- since Alice is using Half-Trickle >> ICE, all her candidates are sent in the INVITE, and she shouldn't send >> any INFO requests. But an INFO request from Alice is shown. > No, that is correct. The INFO from Alice acknowledges the receipt of the 183 > and stop retransmissions (see section 4.3.2) >> >> 6. Considerations for RTP and RTCP multiplexing >> >> INFO sip:[email protected] SIP/2.0 >> ... >> Info-Package: trickle-ice >> Content-type: application/sdp >> Content-Disposition: Info-Package >> Content-length: 161 >> >> Shouldn't this be "Content-type: application/trickle-ice-sdpfrag"? > Yes. Thanks for spotting this. >> >> 7. Considerations for Media Multiplexing >> >> INFO sip:[email protected] SIP/2.0 >> ... >> Info-Package: trickle-ice >> Content-type: application/sdp >> Content-Disposition: Info-Package >> Content-length: 219 >> >> Shouldn't this be "Content-type: application/trickle-ice-sdpfrag"? > ditto >> >> 9.2. Grammar >> >> A sender SHOULD stick to lower- >> case for such grammars, but a receiver MUST treat them case- >> insensitive. >> >> This is awkward. Perhaps better is "A sender SHOULD use lower-case >> for attributes from such earlier grammars, but a receiver MUST treat >> them case-insensitively." > OK >> >> I don't know the practice regarding the overall grammar of SDP, but >> the grammar shown here is very rigid regarding the order of fields in >> <session-level-fields> and in <trickle-ice-attribute-fields>. If this >> rigidity is not intended, there should be some notation on the grammar >> to show this. > I don't see any benefit in making it less rigid and I'm somewhat reluctant to > change this at this point of the process. >> >> 10.1. Rationale - Why INFO? >> >> The decision to use SIP INFO requests as a candidate transport method >> is based primarily on their lightweight nature. Once a dialog has >> been established, INFO requests can be exchanged both ways with no >> restrictions on timing and frequency and no risk of collision. >> >> On the other hand, using Offer/Answer and UPDATE requests [RFC3311] >> would introduce the following complications: >> >> A critical fact is that the sending of Trickle ICE candidates in one >> direction is entirely uncoupled from sending candidates in the other >> direction. Thus, the sending of candidates in each direction can be >> done by a stream of INFO requests that is not correlated with the >> stream of INFO requests in the other direction. And since each INFO >> request is cumulatively includes the contents of all previous INFO >> requests in that direction, ordering between INFO requests need not be >> preserved. All of this permits using largely-independent INFO >> requests. >> >> Contrarily, UPDATE or other offer/answer mechanisms assume that the >> messages in each direction are tightly coupled with messages in the >> other direction. > I like this text and will take it over, unless somebody objects. >> >> 12.2. application/trickle-ice-sdpfrag Media Type >> >> This section defines a new Media Type 'application/trickle-ice- >> sdpfrag' in accordance with [RFC6838]. >> >> s/This section/This document/ -- as in section 12.2. > OK >> >> This section should refer to the grammar of section 9 as the >> definition of the media type content. >> >> SDP files are primarily UTF-8 format text. >> >> The content of application/trickle-ice-sdpfrag is not SDP, strictly >> speaking. And as noted later in the document, not all of them are >> UTF-8. I think you want something like this: >> >> The media contents follow the same rules as SDP, except as noted >> in this document. The media contents are text, with the grammar >> specified in section 9. >> >> Then continue with "Although the initially ...", which describes the >> character set implications of "are text". > OK, will do. >> >> The "a=charset:" >> attribute may be used to signal the presence of other character >> sets in certain parts of a trickle-ice-sdpfrag body (see >> [RFC4566]). >> >> Except that "a=charset" is not permitted by the grammar of section 9. > As above, it will fall into the extension-attribute bucket if not understood. > > Thanks again for your thorough review and numerous proposal for improvement. > Regards > Thomas >> >> [END] >> >> >> > > _______________________________________________ > Gen-art mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/gen-art _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
