Re: [MMUSIC] Review of draft-ietf-mmusic-trickle-ice-02

[Peter Saint-Andre - &yet <[e-mail]>]

On 9/3/15 1:36 PM, Brandon Williams wrote:
> I've reviewed draft-ietf-mmusic-trickle-ice-02. Sorry for the delay
> since Prague.
>
> First some nits, mostly in the introduction.
>
> s 1, para 1
> There's an extra comma I think.
> s/gathering, candidates,/gathering candidates,/

That will be fixed in the next version.

> s 1, para 2 and elsewhere
> Does the term "candidate harvesting" come from somewhere specific? I
> don't find it in RFC5245. I would stick "candidate gathering" throughout
> for consistency with 5245 unless you mean something distinctly different
> by the term, in which case a definition is called for in section 2.

I agree that using "candidate gathering" and "candidate gatherer" would 
be better.

> s 1, para 2
> "Gathering candidates ... would often involve". I suggest just
> "Gathering candidates ... often involves"

I plan to complete a copy edit before we publish the next version.

> s 1
> The intro could benefit from a specific example of the lengthy session
> establishment time mentioned in para 3.

We'll look into adding an example.

> s1, para 4
> It's just '... known as "trickle ICE"' I think, not 'also known as'.

Agreed.

> s1, para 5
> trickle ICE allows candidates of any type to be exchanged as soon as
> they've been discovered, right? Not just "exchange host candidates as
> soon as a session has been initiated".

Correct.

> s1, para 7
> "needs to be" and "should operate" make the paragraph a little harder to
> read. I suggest "... how support for trickle ICE is discovered, how
> checklists are built and updated with trickle ICE, and how trickle ICE
> implementations interoperate with ..."

See above about the copy edit. :-)

> s 4, para 1
> Seems to indicate that RFC5245 describes what a trickle ICE agent should
> do. Needs re-wording. I think you mean that there's a requirement in
> 5245 that leads to the requirement stated here.

I'm suggesting that we change this to:

    According to [RFC5245], supported features are to be advertised in
    the ice-options attribute.  Therefore an agent supporting trickle ICE
    MUST include a token of "trickle" in the ice-options attribute every
    time it generates an offer or an answer.

> s 4.1
> s/relay candidates/relayed candidates/

Will fix.

> s 9.3
> There's a redundant sentence in the first paragraph on page 16.
> "Sending the indication is necessary in order to avoid ambiguities and
> speed up ICE conclusion. This is necessary in order to avoid
> ambiguities and speed up ICE conclusion."

Will fix.

> Now for some (hopefully) more substantive comments.
>
> s 3
> I think the section on "Incompatibility with Standard ICE" comes too
> early. It's difficult to process because the doc has not yet introduced
> the details of how trickle ICE works. Also in this section, do you
> really mean to indicate that trickle ICE is "incompatible" with RFC5245?
> The first example simply appears to be pointing to a way in which
> trickle ICE differs from vanilla ICE.

True, this is more about motivations for defining the trickle mode as an 
extension to the core ICE spec. We'll look into rewording this.

> s 4, para 2
> I think the draft should probably say the offering agent "MUST NOT
> attempt trickle negotiation" if it doesn't know the remote peer supports
> trickle, as opposed to "is is important that ..."

Section 5 has some text along these lines...

    Prior to sending an initial offer, agents using signaling protocols
    that support capabilities discovery MAY attempt to verify whether or
    not the remote party supports trickle ICE.  If an agent determines
    that the remote party does not support trickle ICE, it MUST fall back
    to using vanilla ICE or abandon the entire session.

I'd agree that the text §4 needs to be consistent with that, and will 
look into improvements.

> s 4.1, para 1
> The concept of "end-of-candidates indication" is introduced here without
> explaining it. In general, I think that describing Full Trickle
> completely before introducing Half Trickle would make it easier to
> follow the Half Trickle details.

Good point. We'll look into fixing that.

