Subject: Re: [MMUSIC] Review of draft-ietf-mmusic-ice-13

From: Jonathan Rosenberg <[email-masked]>
Date: Sun, 04 Mar 2007 22:21:17 -0500

ekr,

Thanks for the extremely detailed and thorough comments. I think they 
have substantially improved readability. Only took me about 15 hours to 
go through them ;)

Responses inline (nothing critical in here):

Eric Rescorla wrote:

> $Id: draft-ietf-mmusic-ice-13-rev.txt,v 1.1 2007/01/16 23:38:58 ekr Exp $
> 
> GLOBAL COMMENTS
> There is a *lot* of new terminology in this document. In my experience
> glossaries at the beginning really don't help much. Each term should
> be capitalized on first introduction and come with a definition in
> context. Wherever possible, we should avoiding new terms that are
> only used a few times.

Done.

> 
> In S 1-12 there are lots of forward references to stuff that
> is only defined in S 12. I understand the urge to keep the
> formal grammar towards the end, but you need to provide some
> sort of abstract sketch of what's in an offer or answer in
> order to make this understandable before the definition.

I think this is addressed now.

> 
> Somewhere there needs to be a diagram of the relationship of the
> various pieces of the system. I.e., addresses, candidates, candidate
> pairs, checks, check lists, valid lists, etc.

Well, these are really hard to do. I'm iffy on whether it would help.
FOr example gruu had a uml model that tried to explain relationship of
terms and it only confused people so it got yanked.

Anyway, I did take a stab. See my other post for a proposal.

> 
> Many sections are just run-on descriptions of
> protocol elements or operational steps. These each need to be broken
> out with some sort of textual marker (bullets, whitespace, heds,
> depending on the length of the relevant subsection). For instance,
> S 4.1.1 reads (in part)

this was fixed extensively, probably the biggest change for -14

> 
>    An agent gathers candidates when it believes that communications is
>    imminent.  An offerer can do this based on a user interface cue, or
>    based on an explicit request to initiate a session.  Every candidate
>    is a transport address.  It also has a type and a base.  Three types
>    are defined and gathered by this specification - host candidates,
>    server reflexive candidates, and relayed candidates.  The base of a
>    candidate is the candidate that an agent must send from when using
>    that candidate.
> 
>    The first step is to gather host candidates.  Host candidates are
>    obtained by binding to ports (typically ephemeral) on an interface
>    (physical or virtual, including VPN interfaces) on the host.  The
>    process for gathering host candidates depends on the transport
>    protocol.  Procedures are specified here for UDP.
> 
>    For each UDP media stream the agent wishes to use, the agent SHOULD
>    obtain a candidate for each component of the media stream on each
>    interface that the host has.  It obtains each candidate by binding to
>    a UDP port on the specific interface.  A host candidate (and indeed
>    every candidate) is always associated with a specific component for
>    which it is a candidate.  Each component has an ID assigned to it,
>    called the component ID.  For RTP-based media streams, the RTP itself
>    has a component ID of 1, and RTCP a component ID of 2.  If an agent
>    is using RTCP it MUST obtain a candidate for it.  If an agent is
>    using both RTP and RTCP, it would end up with 2*K host candidates if
>    an agent has K interfaces.
> 
>    The base for each host candidate is set to the candidate itself.
> 
>    Agents SHOULD obtain relayed candidates and MUST obtain server
>    reflexive candidates.  The requirement to obtain relayed candidates
>    is at SHOULD strength to allow for provider variation.  If they are
>    not used, it is RECOMMENDED that it be implemented and just disabled
>    through configuration, so that it can re-enabled through
>    configuration if conditions change in the future.
> 
>    The agent next pairs each host candidate with the STUN server with
> 
> Each candidate type should be set off with its own hed.

I can do that for host and server reflexive/relayed, but I can't split
the latter up because they are done together. I also broke out some of
the other into subsections - eliminating redundacies, computing
foundation, and keepalives.

