Re: [rtcweb] Comments on draft-ietf-rtcweb-jsep-06

[Justin Uberti <[e-mail]>]

Sorry for the super long latency on this. We've created issues in the GH
tracker for the points identified below. Specific comments inline.
https://github.com/rtcweb-wg/jsep/issues/51
https://github.com/rtcweb-wg/jsep/issues/52
https://github.com/rtcweb-wg/jsep/issues/53
https://github.com/rtcweb-wg/jsep/issues/54

On Fri, Mar 14, 2014 at 9:48 AM, Justin Uberti <[e-mail]> wrote:

> Thanks for the detailed review. We will work through these comments over
> the next few days.
>
>
> On Fri, Mar 14, 2014 at 6:05 AM, Magnus Westerlund <
> [e-mail]> wrote:
>
>> Hi,
>>
>> I did review the JSEP -06 prior to the meeting. Now I have had time to
>> write-up my comments.
>>
>> 1) Section 1):
>>
>> [W3C.WD-webrtc-20111027]
>> This is very old version of the API, please update to the currently
>> relevant version.
>>
>
Will do.

>
>> 2) Section 3.4.1.1:
>> The m-line index is a zero-based index, referring to the Nth m-line in
>> the SDP.
>>
>> a) I think the above is a bit confusing. As a zero-based index would
>> mean that the first m= section would have index 0. Thus, the referring
>> to the Nth m-line in the SDP is unclear. Is N, the provided index, in
>> which case it would be the N+1th m-line. Please rewrite.
>>
>
Will clarify.


>
>> b) I also wonder if one has to be clearer on what "the SDP" is. I assume
>> it is the one that has been generated by the agent, rather than the
>> latest sent or received?
>>
>
It can be for either the local generated SDP, in the case where the
candidate is gotten from the onicecandidate callback, or the remote SDP, in
the case where the candidates are trickled from the remote side. We'll add
some text to make this clear.

>
>> 3) Section 3.4.1.1:
>> "The MID uses the "media stream identification", as defined in
>>    [RFC5888] , to identify the m-line."
>>
>> a spurious space after ref.
>>
>
OK.

>
>> 4) JSEP applications typically inform the browser to begin ICE gathering
>>    via the information supplied to setLocalDescription, as this is where
>>    the app specifies the number of media streams to gather candidates
>>    for.
>>
>> I find the statement that ICE candidate gathering starts first with
>> setLocalDescription. Doesn't the created offer already contain local
>> candidates, thus one must have already gathered some candidates? To me
>> it appears reasonable to start gathering earlier, rather than later.
>> Thus, as soon as one create the PeerConnection object and have added any
>> MST to it or a data channel, one can start gathering.
>>
>
The initial offer comes out as soon as possible, so it contains no
candidates.

setLocalDescription is the first time the implementation knows exactly how
many m= lines it needs to gather for, so gathering can't definitively start
until this point. Candidates may be pre-gathered using the candidate pool,
but they aren't released via onicecandidate until setLD.


>
>> 5) Section 3.5.2:
>>    In the parallel forking case where the Javascript application wishes
>>    to simultaneously exchange media with multiple peers, the flow is
>>    slightly more complex, but the Javascript application can follow the
>>    strategy that [RFC3960] describes using UPDATE.  (It is worth noting
>>    that use cases where this is the desired behavior are very unusual.)
>>
>> I mostly react to the last sentence within the paragraph. I think the
>> poster child for semi-parallel forking was the MMORG that like to
>> establish PCs with anyone that is coming close in the virtual world.
>> Thus either needs to have a number of PCs on standby or have just one,
>> but treat that as parallel forking. And if you like to discourage the
>> later, then I think you might need to at least inform about the
>> possibility to have stand-by PCs.
>>
>
Is the MMORG case described in the use-case doc? If so, I agree the
parenthesized text should be changed.