> s 4.1, para 1
> It's not clear that it would ever make sense to send a Half Trickle
> offer without a complete candidate list and an end-of-candidates
> indication, since if trickle support is not confirmed, there would be no
> opportunity to send additional candidates and have the non-trickle peer
> use them for anything. Suggesting such an operation appears to
> contradict the earlier discussion in section 4.

I see your point and will consult with the authors.

> s 4.1
> The explanation of how Half Trickle can deliver more than half the
> benefit almost seems to indicate that there is no reason for a UA that
> gathers in response to UI cues to perform Full Trickle. Is that what you
> intend?

I doubt that was the intent.

> s 5
> It might be good in the paragraph that says "agents MAY generate an
> offer that contains no candidates" to explain the benefit. I think its
> because of one or both of: 1) allows the offering agent to receive the
> remote agents initial candidate list sooner, and 2) it allows the
> answering agent to begin candidate gathering sooner.

That seems like a sensible explanation; in essence it enables both 
parties to start ICE gathering and trickling as soon as possible.

> s 5.1
> Your use of "... would still need to ..." in the paragraph about sending
> no candidates indicates that the use of IP6 :: is a MUST when there is
> no suitable candidate to send in the initial offer, rather than a MAY.

I don't think that was the intent, but I can see how the confusion might 
arise. We'll try to reword this text for increased clarity.

> s 6
> I'm not sure what "Specifically, such verification would indicate lack
> of support when the offer contains no candidates." means here. I think
> you mean that the RFC%@$% >

That's how I feel about some RFCs, too. ;-)

> support verification procedure would fail, but
> that it will not if trickle is supported. Is this the only change to the
> RFC5245 support verification procedure? And wouldn't it be better to
> specifically call out the required change(s) with MUST?

My sense of the text as it stands in draft-ietf-mmusic-trickle-ice-02 is 
that it's saying "if you include no candidates in the offer, the rules 
from RFC 5245 would imply that ICE is not supported, but that's not true 
if you can confirm that trickle ICE is supported". But the current text 
is not as clear as it could be here.

> s 6.1
> What would be the benefit of sending and answer with no candidates? Is
> this to avoid a timeout of some sort triggering on the offering peer? It
> might be helpful to implementers to give an example.

Yes, an example would help. I'd assume that the benefit is to send the 
answer as quickly as possible so that both parties can consider the 
overall section to be under active negotiation as quickly as possible.

> s 6.2
> It's unclear to me why it's useful to monitor Active/Frozen state for
> check lists with no remote and/or local candidates.

Does §6.2 actually say that?

> What is actually
> being monitored in? Or is this just meant to indicate than an empty
> checklist MUST/SHOULD be treated as frozen for the purpose of the frozen
> candidates algorithm?
>
> s 9
> Why does trickle ICE do something different than vanilla ICE regarding
> candidate redundancy and priority? Are there any possible negative
> consequences? If there's a reason to believe that this isn't a concern,
> it might be good to call it out.

I'll need to look at that more closely.

> s 9.1
> Are there potential cases where limiting the maximum list of candidate
> pairs with trickle could result in a different list than vanilla ICE?
> For example, it seems that deciding on the candidate pairs after all
> gathering is complete may allow candidate priority to be taken into
> account more effectively than dropping new pairs regardless of priority
> as candidates trickle in.

In practice I doubt that there will be an impact given that the maximum 
number of candidate pairs is 100. Some deployment feedback might be 
helpful here...

> s A.2
> The document goes into some detail about the importance of unfreeze
> timing as part of ensuring that checks get through. This indicates to me
> that resolution of the question of when to unfreeze and start checks is
> critical to resolve.

I agree that it would be good to resolve this issue. Hopefully the 
authors can come to some conclusions here before we publish the next 
version.

> That's it. Hope it's useful. Please let me know if any of these comments
> are unclear.

Thanks for the careful review!

Peter

-- 
Peter Saint-Andre
https://andyet.com/