> 
> Another example would be S 13:
> 
>    The foundation is composed of one or more ice-char.  The component-id
>    is a positive integer, which identifies the specific component for
>    which the transport address is a candidate.  It MUST start at 1 and
>    MUST increment by 1 for each component of a particular candidate.
>    The connect-address production is taken from RFC 4566 [10], allowing
>    for IPv4 addresses, IPv6 addresses and FQDNs.  The port production is
>    also taken from RFC 4566 [10].  The token production is taken from
>    RFC 3261 [3].  The transport production indicates the transport
>    protocol for the candidate.  This specification only defines UDP.
>    However, extensibility is provided to allow for future transport
>    protocols to be used with ICE, such as TCP or the Datagram Congestion
>    Control Protocol (DCCP) [29].
> 
> This defines the following productions:
> 
> - foundation
> - component-id
> - connect-address
> - port
> - token
> - transport
> 
> Each of these should be set off in its own bulleted list.

Done. I also broke out this section into subsections for each attribute.

> 
> 
> DETAILED COMMENTS
> S 2.
> There is a bad break around Figure 1.

Send a bug report to Marshall I guess; I have no control over that one.
xml2rfc does what it does.

> 
>    o  It's directly attached network interface (or interfaces in the
>       case of a multihomed machine
> s/It's/Its/

addressed by other comments already

> S 2.1.
> s/net10/net 10/

addressed by other comments already

> 
> 
>    the STUN server is configured, or learned in some way.)  When the
>                                 ^
>    the STUN server is manually configured or learned in some other
>    unspecified way.)  When the
> 

ok

> 
> The formatting around the Note is screwd up.

addressed by other comments already

> 
>    The final type of candidate is a RELAYED candidate.  The STUN Relay
> 
> s/RELAYED candidate/RELAYED CANDIDATE/

ok

> 
> 
>    Once L has gathered all of its candidates, it orders them highest to
> 
> s/them highest/them in highest/

ok

> 
> 
> S 2.4.
> You need to introduce the concept of foundation here.

added.

> 
> S 2.6.
> "controlling agent" and "controlled agent" should be capitalized on
> first use.

ahh ok, now I see you mean literally BOLD. Well, its a matter of style I
suppose. I've not done that in any other spec I've written to be frank,
but its not a bad idea. I've gone and done that through the spec.

> 
> Figure 4 has the third arrow in the wrong relation to its label.

fixed

> 
> You need diagrams of both the mode where the flag is sent after
> the initial ICE exchange and the aggressive one where it's sent
> with some message subset.

done.

> 
> I don't see what the term "favored" is bringing to the party here.
> In any case, "selected" might be a better state. 

I had it that way originally. Here's the problem. With this
'introspective' algorithm, you could end up where multiple checks
succeed with the flag. So then, ICE still needs to select one of those.
So, it didn't make sense to say something like, "The pair selected for
media is the highest priority one that was 'selected' by the controlling
agent by placing the flag in the request". So, I needed a term that
said, "I'd like to use this one very much", and then you actually use
the highest priority one that had that property. I originally chose 
favored. I changed it now to nominated which works better i think.

> 
> 
> S 2.7.
> I'm not sure that this section adds much to the overview.

Others commented that there wasn't enough here, and that the entire
appendix on lite should get moved in here. So, YMMV. Lite is a key
concept in ice so it does seem important to include it here.

> 
> S 3.
> The definition of "Foundation" seems less than helpful. I would
> rewrite:
> 
>    Foundation: An arbitrary string that is the same for two candidates
>       that have the same type, base IP address, and STUN server. 
>       If any of these are different then the foundation will be
>       different. Two candidate pairs with the same foundation
>       pairs are likely to have similar network characteristics.
>       Foundations are used in the frozen algorithm.
> 
> 

ok, sounds good.

> The way "check" is used to mean "candidate pair" seems confusing.
> A check is an action, not an object.