>
>> 6. Section 3.6:
>>
>>    In the event that the local application state is reinitialized,
>>    either due to a user reload of the page, or a decision within the
>>    application to reload itself (perhaps to update to a new version), it
>>    is possible to keep an existing session alive, via a process called
>>    "rehydration".  The explicit goal of rehydration is to carry out this
>>    session resumption with no interaction with the remote side other
>>    than normal call signaling messages.
>>
>> I think this paragraph makes two erronous statements:
>> a) "keep an existing session alive"
>> I think "resume" an existing session would be more correct. Because it
>> would be broken when the reload happens.
>>
>> b) "with no interaction with the remote side"
>> My understanding is that due to DTLS and key handling a DTLS
>> renegotiation will be required with the peer. Isn't also a signalling
>> exchange likely to be required due to change of local DTLS cert as well
>> as ICE candidates as parameters? Any other parameters that will be
>> required to exchanges when rehydrating?
>>
>> This section has been removed, as nobody was working on this and it
wasn't clear exactly what it ought to do.


>  7. Section 3.6:
>> Next, a new offer is
>>    generated by the new PeerConnection; this offer will have new ICE and
>>    possibly new DTLS-SRTP certificate fingerprints (since the old ICE
>>    and SRTP state has been lost).
>>
>> I think this may require quite a bit more details. A forward pointer to
>> where you will specify that?
>>
>
See above

>
>> 8. Section 3.6:
>>    [OPEN ISSUE: EKR proposed an alternative rehydration approach where
>>    the actual internal PeerConnection object in the browser was kept
>>    alive for some time after the web page was killed and provided some
>>    way for a new page to acquire the old PeerConnection object.]
>>
>> Can't we declare this open issue closed? I have seen no details on the
>> alternative proposal. Thus, lets work with the one that is present.
>>
>> See above


>  9. Section 4.1.1:
>> There is no discussion of how the bundle policies relate to the Data
>> Channels? Are they included or do they have a specific set of policies.
>> Please extend this to discuss this.
>>
>
Will do.

>
>> 10. Section 4.1.3:
>>
>> This text doesn't appear to contain a requirement on that one MUST have
>> called setRemote prior to being able to call it.
>>
>
Will fix.

>
>> 11. Section 4.1.4.1:
>>    While this could also be done with an inactive "pranswer", followed
>>    by a sendrecv "answer", the initial "pranswer" leaves the offer-
>>    answer exchange open, which means the caller cannot send an updated
>>    offer during this time.
>>
>> Should one mention that also the callee can't send an offer without
>> doing a "final" answer?
>>
>
The callee could still send more pranswers, so it is not as restricted in
this state as the caller. However, it can't add streams at this point, so
there are some limitations.

If you think this is unclear I could change the text to say "neither side
can send an updated offer during this time"

>
>> 12. Section 4.1.4.1
>>
>> By the time the human has accepted the call and
>>    triggered the new offer, it is likely that the ICE and DTLS
>>    handshaking for all the channels will already be set up.
>>
>> Wouldn't the handshaking have "finished" rather than "set up"?
>>
>
Yes.

>
>> 13. Section 4.1.4.2:
>>    Rollback can only be used to cancel proposed changes; there is no
>>    support for rolling back from a stable state to a previous stable
>>    state.
>>
>> What is meant with "proposed changes"? From my side it is not clear how
>> much you can do after having performed a SetLocal or SetRemote. I think
>> these needs to be treated separately.
>
>
>> For an offerer, they can createOffer, SetLocal, send to peer, receive
>> reject and then undo the SetLocal. That is straight forward, and if they
>> done the SetRemote, they would be consider in a new Stable state and
>> can't roll back anymore?
>>
>> For the answering side, it receive an offer, does SetRemote,
>> CreateAnswer, determines that it doesn't like it, the can undo the
>> SetRemote and send a reject message to offerer.
>>
>> But what about this case? it receive an offer, does SetRemote,
>> CreateAnswer, SetLocal and sends the message, then receive a reject from
>> the peer, because it didn't like the answer? Are the answerer then in a
>> stable state that can't be unrolled, or?
>>
>
If you don't like the answer, I think it's game over. The answerer has
already started acting on the new local/remote description at this point.

