Re: [Gen-art] Genart last call review of draft-ietf-mmusic-trickle-ice-sip-12

[Thomas Stach <[e-mail]>]

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:[e-mail] 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:[e-mail] 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]
>
>
>