Yeah, its not consistent in fact. The document uses it to refer to a 
candidate pair and to the STUN transaction sent on that candidate pair. 
I think its more natural to use it to refer to the stun transaction. 
Then, I can just call the thing on the check list a candidate pair. I've 
made that change (there were TONS of instances of check to check). I 
think it cleared things up.

> 
> S 4.1.1.
> Each type of candidate should have its own subsection.

Mostly; I can't split relayed and server reflexive since they are 
descriebd jointly.

> 
>    has a component ID of 1, and RTCP a component ID of 2.  If an agent
>    is using RTCP it MUST obtain a candidate for it.  If an agent is
>    using both RTP and RTCP, it would end up with 2*K host candidates if
>    an agent has K interfaces.
> 
> Doesn't this 2*K rule apply to every kind of candidate?
yes.

> 
> Can a TURN server refuse to allocate a port? If so, does that
> mean you get denied the server-reflexive address as well?

Two interesting things here.

Firstly, TURN can refuse a port but it turns out that there is no error 
code for it. Looks like a bug in TURN. That needs to be fixed.

Assumings there, you are correct that the client would get denied its 
server reflexive. You'd have to fall back to the binding usage. I'll 
note that.

> 
> In the definition of foundation you should have a list of the
> criteria, not a para.

ok

> 
> S 4.1.2.
> At minimum the rules for each component of the priority formula
> should go in their own graf. Arguably under their own heds.

I made each its own paragraph.

> 
> 
>    These rules guarantee that there is a unique priority for each
>    candidate.  This priority will be used by ICE to determine the order
>    of the connectivity checks and the relative preference for
>    candidates.  Consequently, what follows are some guidelines for
>    selection of these values.
> 
> This section should go earlier.

right. done.

> 
> The region from "One criteria for selection of the type..."
> to the end of the section is informative. I wonder if it
> belongs here at all. It certainyl should be its own section.

I made it its own section. I do think it belongs here rather than an 
appendix; there are real normative statements in there.

> 
> 
>    important, the type preference for relayed candidates can be set
>    lower than the type preference for reflexive and host candidates.
>    Indeed, it is RECOMMENDED that in this case,
> 
> This can be compressed to be "SHOULD be set lower. The value
> of 126 is recommended..."
> 

compressed

> 
> S 4.1.3.
>    A candidate is said to be "in-use" if it appears in the m/c-line of
>    an offer or answer.  When communicating with an ICE peer, being in-
> 
> That should be a "a current offer or answer", right? If it appears
> in the m/c line of a previous o/a, then it's not necessarily in-use
> I would htink.
> 

this is cleaned up using the new default terminology

> 
>    each active media stream, to be in-use.  A media stream is active if
>    it does not contain the a=inactive SDP attribute.
> 
> A reference for a=inactive seems appropriate.

Its a standard SDP attribute in RFC 4566.

>    Unfortunately ... needed
> This seems superfluous.

removed

> 
> 
> Isn't the last graf redundant?

I merged them together.

> 
> 
> S 4.3.
> This whole section seems problematic. It's full of stuff that
> only makes sense if you have seen the SDP grammar yet. I 
> would replace it with an abstract discussion of the contents
> of a candidate in an offer or answer. This will provide enough
> context for the reader while avoiding the confusing forward
> references to the grammer.

done. I moved the meaty normative text in to the SDP grammar section. 
What remains here is overview and rules for setting other parts od the SDP.

> 
> I would also include an example with a brief explanation of
> how each value was being represented.

done

> 
> Why are types optional? When should I include them? Why wouldn't
> I?

These are SHOULD because ICE itself doesn't break if you don't include 
them. Thats my rule for SHOULD vs. MUST. The only place you run into 
trouble is if you are in one of these cable deployments and they need 
this information for access QoS. Hmm. Since things could not work as 
well if you omit it, why create choices when there is no reason not to? 
I changed to MUST.