IOW, "proposed changes" really only means what has been offered.

>
>> 14. Section 4.1.5:
>>    This API indirectly controls the candidate gathering process.  When a
>>    local description is supplied, and the number of transports currently
>>    in use does not match the number of transports needed by the local
>>    description, the PeerConnection will create transports as needed and
>>    begin gathering candidates for them.
>>
>> Another part of an earlier question regarding what triggers ICE gathering?
>>
>
See answer above.

>
>> 15. Section 4.1.7:
>>    TODO: Do we need to expose accessors for both the current and
>>    proposed local description?
>>
>> This needs to be resolved, I don't know if I can answer this question.
>>
>
We agreed in the interim that we would expose them.

>
>> 16. Section 4.1.9:
>>
>> Should this section discuss that it might be browser that has overriding
>> controls regarding if other candidates then relay ones may be provided,
>> i.e. privacy mode?
>>
>
 Probably. The use of host candidates is something that might be forbidden
by local policy. I think the right thing to do would be to introduce the
concept of iceTransports in section 3.4 (ICE), and then refer to it here in
updateIce (now called setConfiguration), as well as the ctor section, where
the initial policy can be set.

>
>> 17. Section 4.1.9:
>>    Regardless of the configuration, the gathering process collects all
>>    available candidates, but excluded candidates will not be surfaced in
>>    onicecandidate callback or used for connectivity checks.
>>
>> Doesn't this result in both resource consumption and that the configured
>> TURN server will learn the external IP address and port for the client?
>>
>
I'm not sure what the exact concern is here. Gathering candidates, even
those that aren't desired, allows the withheld candidates to be released as
soon as the app asks for them.

>
>> 18. Section 5.2.1:
>>
>> Structure in relation between RTP media and DATA channel. Maybe you
>> should try to find a structure that makes the difference between the two
>> types of transport more clear, and what applies to the perspective.
>>
>> Agree this could be improved.


>  19. Section 5.2.1:
>>
>>
>>    o  For the current default candidate, a "c=" line, as specific in
>>       [RFC5245], Section 4.3., paragraph 6.  If no candidates have been
>>       gathered yet, the default candidate should be set to the 'null'
>>       value defined in [I-D.ietf-mmusic-trickle-ice], Section 5.1.
>>
>> What about host candidates?
>>
>
See above. This text has been clarified in the latest non-public revision.

>
>> 20. Section 5.2.1:
>> The CNAME must be generated
>>       in accordance with draft-rescorla-random-cname-00.  [OPEN ISSUE:
>>       How are CNAMEs specified for MSTs?  Are they randomly generated
>>       for each MediaStream?  If so, can two MediaStreams be synced?]
>>
>> I think you should point to the RTP usage for this particular piece.
>> Because we do have text there both for the CNAME generation and how
>> CNAMES among MSTs and between PeerConnection. Likely to be changed soon
>> based on the mailing list discussion.
>>
>
Agreed.

>
>> 21. Section 5.2.1:
>>    o  [OPEN ISSUE: Handling of a=imageattr]
>>
>> Yes, I think imageattr should be supported to give guidance on suitable
>> resolutions that an endpoint like to receive for a particular MST.
>>
>> Agreed. Harald is working on changes to address this.


>  22. Section 5.2.1:
>>    Once all m= sections have been generated, a session-level "a=group"
>>    attribute MUST be added as specified in [RFC5888].  This attribute
>>    MUST have semantics "BUNDLE", and MUST include the mid identifiers of
>>    each m= section.
>>
>> Isn't this overriding BUNDLE as currently specified, forcing everything
>> into a single bundle group. Thus, preventing the bundle policies to work.
>>
>> The BUNDLE policies only affect what is bundle-only. Between two
browsers, a single BUNDLE will always be used.


