Subject: [Ice] Taylor's review of draft-ietf-ice-trickle-08.txt

From: Taylor Brandstetter <[email-masked]>
Date: Thu, 30 Mar 2017 22:28:42 -0700

I reviewed Trickle ICE, and overall I think it's in pretty good shape. The
main remaining areas I believe could be improved are:

   1. Using terminology that's too SDP-specific. This has been improved a
   lot since I last reviewed, but in my opinion it could still be made a bit
   better. If it used the same sort of terminology as ICEbis ("convey the ICE
   parameters" as opposed to "send an ICE description"), I'd be happy. But I
   may be nitpicking too much.
   2. The unfreezing logic still doesn't seem perfect. If there are people
   who still feel it's important, I'd be glad to explain the issues as I see
   them further and work on fixing them. Though, I'm fine with saying "what we
   have is good enough." The current unfreezing logic at least now clearly
   ensures that one candidate per foundation is always unfrozen.

Here are the individual issues I found:

   - In the terminology section:
      - The definition of "ICE description" refers to SDP attributes like
      "ice-ufrag" by name. This should be changed to just say
"username fragment"
      (for instance) and not rely on SDP terminology.
      - Also, in my opinion, using the terms "description," "session" and
      even "attribute" ties this too much to SDP. ICEbis uses the phrase
      "candidate information" to refer to the collection of information that's
      signaled; can Trickle ICE refer to this, and use similar terminology?
      ICEbis also says "parameter" rather than "attribute".
      - Related to the above: the concept of "sending an ICE description,"
      used in many places, sounds too much like "sending an SDP offer" to me.
      ICEbis uses terminology like "the following parameters and their data
      types need to be conveyed as a part of the candidate exchange process,"
      which is much less prescriptive of an offer/answer or message-based
      signaling protocol. Again; could Trickle ICE use similar terminology?
   - In section 5.2: "With regard to pruning of duplicate candidate pairs,
   a Trickle ICE agent SHOULD follow a policy of pruning the lower priority
   candidate unless it is peer reflexive." This makes it sound like peer
   reflexive candidates are *never* pruned, but it's the opposite; they're
   *always* pruned, as described in Section 8.1. Also, in ICEbis,
   candidates themselves aren't pruned, only candidate pairs are.
   - In section 7.2: "Therefore a Trickle ICE agent MUST monitor whether a
   check list is active or frozen independently of the state of the candidate
   pairs in the check list (since there might not be any pairs yet), and MUST
   consider a check list to be active whenever it adds the first candidate
   pair with a state of Waiting to the check list (see Section 8.1.1)." The
   first MUST seems redundant with the second. In earlier drafts, the purpose
   of monitoring the "active/frozen" state independently was so that if a
   check list is empty and frozen, the first pair added starts as "Frozen",
   while if it's empty and active, the first pair added starts as "Waiting."
   If the first pair *always* starts as "Waiting," then tracking the
   active/frozen state of a check list independently is no longer necessary.
   Though I don't see anything in Section 8.1.1 that relates specifically to
   this, so I'm not sure what "see Section 8.1.1" is referring to.
   - In section 8.1.1: In general I like the improvements that have been
   made. However:
      - The rules for how a new pair gets either a "Waiting" or "Frozen"
      state don't completely match standard ICE; at least the part
where when one
      "media stream" completes, pairs from other media streams with matching
      foundations are unfrozen. With the current rules in section
8.1.1, only the
      topmost remaining pair in each foundation is guaranteed to be
unfrozen. Is
      this difference acceptable? If it is, we should at least acknowledge it,
      and not give the wrong impression by saying "Trickle ICE preserves all of
      [the standard ICE] rules."
      - The rule that a pair changes to "Waiting" if a pair in a lower row
      is "Succeeded" means that if a pair in the lowermost row is
unfrozen first
      (which is possible, if the signaling mechanism doesn't preserve
ordering of
      candidates), all of the other pairs with its foundation would be unfrozen
      as soon as it's unfrozen. This seems odd.
   - As mentioned in a different email thread, Section 16 doesn't make
   sense any more if the signaling protocol isn't required to preserve
   candidate ordering.

I also found a couple of really trivial things, which I made a PR for:
https://github.com/ice-wg/trickle/pull/10

---

From: Taylor Brandstetter <[email-masked]>
Date: Thu, 30 Mar 2017 22:35:15 -0700

Minor clarification: when I said "all of the other pairs with [the
lowermost pair's] foundation would be unfrozen as soon as it's unfrozen",
this would only occur if the other pairs are formed after the lowermost
pair succeeds. Which makes this more of a corner case.

On Thu, Mar 30, 2017 at 10:28 PM, Taylor Brandstetter <[email-masked]>
wrote:

>    2. The unfreezing logic still doesn't seem perfect. If there are
>    people who still feel it's important, I'd be glad to explain the issues as
>    I see them further and work on fixing them. Though, I'm fine with saying
>    "what we have is good enough." The current unfreezing logic at least now
>    clearly ensures that one candidate per foundation is always unfrozen.
>
>       "ice-ufrag" by name. This should be changed to just say "username fragment"
>       ICEbis uses terminology like "the following parameters and their
>       data types need to be conveyed as a part of the candidate exchange
>       process," which is much less prescriptive of an offer/answer or
>       message-based signaling protocol. Again; could Trickle ICE use similar
>       terminology?
>    - In section 5.2: "With regard to pruning of duplicate candidate
>    pairs, a Trickle ICE agent SHOULD follow a policy of pruning the lower
>    priority candidate unless it is peer reflexive." This makes it sound like
>    peer reflexive candidates are *never* pruned, but it's the opposite;
>    they're *always* pruned, as described in Section 8.1. Also, in ICEbis,
>    - In section 7.2: "Therefore a Trickle ICE agent MUST monitor whether
>    a check list is active or frozen independently of the state of the
>    candidate pairs in the check list (since there might not be any pairs yet),
>    and MUST consider a check list to be active whenever it adds the first
>    candidate pair with a state of Waiting to the check list (see Section
>    8.1.1)." The first MUST seems redundant with the second. In earlier drafts,
>    the purpose of monitoring the "active/frozen" state independently was so
>    that if a check list is empty and frozen, the first pair added starts as
>    "Frozen", while if it's empty and active, the first pair added starts as
>    "Waiting." If the first pair *always* starts as "Waiting," then
>    tracking the active/frozen state of a check list independently is no longer
>    necessary. Though I don't see anything in Section 8.1.1 that relates
>    specifically to this, so I'm not sure what "see Section 8.1.1" is referring
>    to.
>       state don't completely match standard ICE; at least the part where when one
>       foundations are unfrozen. With the current rules in section 8.1.1, only the
>       topmost remaining pair in each foundation is guaranteed to be unfrozen. Is
>       - The rule that a pair changes to "Waiting" if a pair in a lower
>       row is "Succeeded" means that if a pair in the lowermost row is unfrozen
>       first (which is possible, if the signaling mechanism doesn't preserve
>       ordering of candidates), all of the other pairs with its foundation would
>       be unfrozen as soon as it's unfrozen. This seems odd.