> 
> 
> S 5.1.
> I find the first full graf (starting with "If...") very
> confusing. I would make the following changes to this
> section: 
> 
> - Make this list a numbered list.
> - Add the ice=lite criteria from the last para to the list.
> 
> Have the rest of the section be separate directions about
> what to do if each constraint is violated... I would make
> these separate grafs.

was able to combine it all together into a structure like:

proceed with ice if the following are all true:

* A
* B
* C

if any are not true then don't do ICE except for:

*X
*Y

> 
> 
> S 5.2.
>    Based on this definition, once roles are determined for a session,
>    they persist unless ICE is restarted, as discussed below.  A restart
>    causes a new selection of roles.
> 
> "discussed below" should point to somehting.

already done based on other comments

> 
> S 5.7.
>    There is one check list...
> 
> This graf should be moved to S 4 somewhere... It's a concept issue.

Definition of 'in-use' moved up to its first occurrence in section 4.

> 
> The remainder of this section is in dire need of heds.

broke up the various steps each into its own heading

> 
> You should have a state machine diagram for the check states.

OK. Not sure it says a whole lot since the FSM is trivial:

Please view in a fixed-width font such as
                 Courier.

    +-----------+
    |           |
    |           |
    |  Frozen   |
    |           |
    |           |
    +-----------+
          |
          |unfreeze
          |
          V
    +-----------+         +-----------+
    |           |         |           |
    |           | perform |           |
    |  Waiting  |-------->|In-Progress|
    |           |         |           |
    |           |         |           |
    +-----------+         +-----------+
                                / |
                              //  |
                            //    |
                          //      |
                         /        |
                       //         |
             failure //           |success
                   //             |
                  /               |
                //                |
              //                  |
            //                    |
           V                      V
    +-----------+         +-----------+
    |           |         |           |
    |           |         |           |
    |   Failed  |         | Succeeded |
    |           |         |           |
    |           |         |           |
    +-----------+         +-----------+

> 
> I'm not sure it helps to introduce states for a Check List. They're
> not used that often. I would just replace them with the relevant
> conditions inline in the text.

actually its used four separate times below; I think that is sufficient 
to warrant a definition.

> 
> 
> S 5.8.
>    When the timer fires, the agent MUST find the highest priority check
>    in that check list that is in the Waiting state.  The agent then
>    sends a STUN check from the local candidate of that check to the
>    remote candidate of that check.  The procedures for forming the STUN
>    request for this purpose are described in Section 7.1.1.  If none of
>    the checks in that check list are in the Waiting state, but there are
>    checks in the Frozen state, the highest priority check in the Frozen
>    state is moved into the Waiting state, and that check is performed.
>    When a check is performed, its state is set to In-Progress.  If there
>    are no checks in either the Waiting or Frozen state, then the timer
>    for that check list is stopped.
> 
> Breaking this into bullets would help readability.

ok, done

> 
>    Performing the connectivity check requires the agent to know the
>    username fragment for the local and remote candidates, and the
>    password for the remote candidate.  For periodic checks, the remote
>    username fragment and password are learned directly from the SDP
>    received from the peer, and the local username fragment is known by
>    the agent.
> 
> And for triggered checks they are...?

not treated in this section, which is about periodic checks. I clarified 
this by removing the qualifiers so its just statements of fact for the 
scope here.

> 
> 
> S 6.
> Most of these sections say "do what S 5 says". Why do we need
> this section at all?

Like S5.1 through S5.6, they need to be there since these are the steps 
that an agent follows on receipt of the offer. THe fact that they happen 
to match previous sections just allows me to backreference them.

> 
> 
> S 7.1.1.
>    A Binding Request ... a password of APASS.
> 
> This at least should be in a subsections since it's a detail
> and doesn't flow well with the rest of the section. I'd put it
> at the end of 7.1.1. too.

