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

[Justin Uberti <[e-mail]>]

Thanks for the detailed and thorough review. Most of these suggestions are
simple and straightforward and we will adjust the document accordingly. I
addressed each comment individually below.

Look for a new rev of the document in github next week.

On Wed, May 28, 2014 at 9:07 PM, tshields <[e-mail]> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
>
> Below are some comments that I have regarding
> [draft-ietf-rtcweb-jsep-06]. Overall it is very good, understandable,
> and concise, but I've provided my thoughts on certain sections that
> were not entirely clear to me.
>
> I guess my one comment addressing the document as a whole would be
> that the examples used in the draft are helpful in explaining certain
> aspects and therefore even more examples would be even more helpful.
>
> Which particular examples were most useful - the inline hand-wavy
descriptions, the API flows, or the example SDP?


>
> + Section 1.1, Paragraph 3, Last Sentence
>
> > "This mechanism effectively removes the browser almost completely
> > from the core signaling flow; the only interface needed is a way
> > for the application to pass in the local and remote session
> > descriptions negotiated by whatever signaling mechanism is used,
> > and a way to interact with the ICE state machine."
>
> I think this can be reworded to be clearer and more powerful, something
> such as:
>
> "JSEP removes the browser almost entirely from the core signaling flow.
> The Javascript application needs interfaces for only (1) passing in
> local and remote session descriptions and (2) interacting with the ICE
> state machine."
>
> Perhaps the exact wording I've suggested is not ideal, but something
> that more clearly shows that JSEP removes the browser from the signaling
> flow and that the application (and therefore, the application's
> developer) needs to do only two things for establishing a session. The
> way it is worded now is unclear at first read and does not adequately
> persuade me why JSEP is awesome (IMO).
>

Agree your wording is more direct. We'll rework this part.

>
>
> + Section 3.4.2, Paragraph 1
>
> > "However, to accelerate cases where the browser knows the number of
> > media streams to use ahead of time"
>
> An example of a situation where the browser might know the number of
> media streams ahead of time might be useful for context here, if one is
> easily explained.
>
>
Sure, that makes sense. The typical case here is for an endpoint that wants
to be able to quickly set up a received call. The caller has the ability to
just start the call and leave it in the local-offer state instead.


>
> + Section 3.5.1, Paragraph 2
>
> > "... the application can choose to apply it either as a provisional
> > answer, leaving open the possibility of using a different answer in
> > the future, or apply it as a final answer, ending the setup flow."
>
> How does the application decide whether to apply as provisional or
> final? Does JSEP care at all or is this left entirely up to the
> application to decide? This is expounded on somewhat in the 2 paragraphs
> that follow, but I still don't feel like these questions are completely
> answered.
>

This is an application decision; in JSEP's opinion, forking should overall
be avoided.

>
>
> + Section 3.6, Paragraph 3 (Open Issue)
>
> > "[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.]"
>
> Having some mechanism for keeping the PeerConnection object in the
> browser would be awesome and I agree that it should be explored further.
> However, I also think it should not replace the current rehydration
> method described as this allows the session state to be saved and
> restored from a server to the Javascript application running in the
> browser, correct?
>

The rehydration stuff has proved hard to get right. In the interest of
finishing this document, we have removed this section from JSEP. If there
is still sufficient interest in this in the future, we can revisit
rehydration in an extension document.

>
>
> + Section 4.1.2, Paragraph 2
>
> > "the generated SDP will contain all desired functionality for the
> > session (certain parts that are supported but not desired by
> > default may be omitted)"
>
> I think this sentence would be clearer if "certain parts that are" was
> replaced with "functionality that is".
>

Agreed.

>
>
> + Section 4.1.2, Paragraph 3
>
> > "For each existing stream, the generation of each SDP line must
> > follow the process defined for generating an updated offer from the
> > document that specifies the given SDP line."
>
> This sentence was too hard to compute in my opinion, and I'm still not
> quite sure what "the document" refers to. I think rewording would be
> beneficial.
>

Agreed.

>
>
> + Section 4.1.4, Paragraph 5
>
> > "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)."
>
> So is it entirely up to the receiver of an answer to decide if an answer
> should be treated as provisional or final, or does the offerer get to
> make a suggestion on this matter? Is there any recommendation or
> requirement on how long an application should keep a pranswer. My point
> is that an application may set a pranswer anticipating another answer
> that never comes. Does JSEP care at all about this situation?
>

The sender of an answer can mark it as pranswer or answer, as can the
receiver. In practice though, I expect this to be done by the receiver, as
it deals with multiple SIP replies. In the end this is up to the app, and
JSEP is This is really up to the app. The things you mention are all issues
the app needs to be aware of.

