Subject: Re: [Ice] Comments on draft-ietf-ice-trickle-04 (Taylor)

From: Peter Saint-Andre <[email-masked]>
Date: Wed, 4 Jan 2017 20:04:32 -0700

Hi Taylor, Emil and I are working through all feedback now and might 
have a few more comments beyond those which you can find inline.

On 10/19/16 12:53 PM, Taylor Brandstetter wrote:
> I reviewed draft-ietf-ice-trickle-04 and noted various
> comments/questions as I went along. Looking back, many of these are the
> same things Christer mentioned a week ago, or are purely editorial. But
> my major takeaways were:
>
> 1. Many sections need to be changed in order to not directly prescribe
> the SDP offer/answer model. Though I feel it's fine to use SDP
> offer/answer in examples.

That approach will be reflected in -05.

> 2. There are still some problems with the freezing/unfreezing rules as I
> understand them.
>
> Anyway, here are my comments, grouped by section:
>
> *Section 2 - Terminology*
>
>   * Here and throughout the document, UPnP based port allocation and
>     XMPP Jingle Relay Nodes are mentioned, which not even ICEbis
>     references. Should these be removed?

Yes.

>   * The concept of a “generation of candidates” is not defined anywhere,
>     even though I intuitively know what it means.

Subject to discussion with my co-authors, I've provisionally changed 
this to "batch".

The term "generation" is used in a different sense within the original 
documentation of the trickle concept (XEP-0176), so I'd prefer not to 
use it here.

> I recommend defining
>     it in ICEbis. The phrase “ICE negotiation session” is also used
>     later, referring to (I think) the same thing.

My understanding of those terms is that they're not referring to the 
same thing.

>   * It’s stated that Trickle ICE is not defined “in terms of SDP or the
>     offer/answer model”. But it is, throughout the entire document. The
>     document needs to be changed to refer to “candidate exchanges”, for
>     example, instead of offers/answers.

Yes, those changes are underway, and Emil and I will smooth over any 
rough edges that result.

> *Section 4 - Sending the Initial Offer*
>
>   * “An agent starts gathering candidates as soon as it has an
>     indication that communication is imminent”. This isn’t always true.
>     There are even examples later of the opposite happening (candidates
>     being gathered well in advance). Maybe just say “an agent
>     *generally* starts gathering…”

I like "An agent can start gathering candidates as soon..." It's really 
an application decision.

>   * “A host candidate at a media relay”. Is that really a host
>     candidate? Where is this described in ICEbis?

I suggest we remove "host" here.

>   * There is discussion around hiding host candidates for privacy
>     implications. This isn’t specific to Trickle ICE though, so it
>     should go in ICEbis or somewhere else.

Perhaps. It was just a side-comment here, and in any case (per comments 
by Bernard Aboba) we will probably remove that paragraph.

>   * “Methods for calculating priorities and foundations, as well as
>     determining redundancy of candidates, work just as with ICEbis.” Not
>     the redundancy part, since there are the special prflx rules.

Good catch.

> *Section 5.2 - Forming Check Lists and Beginning Connectivity Checks*
>
>   * There is talk about unfreezing candidates. But the concept of
>     freezing a candidate doesn't make sense to me; only pairs are
>     unfrozen. Is this meant to say “candidate pairs”? I actually notice
>     the same problem in ICEbis.

Will fix.

>   * The "first checklist with a candidate pair wins" unfreezing rule
>     means that the initial unfrozen checklist could be different between
>     the initiating and responding agents due to racy trickling, leading
>     to at least one of the checklists failing. Suppose for example that
>     checklist 1 has only pairs with foundation A and checklist 2 has
>     only pairs with foundation B; there's no guarantee on ordering
>     between foundations. Why not just "first checklist starts as active,
>     all others frozen"?

I'm not completely clear on the implications of that change and will 
discuss it with my co-authors.

>   * “With regard to pruning of duplicate candidate pairs, a Trickle ICE
>     agent SHOULD follow a policy of ‘first one wins’.” Is this out of
>     date? In “Pairing Newly Learned Candidates and Updating Check Lists”
>     there are rules for handling duplicate candidates. There are other
>     places where this “first one wins” policy is mentioned that should
>     be updated.

Yes, that was out of date. Will be fixed in -05.