I broke this out into three subsections: (1) setting USE-CANDIDATE and 
PRIORITY, (2) forming credentials, (3) diffserv.
> 
> 
>    An agent MUST include the PRIORITY attribute in its Binding Request.
>    The attribute MUST be set equal to the priority that would be
>    assigned, based on the algorithm in Section 4.1.2, to a peer
>    reflexive candidate learned from this check.
> 
> peer reflexive candidates are never defined adequately outside
> the glossary. This is where they're really introduced, so you need
> a narrative explanation.

There is now text in the overview on them; enough I think to explain the 
idea. You really only need their definition to be defined so far, since 
the rules here are just a methodically re-application of the priority 
formula with a new value for one of the inputs. I tried to clarify a bit.

> 
> 
> S 7.1.2.
> I think it would pay off to explain how you correlate the STUN
> responses here.

its in 3489bis, but I added a sentence summarizing:

<t>
When a Binding Response is received, it is correlated to its Binding
Request using the transaction ID, as defined in <xref
target="I-D.ietf-behave-rfc3489bis"/>, which then ties it to the
candidate pair for which the Binding Request was sent.
</t>

> 
> Each of the cases described here needs to be broken out in its
> own subsection.

done.

> 
>    The agent MUST check that the source IP address and port of the
>    response equals the destination IP address and port that the Binding
>    Request was sent to, and that the destination IP address and port of
>    the response match the source IP address and port that the Binding
>    Request was sent from.  If these do not match, the processing
>    described in the remainder of this section MUST NOT be performed.  In
>    addition, an agent sets the state of the check to Failed.
> 
>    If the check succeeds, processing continues.  The agent creates a
>    candidate pair whose local candidate equals the mapped address of the
>    response, and whose remote candidate equals the destination address
>    to which the request was sent.  This is called a validated pair,
>    since it has been validated by a STUN connectivity check.  It is very
>    important to note that this validated pair will often not be
>    identical to the check itself; in many cases, the local candidate
>    (learned through the mapped address in the response) will be
>    different than the local candidate the request was sent from.
> 
> OK. This is a really important section but it just sort of flies
> by. The key points here are:
> 
> 1. The transport addresses in the UDP must match those you expect.
> 2. The XOR-MAPPED-ADDRESS attribute may not match.
> 
> I find this bit about creating a new candidate pair very confusing, since in
> at least *some* cases this candidate pair will match the one you're
> doing the check on. Is the notion here that you're creating a new
> pair and then putting it on the valid list, so you need a copy even
> if it's the same? If so, that seems unnecessarily prescriptive about
> implementation. An alternative would be to not maintain a separate valid list and
> just mark these checks with their own state....
> 
> Am I missing something?

In most cases, the resulting pair won't exist in the check list. Recall 
that the check list has local candidates which are always *bases* - 
typically host candidates. After the stun check, what comes back in the 
xor-mapped-address will be a reflexive candidate if there is a nat 
between the two. Since none of the pairs in the check list have bases 
that are server reflexive, it'll be new.

I reworked this text a bunch to try and explain this:

<t>
It will be very common that the pair will not be on any check list.
Recall that the check list has pairs whose local candidates are never
server reflexive; those pairs had their local candidates converted to
the base of the server reflexive candidates, and then pruned if they
were redundant. When the response to the STUN check arrives, the
mapped address will be reflexive if there is a NAT between the two. In
that case, the valid pair will have a local candidate that doesn't
match any of the pairs in the check list.
</t>

> 
> 
>    Once the priority of the candidate pair has been computed, the pair
>    is added to the valid list for that media stream.  If the agent was a
>    controlling agent, and the check had included a USE-CANDIDATE
>    attribute, the candidate pair is marked as "favored".  If the agent
>    was a controlled agent, and the check was a triggered check, and the
>    request which caused the triggered check included the USE-CANDIDATE
>    attribute, the candidate pair is marked as "favored".
> 
> Isn't there a race condition here if the periodic check for the candidate
> precedes the triggered check?