>  23. Section 5.2.2:
>>
>>    o  If any MediaStreamTracks have been added, and there exist m=
>>       sections of the appropriate media type with no associated
>>       MediaStreamTracks
>>
>> Isnt' the last a "MediaStreamTrack" rather than plural ones?
>>
>> Agree this could be clarified.


>  24. 5.2.3.3.  VoiceActivityDetection
>>
>>    If the "VoiceActivityDetection" constraint is specified, with a value
>>    of "true", the offer MUST indicate support for silence suppression by
>>    including comfort noise ("CN") codecs for each supported clock rate,
>>    as specified in [RFC3389], Section 5.1.
>>
>> As currently specified this text requires the usage of a comfort noise
>> codec, currently there is no such on required. I think the "MUST" needs
>> to be rewritten.
>>
>
I think this needs to be specified as a MUST in RTP usage then. Otherwise,
this setting won't work in some browsers, leading to nondeterministic
behavior.

This should also be updated to set usedtx=1 for Opus.

>
>> 25. Section 5.3.1:
>>                                                                   Note
>>       that for simplicity, the answerer MAY use different payload types
>>       for codecs than the offerer, as it is not prohibited by
>>       Section 6.1.
>>
>> I react to the word simplicity here. I never consider usage of different
>> PTs per direction anything simple. But, yes it is allowed by RFC 3264.
>> But I kind of protest to encourage the usage.
>>
>>
Clearly this needs more explanation. The basic issue is the fact that using
the same payload types gets really hairy when using RTX and when bundling.
We can add an example of the problem.


 26. Section 5.3.2 etc.
>>
>> When will we see a first draft of the text in the sections:
>> 5.3.2, 5.3.3., 5.4, 5.5, 5.6, 5.7?
>>
>> I really like to see it prior to the Interim meeting.
>>
>
Work in progress, will be in jsep-07 before Toronto.


>
>> 27. Section 6:
>>    Note: This section is still very early and is likely to significantly
>>    change as we get a better understanding of a) the use cases for this
>>    b) the implications at the protocol level c) feedback from
>>    implementors on what they can do.
>>
>> Shouldn't this mention W3C discussion also?
>>
>
Sure.


>
>> 28. Section 7 - Security Considerations:
>>
>> First of all there are no references to the Security Considerations or
>> architecture document.
>>
>> Secondly, I would like to have some security discussion on significant
>> issues with JSEP itself. Resource attacks on the local endpoint for
>> example?
>>
>
We will add a open issue for this.

>
>> 29. section 10
>>
>> There are a couple of draft references that are either published RFCs or
>> at least WG versions. Please update the reference list.
>>
>> OK.


>  30. Section 10.2:
>> Looking at the spec, I think the following references are normative:
>>
>>    [I-D.ietf-mmusic-trickle-ice]
>>               Ivov, E., Rescorla, E., and J. Uberti, "Trickle ICE:
>>               Incremental Provisioning of Candidates for the Interactive
>>               Connectivity Establishment (ICE) Protocol", draft-ietf-
>>               mmusic-trickle-ice-00 (work in progress), March 2013.
>>
>>    [I-D.ietf-rtcweb-data-protocol]
>>               Jesup, R., Loreto, S., and M. Tuexen, "WebRTC Data Channel
>>               Protocol", draft-ietf-rtcweb-data-protocol-04 (work in
>>               progress), February 2013.
>>
>>
>> OK.


>  Cheers
>>
>> Magnus Westerlund
>>
>> ----------------------------------------------------------------------
>> Services, Media and Network features, Ericsson Research EAB/TXM
>> ----------------------------------------------------------------------
>> Ericsson AB                 | Phone  [phone]
>> Färögatan 6                 | Mobile [phone]
>> SE-164 80 Stockholm, Sweden | mailto: [e-mail]
>> ----------------------------------------------------------------------
>>
>> _______________________________________________
>> rtcweb mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/rtcweb
>>
>
>