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

[Robert Sparks <[e-mail]>]

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.

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.

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?

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.