Re: [rtcweb] Review of draft-ietf-rtcweb-jsep-07

[Justin Uberti <[e-mail]>]

Thanks for the review. Comments inline.

On Fri, Jul 18, 2014 at 2:32 PM, Paul Kyzivat <[e-mail]> wrote:

> I just did my first thorough read-through of JSEP in quite some time. I
> came away with a number of concerns:
>
> Section 1.1:
>
> When describing offers, modification by the application is mentioned:
>
>    JSEP's handling of session descriptions is simple and
>    straightforward.  Whenever an offer/answer exchange is needed, the
>    initiating side creates an offer by calling a createOffer() API.  The
>    application *optionally modifies that offer*, and then uses it to set
>    up its local config via the setLocalDescription() API.
>
> but when describing answers it is not:
>
>    When the call is accepted, the callee uses the createAnswer() API to
>    generate an appropriate answer, applies it using
>    setLocalDescription(), and sends the answer back to the initiator
>    over the signaling channel.  When the offerer gets that answer, it
>    installs it using setRemoteDescription(), and initial setup is
>    complete.
>
> And Section 6 only talks about changing offers, not answers. It is equally
> important to be able to modify answers. (More on this in later comment on
> section 6.)
>

Agreed. This was simply an oversight.

>
> Section 4.1.4:
>
>    The only difference between a provisional and final answer is that
>    the final answer results in the freeing of any unused resources that
>    were allocated as a result of the offer.  As such, the application
>    can use some discretion on whether an answer should be applied as
>    provisional or final, and can change the type of the session
>    description as needed.  For example, in a serial forking scenario, an
>    application may receive multiple "final" answers, one from each
>    remote endpoint.  The application could choose to accept the initial
>    answers as provisional answers, and only apply an answer as final
>    when it receives one that meets its criteria (e.g. a live user
>    instead of voicemail).
>
> Issue: in this forking case, until the final selection is made it is
> unclear which one will need ICE completed. Only when a setlocal is done on
> one of the answers will ICE begin to be performed. Then, if another answer
> is provisionally set, won't that stop ICE for the prior one? And won't it
> require new candidates? What if one of the earlier ones is eventually
> chosen as final?
>

Yes. This will be discussed in the setRemoteDescription handling section,
when it is written. Basically, ICE will be performed with the current
answer, and may complete. If another answer is set, that will cause all
current remote ICE candidates to be discarded, breaking any existing
connections; ICE will resume using any candidates supplied in the new
answer (or after the fact in addIceCandidate).


> And what if ICE completes for one of the candidates? Can't media start to
> flow? Then what if a different candidate is set as the final answer?
>

Yes. This is an application problem to resolve.

>
> Section 4.1.4.1:
>
> I question the following:
>
>    ...  While it is good practice to send an immediate
>    response to an "offer", in order to warm up the session transport and
>    prevent media clipping, the preferred handling for a web application
>    would be to create and send an "inactive" final answer immediately
>    after receiving the offer.  Later, when the called user actually
>    accepts the call, the application can create a new "sendrecv" offer
>    to update the previous offer/answer pair and start the media flow.
>
> This is very unfriendly when receiving calls that might be forked. By
> immediately "answering" a call before knowing if the user will accept it,
> you preempt the possibility that it will be answered on some other fork.
>
> For instance, if a call could come to your browser, or be sent to an
> answering service, and your broswer is on but you aren't present to accept
> the call, then the call will be accepted and then dropped rather than sent
> to the answering machine.


> So this technique should not be used if there is any possibility that the
> request could be coming from a source that might try other possibilities.
>

Agreed. But if you are building an application from scratch, I think it
would be better to use a separate PeerConnection for each remote endpoint,
so that you can use this technique without any downsides.

>
> Section 5.2.1:
>
>    When createOffer is called for the first time, the result is known as
>    the initial offer.
>
> By this definition, if a peer connection initially *received* an offer and
> sent an answer, and then it later sends an offer then that offer would be
> considered an initial offer.
>
> I think perhaps what is intended is:
>
>    When createOffer is called before setLocal has been called with
>    an offer,  the result is known as the initial offer.
>

OK. Will clarify.

>
> The following doesn't support the "balanced" BUNDLE policy:
>
>    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.  The effect of this is that the browser offers all
>    m= sections as one BUNDLE group.  However, whether the m= sections
>    are bundle-only or not depends on the BUNDLE policy.
>

Not sure what you mean; the BUNDLE policy controls the use of bundle-only,
as described here, so I don't see an issue.

>
> Section 5.2.2 says:
>
>    o  If any MediaStreamTracks have been added, and there exist recvonly
>       m= sections of the appropriate media type with no associated
>       MediaStreamTracks, or rejected m= sections of any media type,
>       those m= sections MUST be recycled, and a local MediaStreamTrack
>       associated with these recycled m= sections until all such existing
>       m= sections have been used.  This includes any recvonly or
>       rejected m= sections created by the preceding paragraph.
>
> This fails to say anything about codec compatibility. SDP O/A requires the
> answer to be a subset of the offer in terms of PT/codec configuration. You
> should not use the same m-line unless there is a desire to use the same
> settings for the new track as are currently in use in the other direction.
>

OK. This is an open issue (https://github.com/rtcweb-wg/jsep/issues/21) and
we were waiting for someone to point out a specific problem here. Would you
mind coming up with a specific example to illustrate the issue?

>
> Section 5.3.1:
>
>    When createAnswer is called for the first time after a remote
>    description has been provided, the result is known as the initial
>    answer.
>
> By this definition, if a peer connection initially sent an offer and
> received an answer, and then it later receives an offer then the answer to
> that first *received* offer would be considered an Initial Answer.
>
> I think perhaps what is intended is:
>
>    When createAnswer is called before setLocal has been called with
>    an offer,  the result is known as the initial answer.
>

As above.

>
> When specifying the mapping of local tracks to m-lines in a received
> offer, there is again no discussion of codec compatibility. It is quite
> possible that the m-line chosen by the algorithm in this section won't have
> compatible codec attributes, but some other m-line will.
>

As above.

>
> Sections 5.3.2, 5.3.3, and 5.4-5.7:
>
> Are these empty because the content is yet to be written?
>

Yes.

>
> Section 6:
>
> Other likely changes are addition of extra attributes and maybe other
> parameters. For instance, CLUE will want to add another grouping construct.
>

Would these things be changed prior to setLocalDescription? The browser
won't act on them, so I would think they could be added after the fact.

>
>         Thanks,
>         Paul
>
> _______________________________________________
> rtcweb mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rtcweb
>