Yes. It is resolved with text in the section on processing of received 
requests. It was one of the hardest pieces of text to follow I think. 
I've converted it to bullet text.

> 
> 
>    PRIORITY attribute of the request.  Its foundation is selected as
>    described in Section 4.1.1.  The peer reflexive candidate is then
>    added to the list of local candidates known by the agent (though it
>    is not paired with other remote candidates at this time).
> 
> When is it paired with other remote candidates?

Never unless you really want to (whether to do it or not was a big open 
issue for a while). This was not sufficiently well explained. Now says:

<t>
This peer reflexive candidate is then added to the list of local
candidates for the media stream. Its username fragment and password
are the same as all other local candidates for that media
stream. However, the peer reflexive candidate is not paired with other
remote candidates. This is not necessary; a valid pair will
be generated from it momentarily based on the procedures in <xref
target="sec-valid-cons"/>. If an agent wishes to pair the peer
reflexive candidate with other remote candidates besides the one in
the valid pair that will be generated, the agent MAY generate an
updated offer which includes the peer reflexive candidate. This will
cause it to be paired with all other remote candidates.
</t>

> 
> 
>    unfrozen.  If the check had a component ID of one, the agent MUST
> 
> please be consistent about whether you use "one" or "1" here.

changed to 1 throughout

> 
> Also, please break out the cases in this para....

done.

> 
> 
> S 7.2.1.
> This section needs heds again...

done

> 
> Why would something be encapsulated in a Data Indication? If it's relayed,
> right? Please explain.

Yes, if relayed and if the turn server doesn't have an active 
destination. Added.

> 
> What's a tentative check? I think it would help to explain upfront
> why this is tentative. Maybe something to the effect of 
> "In some cases, the receipt of a check requires that the implementation
> send an immediate "triggered check" in the remote direction..."

I was able to nuke this entire concept of tentative; fixing the 
confusion around 'check' cleared it up.

> 
> The list of cases here is flat but the first four cases are one
> branch and the last case is a separate branch. Some hierarchy
> here would help.
added

> 
>    If the tentative check is to be performed, it is constructed and
>    processed as described in Section 7.1.1.
> 
> But sent immediately, right?

yes - this is clearer now that I was able to ditch the tentative check 
terminology

> 
> 
> S 8.
> More heds needed again...

done.

> 
> I don't think the terms "introspective selection" and "proactive
> selection" add much value here. It's not like they're used a
> lot later. If you add heds you can refer to the section numbers
> and give them sort of informal names...

actually they are quite important and aren't given enough treatment. I 
renamed to 'aggressive nomination' and 'regular nomination' which is 
less arcane and more descriptive. There is now a subsection on each 
type. They are referenced in a few places including now the overview.

> 
> You should have a diagram of each of the cases here (or have
> it in the intro...)

now in intro

also i bulletized some more rules here.

> 
> S 9.1.
>    An agent MAY change the ice-pwd and/or ice-ufrag for a media stream
>    in an offer.  Doing so is a signal to restart ICE processing for that
>    media stream.  When an agent restarts ICE for a media stream, it MUST
>    NOT include the a=remote-candidates attribute, since the state of the
> 
> Why would it occur to me to include this attribute since this is the
> first the reader has heard of it?
> 
> 
> I'm trying to wrap my head around the cases here. It seems to me 
> that you would do some kind of ICE restart if:
> 
> 1. There is a new media session.

well, thats more of a START and not a RESTART, but yeah

> 2. One of the media sessions has changed, right?

its destination has changed

> 3. You want to change your implementation level so you do a full
>    restart.

yes

> 	 
> Maybe you should enumerate these?

Good point. I did.

> 
> So the bit about not setting c=0.0.0.0 is sort of a parenthetical,
> right? Maybe make it a note?

There is a normative bit here, but its separable so its at the bottom 
underneath the enumeration.

> S 9.1.1.
> Need heds again.

Indeed, more than that.