>   * The concepts of “active” and “frozen” check lists are used here, but
>     the definitions are only elaborated upon in “Check List and Timer
>     State Updates”.

Actually those descriptions (not states) are described in Section 
5.1.3.1 of rfc5245bis, so we could just point there.

> *Section 7.2 - Check List and Timer State Updates*
>
>   * The definitions of active/frozen are a SHOULD rather than a MUST. If
>     the unfreezing algorithm is vital, as stated elsewhere, shouldn’t it
>     be a MUST?

I'm fine with MUST.

>   * “A Trickle ICE agent SHOULD consider a check list to be active …
>     when the check list is empty.” Wouldn’t that mean that everything
>     begins as active, in the common case where you start with 0 pairs? I
>     think the intent here is that an empty check list may be considered
>     active or frozen, but that’s not exactly what’s written. Also, if
>     that *is* the intent, the specification should explain exactly when
>     an empty frozen checklist becomes an empty active checklist. I
>     believe those conditions are:
>       o Another checklist is completely finished (every pair Successful
>         or Failed).
>       o Another checklist has a valid pair for all components.

That seems reasonable, and the authors will discuss to see if we agree.

> *Section 8 - Discovering and Sending Additional Local Candidates*
>
>   * In my opinion, the rule that you MUST NOT pair a local candidate
>     until it's been trickled is not clear enough. Though it's heavily
>     implied.

Yes, we need to make that explicit.

>   * The rule that you must trickle candidates in the order of
>     components, to protect the freezing algorithm, is not enough. I
>     believe you'd also need to trickle them in order of media stream and
>     priority since this also affects which pair is initially unfrozen.

It seems that we've modified this text a bit in our working version of 
-05 and that it might no longer be in this section (or in the document 
at all). We'll track this down before posting -05.

> *Section 8.1 - Pairing Newly Learned Candidates and Updating Check Lists*
>
>   * If a checklist is active, the new pair is set to “Waiting” if it’s
>     the first pair in that list with that foundation. This puts pairs in
>     the Waiting state much more aggressively than ICEbis; if a pair for
>     the first media stream/component is still in the Waiting state, it
>     will basically be ignored when pairs with that foundation are added
>     for other media streams, and they'll all end up as "Waiting". Should
>     we effectively replace “if no pair in this checklist has a matching
>     foundation” with “if no pair in *any* checklist has a matching
>     foundation”?

To be discussed among the authors. We'll post further.

>   * Even with the above change, the unfreezing rules don’t exactly match
>     ICEbis. So a race between candidate signaling and connectivity
>     checks could cause a pair to be Frozen on one side and Waiting on
>     the other. Are we ok with this?

That sounds sub-optimal to me.

> *Section 8.2 - Announcing End of Candidates*
>
>   * “When an end-of-candidates indication is sent as part of an offer of
>     an answer, it can be considered to apply to the session as a whole,
>     which is equivalent to having it apply to all media streams.” Again,
>     shouldn’t be getting into offer/answer semantics in this document.
>     But even so, this is incorrect. “a=end-of-candidates” is media-level.

Yes, I think we will delete that paragraph before we submit -05.

> *Section 11 - Trickle ICE and Peer Reflexive Candidates*
>
>   * This section doesn’t seem to take into account the recent changes,
>     where peer reflexive candidates can be replaced by server reflexive
>     candidates.

I think we've subsequently fixed that, but we'll double-check.

> *Section 18 - Acknowledgements*
>
>   * I don’t think I deserve my own paragraph, I hardly did anything. :)

But now you've done more! ;-)

Peter

---

From: Emil Ivov <[email-masked]>
Date: Thu, 5 Jan 2017 12:26:28 -0600

On Wed, Jan 4, 2017 at 9:04 PM, Peter Saint-Andre <[email-masked]> wrote:
> Hi Taylor, Emil and I are working through all feedback now and might have a
> few more comments beyond those which you can find inline.
>

I don't see why. Those are informational references and provide good
examples of things that take time during gathering. Is there any
problem with having them?

> Subject to discussion with my co-authors, I've provisionally changed this to
> "batch".
>
> documentation of the trickle concept (XEP-0176), so I'd prefer not to use it
> here.