>
>
> + Section 4.1.7, Paragraph 2 & Section 4.1.8, Paragraph 2
>
> > "TODO: Do we need to expose accessors for both the current and
> > proposed local description?"
>
> I don't think I can offer much on this issue except to say that it
> would be nice if you could offer all the necessary functionality
> without having to expose these accessors to keep it as simple as
> possible for application developers.
>

As accessors, I don't think this is really forcing any additional
complexity on the developer. And I think they are needed. See
https://github.com/rtcweb-wg/jsep/issues/16

>
>
> + Section 5.1.2, bullet R-2
>
> > "R-2  ICE, as specified in [RFC5245], MUST be used.  Note that the
> > remote endpoint MAY use a Lite implementation."
>
> This shouldn't be MAY because it is not referring to an optional
> implementation specification of JSEP but rather a possible scenario that
> JSEP needs to support. It should read as just a warning and possibly
> specify that JSEP MUST be equipped to interface with an endpoint that is
> using a Lite ICE implementation.
>

Agree. We'll adjust accordingly.

>
>
> + Section 5.2.1, bullet 3
>
> > "   o  The third SDP line MUST be a "s=" line, as specified in
> > [RFC4566], Section 5.3; to match the "o=" line, a single dash
> > SHOULD be used as the session name, e.g. "s=-"."
>
> Is there a reason to ignore the following SHOULD from RFC4566?
>
> > "If a session has no meaningful name, the value "s= " SHOULD be
> > used (i.e., a single space as the session name)."
>
> It seems like "-" and " " are both equally meaningless, so why deviate
> from the standard? (I'm also not sure why we need to match it with the
> username in the o= line). There may be good reason for both of these,
> but it is not clear to me from reading the draft so I wanted to bring
> it up.
>
> It's entirely arbitrary. Since they are meaningless, I figured using
similar meaningless values made this more clear than individual meaningless
values. We can add some text to make this clear.

>
> + Section 5.2.1, Paragraph 3
>
> > "The next step is to generate m= sections for each
> > MediaStreamTrack..."
>
> Consider adding: "...as specified in [RFC4566] Section 5.14..." The
> draft references specific sections for the other SDP fields and I think
> it is useful here as well.
>

Sounds good.

>
>
> + Section 5.2.1, Paragraph 6, bullet 1 AND Paragraph 7, bullet 11
>
> > "...as indicated in [I-D.ietf-mmusic-trickle-ice], Section
> > 5.1...."
>
> Section 5.1 of [I-D.ietf-mmusic-trickle-ice] did not really clear this
> point up for me at all. Is using a 'null' port value the same as setting
> the connection address to "IP 6 ::" (which is mentioned in Section 5.1)?
> If we wanted to set two m= sections to null port values, how will we do
> so while maintaining this uniqueness constraint?
>

This is being tracked in https://github.com/rtcweb-wg/jsep/issues/41

>
>
> + Section 5.2.3, Paragraph 1
>
> > "The createOffer method takes as a parameter a MediaConstraints
> > object.  Special processing is performed when generating a SDP
> > description if the following constraints are present."
>
> I think an example of when this would be useful might be nice for context.
>

I assume you are asking for when each of the individual constraints
parameters would be useful. That sounds helpful.

>
>
> + Section 5.2.3.1 AND Section 5.2.3.2, Paragraph 1
>
> > "If the "OfferToReceiveVideo" constraint is specified, with a value
> > of "N", the offer MUST include N non-rejected m= sections with
> > media type "video",... "
>
> The use of "N" is confusing. Is "N" referencing an integer value or a
> string with value "N". I think removing the quotes around "N" would make
> it entirely more understandable.
>

Agreed. Constraints have been replaced with dictionaries, so we can clearly
refer to this as an integer var.

>
>
> + Section 5.3.1, Paragraph 5
>
> > "For each offered m= section of a given media type, if there is a
> > local MediaStreamTrack of the specified type which has been added
> > to the PeerConnection via addStream and not yet associated with a
> > m= section, and the specific m= section is either sendrecv or
> > recvonly, the MediaStreamTrack is associated with the m= section
> > at this time."
>
> This sentence is long and hard to parse. I think rewording and/or
> breaking up would be beneficial.
>

OK.

>
>
> + Section 5.3.1, Paragraph 10
>
> > "Note that regardless of the presence of "a=bundle-only" in the
> > offer, no m= sections in the answer should have an "a=bundle-only"
> > line."
>
> Should this be a SHOULD NOT or is it more just informational?
>

It's really just informational, to avoid any confusion. The actual BUNDLE
spec will have the exact guidance on this matter.

>
>
> + Section 6, Paragraph 3
>
> > "The following modifications, if done by the browser to a
> > description between createOffer/createAnswer and the
> > setLocalDescription, MUST be honored by the browser:"
>
> Does this sentence just mean that a browser will honor its own changes
> to a session description? Perhaps an example of when this might happen
> would help make the situation a little clearer.
>

This is being tracked in https://github.com/rtcweb-wg/jsep/issues/8