I refactored this entire discussion. Rather than splitting up paragraphs 
based on attribute, i split this up into subsections based on cases. SO 
the cases are now:

  9.  Subsequent Offer/Answer Exchanges  . . . . . . . . . . . . . . 44
      9.1.  Generating the Offer . . . . . . . . . . . . . . . . . . . 44
        9.1.1.  Procedures for All Implementations . . . . . . . . . . 44
          9.1.1.1.  ICE Restarts . . . . . . . . . . . . . . . . . . . 44
          9.1.1.2.  Removing a Media Stream  . . . . . . . . . . . . . 45
          9.1.1.3.  Adding a Media Stream  . . . . . . . . . . . . . . 45
        9.1.2.  Procedures for Full Implementations  . . . . . . . . . 45
          9.1.2.1.  Existing Media Streams with ICE Running  . . . . . 45
          9.1.2.2.  Existing Media Streams with ICE Completed  . . . . 46
        9.1.3.  Procedures for Lite Implementations  . . . . . . . . . 46

so if you are adding a media stream you go right to section 9.1.1.3 and 
it tells you how to set all of the ice parameters. I think this flows 
much, much better now.

> 
>    If a candidate was sent in a previous offer/answer exchange, it
>    SHOULD have the same priority.  For a peer reflexive candidate, the
>    priority SHOULD be the same as determined by the processing in
>    Section 7.1.2.  The foundation SHOULD be the same.  The username
>    fragments and passwords for a media stream SHOULD remain the same as
>    the previous offer or answer.
> 
> Is the basic concept here that you should replicate previous offers?
> Maybe say that first....

Does now:

<section title="Existing Media Streams with ICE Running">

<t>
If an agent generates an updated offer including media stream that was
previously established, and for which ICE checks are in the Running
state, the agent follows the procedures defined here.
</t>

<t>An agent MUST include candidate attributes for all local candidates
it had signaled previously for that media stream.  The properties of
that candidate as signaled in SDP - the priority, foundation,
priority, type and related transport address SHOULD remain the
same. The IP address, port and transport protocol, which fundamentally
identify that candidate, MUST remain the same (if they change, it
would be a new candidate). The component ID MUST remain the same. The
agent MAY include additional candidates it did not offer previously,
but which it has gathered since the last offer/answer exchange,
including peer reflexive candidates.
</t>

I think that is much clearer.

> 
> Again, what does the remote-candidates attribute *do*...? Can
> you explain the concept somewhere before it's introduced?

The refactoring helped this, in that now its only mentioned in the 
section on sending an updated offer when ice is completed. Its there to 
resolve a really annoying and somewhat unlikely race, but a race 
nonetheless. Appendix B.7 spells it out. I'll add a bit more descriptive 
text here and then point to that appendix.

> 
> 
> S 9.2.
>    If the offer contained a change in the a=ice-ufrag or a=ice-pwd
>    attributes compared to the previous SDP from the peer, it is a signal
>    that ICE is restarting for this media stream.
> 
> "a signal"? Is it the only signal? Is this iff?

Refactoring fixed it. ice restart implies change in both ufrag and pass, 
and vice a versa.

> 
> 
> Can you reduce the last graf to "do whatever the offerer would do..."?

more or less, yes.

> 
> 
> S 9.3.
>    If ICE is restarting for a media stream, the agent MUST start a new
>    Valid list for that media stream.  However, it retains the old Valid
>    list for the purposes of sending media until ICE processing
>    completes, at which point the old Valid list is discarded and the new
>    one is utilized to determine media and keepalive targets.
> 
> I'm trying to figure out what the "Valid list" is doing here.
> Isn't what I need to remember the address pairs I'm sending media
> on, not the address pairs I might have sent on but decide not
> to? The valid list seems like too-much information...

Yes, it is. Removed this.

> 
> 
> S 9.3.1.
> 
> The various subcases need to be broken out with bullets or heds
> here...

done.

