Re: [MMUSIC] review of draft-ietf-mmusic-ice-sip-sdp

[Suhas Nandakumar <[e-mail]>]

Hello Peter

  Thanks for the review. Please see inline

On Wed, Oct 3, 2018 at 6:51 PM Peter Saint-Andre <[e-mail]>
wrote:

> Although I am not an SDP expert, I've taken a look at this I-D and have
> a few small questions and comments.
>
> §3.2.1.6 states:
>
>    If the port value associated with an "m=" section is set to zero
>    (implying a disabled stream), the agent SHOULD NOT include ICE
>    related SDP candidate attributes in that "m=" section, unless an SDP
>    extension specifying otherwise is used.
>
> It would be helpful to cite Section 8.2 of RFC 3264 here.
>

[Suhas] Done

>
> §3.2.5 states:
>
>    2.  The transport address from the peer for the default destination
>        correspond to IP address values "0.0.0.0"/"::" and port value of
>        "9".  This MUST not be considered as a ICE failure by the peer
>        agent and the ICE processing MUST continue as usual.
>
> It would be helpful to cite Section 8.4 of RFC 3264 here.
>
>
[Suhas] This para was added to not break the case of Trickle where
IP address of 0.0.0.0 and port 9 can be used in the initial offer.
This doesn't apply to  putting a media stream on hold as indicated by
section 8.4 of RFC3264.
Also the draft advices against it as
"Consequently, ICE implementations MUST NOT utilize this mechanism for call
hold,
and instead MUST use a=inactive and a=sendonly as described in <<RFC3264>>.
"


> §3.4.1.1.1 states:
>
>    To restart ICE, an agent MUST change both the ice-pwd and the ice-
>    ufrag for the data stream in an offer.  Note that it is permissible
>    to use a session-level attribute in one offer, but to provide the
>    same ice-pwd or ice-ufrag as a media-level attribute in a subsequent
>    offer.  This is not a change in password, just a change in its
>    representation, and does not cause an ICE restart.
>
> The last sentence is somewhat nebulous - what do we mean by
> "representation" here?
>

[Suhas] By representation it meant the change in location of these
attributes
[from session to media level] in the SDP.
Changed the above as

"To restart ICE, an agent MUST change both the ice-pwd and the ice-ufrag
for
the data stream in an offer. However, it is permissible to use a
session-level
attribute in one offer, but to provide the same ice-pwd or ice-ufrag as a
media-level attribute in a subsequent offer. This MUST NOT be considered
as ICE restart."



>
> §3.4.1.3 states:
>
>    A lite implementation MUST NOT add additional host candidates or
>    change username fragments or passwords in a subsequent offer.
>    Otherwise, it MUST restart ICE.
>
> This is confusingly worded - if it's truly a MUST NOT, there is no
> "otherwise". Perhaps:
>
>    Unless it wishes to restart ICE, an lite implementation MUST NOT add
>    additional host candidates or change username fragments or passwords
>    in a subsequent offer.
>

[Suhas] Explained it more clearly as

"A lite implementation MUST NOT add additional host candidates in a
subsequent offer.  If an agent needs to offer additional candidates,
it MUST restart ICE. Similarly, the username fragments or passwords
MUST remain the same as used previously.  If an agent needs to change
one of these, it MUST restart ICE for that media stream.
"


> §3.4.2.1 is entitled "Detecting ICE Restart" but it doesn't say anything
> about detecting ICE restart, only about handling ICE restart.
>

[Suhas] Renamed the section "ICE Restart" since its ice restart
handling on the answerer for subsequent o/a


> §3.4.2.2 states:
>
>    Furthermore, if the agent believed it was controlling, but the offer
>    contained the a=remote-candidates attribute, both agents believe they
>    are controlling.  In this case, both would have sent updated offers
>    around the same time.
>
>    However, the signaling protocol carrying the offer/answer exchanges
>    will have resolved this glare condition, so that one agent is always
>    the 'winner' by having its offer received before its peer has sent an
>    offer.  The winner takes the role of controlling, so that the loser
>    (the answerer under consideration in this section) MUST change its
>    role to controlled.
>
> It might be helpful to cite Section 14.2 of RFC 3261 as an example of
> how the signaling protocol resolves the glare condition.
>

[Suhas] I tend to leave it as-is since these sections are generic ice-sdp
procedures and all of SIP specific things are under SIP considerations
section. Also, the glare can happen in general o/a scenario outside SIP too.


>
> §3.4.3.1 states:
>
>       If a pair on the new check list was
>       also on the previous check list, and its state was Waiting, In-
>       Progress, Succeeded, or Failed, its state is copied over.
>       Otherwise, its state is set to Frozen.
>
> Given that those are all the possible states, isn't it simpler to say
> the following?
>
>       If a pair on the new check list was also on the previous check
>       list, its state is copied over.
>
> Alternatively, we might be trying to also say that if a pair on the new
> check list was not on the previous check list, its state is set to
> Frozen. In that case I suggest:
>
>       If a pair on the new check list was also on the previous check
>       list, its state is copied over; if not, its state is set to
>       Frozen.
>
>
[Suhas] Reworded it slightly as
 "If a pair on the new check list was also on the previous check list, and
its state is not Frozen,
  its state is copied over. Otherwise, its state is set to Frozen."


> §6 states:
>
>    When ICE is used with SIP, forking may result in a single offer
>    generating a multiplicity of answers.
>
> However, §6.3 states:
>
>    ICE interacts very well with forking.
>
> Somehow those two statements don't seem fully consistent. :-)
>

[Suhas] I don't see the issue here. The first statement does state what
happens under forking and §6.3 explains it further by saying how with
ICE , some of the issues caused by forking can be resolved at the caller
 based on the connectivity checks that succeed.


>
> Peter
>
>
>
>
>
>
>
> _______________________________________________
> mmusic mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/mmusic
>