>
>
> - --Nits:
>
> + Section 3.2, Paragraph 4
>
> > "...the exact media parameters are only known only after a offer
> > and an answer have been exchanged..."
>
> Should read "only after an offer".
>

yep

>
>
> + Section 3.4.2, Paragraph 1
>
> > "...the number of media streams to gather candidates for."
>
> "... the number of media streams for which to gather candidates."
>

ok, not a fan of ending a sentence with a preposition, I see. I'll go along
with this one.

>
>
> + Section 3.5, Paragraph 1
>
> > "...the media plane which is relevant."
>
> Should say, "... the media plane that is relevant."
>

yep

>
>
> + Section 4.1.5, Paragraph 2 and 3
>
> > "This API changes the local media state"
>
> Maybe I'm alone on this but I prefer the following: "Calling this API
> changes the local media state." The API doesn't do anything on its
> own, but is a mechanism for calling some action.


> > "This API indirectly controls..."
>
> (Same issue as above) "The candidate gathering process is indirectly
> controlled through this API."
>

Will think this over. Overall, want to be consistent in how the term 'API'
is used.

>
>
> + Section 5.2.1, Paragraph 4
>
> > "Otherwise, it MUST use the same ICE credentials and candidates
> > that were used in the m= section that it is being bundled into."
>
> "into which it is being bundled."
>

ok

>
>
> + Section 5.2.1, Paragraph 5
>
> > "as a result, they MUST all have the same [RFC4572]fingerprint
> > value."
>
> Space needed after RFC reference.
>

yep

>
>
> + Section 5.2.1, Paragraph 7, bullet 6
>
> > "... that references the payload type fo the primary codec ..."
>
> "of"
>

yep

>
>
> + Section 5.2.1, Paragraph 7, bullet 11
>
> > "...as specific in [RFC5245]..."
>
> "specified"
>

yep

>
>
> + Section 5.3.1, Paragraph 7
>
> > "...each m= section in the answer should then generated..."
>
> "...should then be generated..."
>

yep

>
>
> + Section 5.3.1, Paragraph 7, bullet 4
>
> > "... and there a local MediaStreamTrack has been associated ..."
>
> "... and there is a local MediaStreamTrack that has been associated ..."
>

indeed, not my best work

>
>
> + Section 5.3.1, Paragraph 7, bullet 5
>
> > "... Section 3, MUST be be supported."
>
> "... Section 3, MUST be supported."
>

yep

>
>
> + Section 5.3.1, Paragraph 7, bullet 6
>
> > "...the payload type fo the primary..."
>
> "... the payload type of the primary..."
>

yep

>
>
> + Section 5.3.1, Paragraph 7, bullet 11
>
> > "... as specific in [RFC5245] ..."
>
> "... as specified in ..."
>

yep

>
>
> Thanks!
>
> - --
> Troy Shields
> Developer of Awesome | Jive Communications, Inc.
> Jive.com | [e-mail]
>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
> Comment: GPGTools - https://gpgtools.org
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQIcBAEBCgAGBQJThrKEAAoJEPpHBvvPaXlQjwEQAJ0gOz+wj6YEp++/GD/fbifB
> 6tA/jXvcF1HHqLLilfHZof7bnWwjYGTzbCFaT21xnouXEb0x1ZEuL2nVvK5CU801
> sOJtOJtEavwkbRQffp8ECE4SMZnEUocRFEro1QtV1Lay5EwV86Kn7gqoJCOf8AVJ
> UuFLrPktJFX9bbWQEXrjHXDjFUj+zvSj23UKAKEvMoLJKoXvuAKadU+UoZcuNBDo
> SFoBSCsu91F4tE3InUn3uZlMt+GogeikfnPbmc8qdw+p7XzsztZf1oHa6eC8AZiT
> 6YHEnNHqG7klc9EO/Gr1lfdwTkbnyybbqRUEpQUNURlmqMOD8+Z3V8t3IWFxkdbU
> 61QMt8TG0N1AVEuIGGU4JaQfqF0kCVgb9H2+UE7hg3jXbT1lavmWLuOwecB79LTQ
> 4Iemyygz1DG4GmJ/vhpVy++azMQqjEWeRQbaXg5j5gvkUyjb1iAnNlNcl98ZqPli
> drcsvWraBldIEpxR3cEA3Cgj+dCbJXuGA6SOy6C6xciwib7ZRAeKL40XPdeaSnbg
> hDXFwuAMOPycpfzpmUIoTQVtAQt8iM2bNyiuVoPnOSFyx/YEh7FyBZg4C/8LW8ZT
> WiW08Jujr96l2b+8J4ajzRoHgGa7iPCajO3DLxJxncAEZyvQk8+nGNTTVVBoso1Q
> XstBoLY6o9nE91hnOqle
> =rkNA
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> rtcweb mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rtcweb
>