> 
> 
> S 10.
>    All endpoints MUST send keepalives for each media session.
> 
> What is the scope of this mandate? Does this update all RTP stacks
> whether or not they implement ICE? Is HTTP media?

It refers to ICE implementations, though the spec does say its a good 
idea even if you are not iCE.

> 
> 
>    used [11].  The Binding Indication SHOULD NOT contain integrity
>    checks; since the messages are simply discarded on receipt regardless
>    of contents.
> 
> s/; since/as/

ok

> 
> 
>    media.  An agent receipt a Binding Indication MUST discard it
> s/receipt/receiving/

ok

> 
> S 11.1.1.
> 
>    If the state of a media stream is Running, there is no old Valid list
> 
> s/,/ and/ ?
> 
> 

I cleaned this up by making it bullet list conditions

> S 12.
> I wonder if this could all be made an appendix...? It seems kind of orthogonal
> to ICE per-se.

well, its pretty important stuff and normative. so i think it needs to 
be here.

> 
> S 13.
> Each production here needs at least its own graf and should be set off
> in the text.
> 
> E.g.,
> 
>     foundation -
>       The foundation is composed of...
> 
> Each attribute should probably hve its own section.

done.

> 
> 
> S 14.
>    Firstly, ICE provides the a=ice-options SDP attribute.  Each
> s/Firstly/First/

ok

> 
> 
> S 15.
> This example is fairly monolithic and hard to parse. It would be
> better if the ladder diagram were broken up into pieces
> (and maybe scattered throughout the text). Then you can
> present a complete ladder diagram as a worked example.

I don't think its so long that it requires this. We've never done this 
for other specs that have even longer flows. It does require you to flip 
back and forth between the ladder diagram and text, but this is no big 
deal and not a readibility problem.

> 
> 
>    Agents L and R both pair up the candidates.  They both initially have
>    two. 
> 
> two pairs, right?

right, fixed

> 
>    However, agent L will prune the pair containing its server
>    reflexive candidate, resulting in just one.  At agent L, this pair
>    (the check) has a local candidate of $L_PRIV_1 and remote candidate
>    of $R_PUB_1, and has a candidate pair priority of 4.57566E+18 (note
>    that an implementation would represent this as a 64 bit integer so as
>    not to lose precision).  At agent R, there are two checks.  The
>    highest priority has a local candidate of $R_PUB_1 and remote
>    candidate of $L_PRIV_1 and has a priority of 4.57566E+18, and the
>    second has a local candidate of $R_PUB_1 and remote candidate of
>    $NAT_PUB_1 and priority 3.63891E+18.
> 
> This seems like listing each side's perspective would help.

It is listing each sides perspective; not sure what you mean.

> Also, I would
> render the priorities in hex or decimal, not scientific notation,
> so it's easy to see what's going on....

lol - well - the reason its scientific is that my calculator won't spit 
out numbers that large without scientific notation....

> 
> 
> S 16.2.
>    force a false valid on a false candidate.  This attack is very hard
>    to launch if the false address identifies a third party, and is
>    prevented by SRTP if it identifies the attacker themself.
> 
> I don't understand this claim.

clarified

> 
> S B.4.
> The point that the username/password is a disambiguator should be 
> made in the main text.

added

> 
> S B.6.
> This section seems redundant to me.

yeah, i think we've put a lot of it into the overview now. removed.

> 
> S B.7.
> Some of this should be promoted to the main text along with
> an example of how it works.

added some motivational text up front, but i dont want to move the whole 
thing up. you don't need to understand this to implement it. Thats why 
its in an appendix.

Thanks!

-Jonathan R.

-- 
Jonathan D. Rosenberg, Ph.D.                   600 Lanidex Plaza
Cisco Fellow                                   Parsippany, NJ 07054-2711
Cisco Systems
[email-masked]                              FAX:   (973) 952-5050
http://www.jdrosen.net                         PHONE: (973) 952-5000
http://www.cisco.com

_______________________________________________
mmusic mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/mmusic