I believe it was the same concept. I find batch more confusing here
since it could also mean any subset of candidates in a single
generation. I think it would be easier to just define generation as
the full set of candidates used by one agent during a full ICE
negotiation session

> My understanding of those terms is that they're not referring to the same
> thing.

Agreed.

> Yes, those changes are underway, and Emil and I will smooth over any rough
> edges that result.
>
> I like "An agent can start gathering candidates as soon..." It's really an
> application decision.
>
> Perhaps. It was just a side-comment here, and in any case (per comments by
> Bernard Aboba) we will probably remove that paragraph.
>
> I'm not completely clear on the implications of that change and will discuss
> it with my co-authors.

What we discussed in Berlin was that once you unfreeze "the first
one", anything that comes in the list before the unfrozen pair, will
automatically be unfrozen too.

This actually removes the importance of "trickling in order" and makes
it a mere optimization. We have moved this to an annex and we have
sort of clarified the pairing section but at this point I am beginning
to think that this entire part needs a full rewrite, possible
explaining it more around foundations and the tables we discussed in
Berlin.

> Actually those descriptions (not states) are described in Section 5.1.3.1 of
> rfc5245bis, so we could just point there.

We have a trickle specific definition of checklist states:

https://tools.ietf.org/html/draft-ietf-mmusic-trickle-ice-02#section-8.1

>>     the unfreezing algorithm is vital, as stated elsewhere, shouldn’t it

Agreed

>>     begins as active,

Where exactly are you reading this? I can't find the exact wording but
the idea was simply to convey that a checklist can be both "Active"
and empty with trickle ICE (contrary to vanilla ice).

> in the common case where you start with 0 pairs?

Right, this wasn't the intent.

> I

I think the spec does explain that already at the bottom of 8.1. I
would slightly modify the last sentence to fix the part with "when
that couldn't happen" because that's not currently super clear.

> Yes, we need to make that explicit.

I don't see why this is a MUST or even a SHOULD. There are examples of
apps that never trickle any of their local candidates and rely on
their peer to discover them as peer-reflexive.

> It seems that we've modified this text a bit in our working version of -05
> and that it might no longer be in this section (or in the document at all).
> We'll track this down before posting -05.

Yup. Gotten rid of that part as explained above.

> To be discussed among the authors. We'll post further.

Yes, that is the section that I meant was currently very bad, and that
we'll probably end up rewriting.

>>     the other. Are we ok with this?

I think the race is eliminated as long as you say that, for every
foundation that has an unfrozen pair, anything that comes in before
that pair is also automatically unfrozen.

I was actually thinking of asking ICE bis to introduce foundation
freeze states but we could also get away with only doing this in
trickle.

>>     But even so, this is incorrect. “a=end-of-candidates” is media-level.

The intent was to also allow use of end-of-candidates as a session
level attribute but I see now that JSEP doesn't go through that
effort, so I guess we could dump this indeed.

Cheers,
Emil

> _______________________________________________
> Ice mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ice

-- 
https://jitsi.org

---

From: Peter Saint-Andre <[email-masked]>
Date: Thu, 5 Jan 2017 16:06:00 -0700

Comments inline, areas of agreement elided.

> problem with having them?

It's not a hill for me to die on. :-)

> the full set of candidates used by one agent during a full ICE

In XEP-0176, a "generation" consists of all the candidates that are sent 
before an ICE restart. Here, a "generation" seems to be the first 
set/batch/group of candidates that are sent (which can be followed by 
more candidates that are trickled over time). I'm happy to work with you 
on better text here, though!

>> My understanding of those terms is that they're not referring to the same

The term "ICE negotiation session" seems reasonable to me. We need to 
make it clear that such a session ends with an ICE restart (but that 
some information can be assumed true across sessions, e.g., whether the 
parties support trickle).

> explaining it more around foundations and the tables we discussed in

That sounds good. You and I can work on that together and propose some 
text to the WG.

> that couldn't happen" because that's not currently super clear.

+1

> their peer to discover them as peer-reflexive.

I think Taylor was referring to pairing, not trickling. If you decide 
not to trickle, there's never a need to pair, right?

> we'll probably end up rewriting.

+1

> that pair is also automatically unfrozen.

I think that would work.

> freeze states but we could also get away with only doing this in

It does seem to apply more directly to trickle.

Agreed.

Peter