Re: [rtcweb] Genart last call review of draft-ietf-rtcweb-jsep-21

[Justin Uberti <[e-mail]>]

Thanks for this careful review. A new version of the document (-22) has
been posted which addresses almost all of these comments, with a few
exceptions noted below.

Diff:
https://www.ietf.org/rfcdiff?url1=draft-ietf-rtcweb-jsep-21&url2=draft-ietf-rtcweb-jsep-22&difftype=--html

On Tue, Aug 8, 2017 at 12:31 PM, Robert Sparks <[e-mail]> wrote:

> Reviewer: Robert Sparks
> Review result: Almost Ready
>
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
>
> For more information, please see the FAQ at
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document: draft-ietf-rtcweb-jsep-21
> Reviewer: Robert Sparks
> Review Date: 2017-08-08
> IETF LC End Date: 2017-08-11
> IESG Telechat date: Not scheduled for a telechat
>
> Summary: Almost ready for publication as a Proposed Standard RFC, but with
> issues that need to be addressed before publishing
>
> Major issues:
>
> The last paragraph of 5.1 needs more context. Why woudn't you expect the
> re-offer to fail, given that you already know the peer is using the older
> profile strings? It's not clear from the email list and proceedings how the
> document ended up with this particular text.
>
> Minor issues:
>
> Second paragraph of 3.2, last sentence: at "these parameters or reject
> them" - "these" and "them" have ambiguous antecedents. It's not clear if
> you
> are trying to point to the parameters that don't follow the earlier stated
> rule, or all parameters. The sentence is complex. Splitting it up and
> finding a way to avoid the need for the semicolon will likely make it
> easier to say what you mean.
>
> Last paragraph on page 5 - first sentence: at "One way to mitigate this" -
> "this" has an unclear antecedent. Did you mean "One way to reduce the
> complexity of the application's responsibilities"?
>
> Last paragraph of 3.5.2.1 - "all of these fields" is ambiguous. Please
> state precisely which fields you mean.
>
> Odd edge: The description createAnswer (in 4.1.7) leaves open the
> possibility that the application could call createAnswer twice
> (back-to-back with no intervening calls to other APIs) with different
> values in the options parameter. Is this ok? If not, should there be
> language that says not to do that somewhere?
>
> Slightly more than minor maybe: The first paragraph of 5.1.2 has at least a
> reference error. UDP/TLS/RTP/SAVPF is not specified in RFC7850. It's in
> RFC5764. Assuming that this is only a reference error, then the last
> sentence seems like a stray. It doesn't seem to be in the right context
> ("this advertisement" doesn't cast back to the discussion of the two
> profiles above it, which are both UDP only). Please clarify the sentence,
> giving it the right context, or remove it.
>
> The fourth sentence of the second bullet of section 5.2.1 is confusing. (It
> starts "It is RECOMMENDED that the <sess-id>".) RFC3265 requires the
> initial sess-id high-order bit to be zero. As written, this can be read to
> say that setting that bit to zero is only RECOMMENDED, and that it's
> possible to have sess-ids that are something other than 64 bits. You mean
> to say "Do what 3265 says to do and make the lower 63 bits random". Here's
> one possible replacement for the sentence: "RFC 3265 requires that the
> high-order bit of the initial <sess-id> be zero. It is RECOMMENDED that the
> remaining 63 bits be initially set to a cryptographically random value."
>
> The last paragraph of 5.3.1 prohibits the same attributes from generated
> answers that are prohibited in generated offers. Consider adding text for
> what to do if an offer shows up from a peer with some of those prohibited
> attributes.
>

This is covered by the last paragraph on page 43:

   Note that these requirements are in some cases stricter than those of
   SDP.  Implementations MUST be prepared to accept compliant SDP even
   if it would not conform to the requirements for generating SDP in

   this specification.



> The second paragraph of section 5.4 is confusing. A slight restructuring of
> the sentences would remove much of the potential confusion, I think. I
> suggest replacing the first sentence (which is very complex) with this:
> "After calling setLocalDescription, the application MAY modify the SDP to
> reduce the capabilities in the offer before sending it to the far side, as
> long as it remains a valid SDP offer and specifies a subset of what was in
> the original offer. Likewise, an application that has received an offer
> from a peer MAY modify the received SDP, subject to those same constraints,
> before calling setRemoteDescription."
>
> The first paragraph of section 5.7.1 is simply restating requirements from
> RFC4566. Restating normative requirements like that has led to interop
> problems through unintended introduced ambiguity and spec maintenance
> problems. Please consider rewriting the paragraph to say "RFC4566 says" and
> avoid the 2119 keywords.
>

