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:al...@example.com 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:al...@example.com 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
Gen-art@ietf.org
https://www.ietf.org/mailman/listinfo/gen-art