We considered this, but found that a) RFC4566 is clearly cited, and b) the
actual restatement was very minor, really just affecting the discussion of
the v= and o= lines. As a result we concluded that pointing the reader
elsewhere just for those two lines did not seem like an improvement.


>
> On page 69, at "If the application attempt to transmit DTMF when using a
> media format that does not have a corresponding telephone-event format,
> this MUST result in an error". This sentence is out-of-place. The section
> is about applying a remote description. If the document needs to retain the
> sentence, it belongs off in some "processing media" section. In particular,
> the current placement of text is confusing about what and when an error
> could be returned, and it could be misread to say that an error should be
> returned to the setRemoteDescription call.
>
> Nits:
>
> Section 1.2 second paragraph - "This was rejected based on a feeling". Can
> you write something that speaks to consensus here? "Based on a feeling"
> isn't quite right. Maybe "using the argument"?
>
> PeerConnection is used first at the next to last paragraph of section 3.
> Please add a reference at that point.
>
> "LS" (lipsync) is first used at 4.1.2, with no context for what it is.
> Please add a reference.
>
> The concept of a "pending local description" is used in sections 3.5.1,
> 4.1.6, and 4.1.7  but isn't really explained until much later in the
> document. Please provide forward references, or move a description of what
> a pending description is earlier in the document.
>
> In 4.1.7 (create Answer) why would calling setLocalDescription cause the
> answer to be a _pending_ local description. I think it's the full on local
> description at that point, not a pending one?
>
> At 4.1.17's first paragraph: Why is "if parsed successfully" necessary to
> call out? I suggest removing the phrase.
>
> 4.2.3 talks about setting direction properties, but the values that can be
> set aren't called out (and even then only by inference from a list in an
> example) in section 4.2.5. Should there be explicit pointers to the api doc
> here? (And in section 4.2.6?)
>
> There are two paragraphs on page 50 that start "The next step is to". They
> are both referring to the same step. I suggest combining the paragraphs.
>
> The 4th bullet on page 64 is ambiguous at "this m= section". It looks like
> this bullet should just be merged with the previous bullet.
>
> The second paragraph of 6.8 starts "Next, ". I think it should start
> "First, ".
>
> It's not clear what the "Or," in the first bullet on page 65 is referring
> to. Are you trying to say "If the m= section is not new, and the ICE ufrag
> and password values have changed" or something else?
>

The intent here is as stated, "if the m= section is not new and the values
have changed". We discussed this but concluded that the meaning of the
current text was clear.


>
> In the last paragraph of section 8 at "programmer MUST recognize". That is
> not an appropriate use of 2119. I suggest "needs to be aware" instead of
> "MUST recognize".
>
> Micro Nits:
>
> Suggested change to the first sentence of 1.1
>
>   OLD:
>
>     The thinking behind WebRTC call setup has been to fully specify and
> control the media plane, but to leave the signaling plane up to the
> application as much as possible.
>
>   New:
>
>     WebRTC call setup has been designed to focus on controlling the media
> plane, leaving signaling plane behavior up to the application as much as
> possible.
>
> The first full bullet on page 18 has a sentence that starts "Note that when
> considering". Please change that to simply "When considering". As written,
> there is an implication that this is a consequence of something specified
> somewhere else.
>
> The verb "surfaced" appears three times in the document without support or
> definition. Can you replace them with simpler language?
>
> At the second to last sub-bullet at the end of page 66, please remove "note
> that". The sentence is stronger without it.
>
> I noted these sections as particularly hard to read when going through the
> document linearly. I don't have concrete suggestions for improvement, but
> perhaps other eyes (if they agree the sections could use improvement) can
> come up with something:
>  - First sentence of the first paragraph on page 19.
>  - 2nd sentence of the 4th paragraph on page 19.
>
>
> _______________________________________________
> rtcweb mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rtcweb
>