Re: [rtcweb] Ben Campbell's Yes on draft-ietf-rtcweb-jsep-24: (with COMMENT)

[Ben Campbell <[e-mail]>]

> 
> 
>> On Dec 21, 2017, at 8:31 PM, Justin Uberti <[e-mail]> wrote:
>> 

[…]


>> ----------------------------------------------------------------------
>> COMMENT:
>> ----------------------------------------------------------------------
>> 
>> I'm balloting "yes", but I have some minor comments and nits:
>> 
>> Substantive:
>> 
>> - General:
>> I still find the edges around where SDP is and isn't required a bit vague.
>> Section 3.3 says that the JSEP implementation internal representation is SDP.
>> While I have trouble imagining implementing this otherwise, do we really mean
>> to mandate the internals? Is there an assumption that said internal
>> representation is _serialized_ SDP vs some abstract internal representation?
>> 
> I suppose this could be restated to say that we can *think* of SessionDescriptions
> as SDP containers, but the internal representation could be
> anything, as long as it can roundtrip to SDP. Would that be preferable?

I think that would be more accurate.

> 
>> Section 3.1 says that the signaling model is not specified. Is there an
>> implicit assumption that, however things are signaled, the signaling process
>> moves around serialized SDP? Or is it envisioned that one could write an
>> application without serialized SDP occurring anywhere above the API?
>> 
> The API does move around session descriptions, and, as mentioned above, they
> can be thought of as SDP containers. However, the application signaling
> protocol need not involve SDP at all, as mentioned:
> 
>      JSEP does not specify a particular signaling model or state
>      machine, other than the generic need to exchange session
>      descriptions in the fashion described by
>      <xref target="RFC3264"></xref> (offer/answer)
> 
> IOW, the app signaling model is not specified, but the API interaction
> model does involve the use of SDP containers.

The clarification you proposed to my previous comment helps here, too.

> 
>> -5, throughout: There are a number of normative keywords used in constructions
>> like "... MUST foo, as described in RFCXXX". That construction is ambiguous
>> about whether it defines a normative requirement to do something, and the doing
>> of that something is described in the RFC, or if describes the referenced RFC
>> as defining the MUST.
>> 
>> In general, it's best to avoid using 2119/8174 keywords to talk about external
>> requirements, except as a direct quote. I think this is especially true here
>> given the interdependencies between drafts in cluster 238. If in the future we
>> update a dependency to change a normative requirement, we risk creating
>> conflicts, and we make it unclear which text is authoritative.
>> 
> Generally, this is pointing the reader at a particular normative section,
>    and saying that yeah, you MUST follow that section as written. Perhaps
>    the use of 2119 language here isn't ideal, but I don't see this as
>    confusing. Do you have a suggestion about how this should be handled instead?

It’s a technicality, but it can create confusion about which text is authoritative. That’s not an issue as long as both are in agreement, but future updates to either this or that spec can easily mess up that agreement.

My personal preference is to reserve 2119/8174 keywords for the text that is authoritative for the requirement, and to have other text that refers to that requirement do it descriptively. For example, “RFC XXXX mandates foo”.


> 
>> -5.1.1: I think the operative requirement is "MUST support" rather than "MUST
>> indicate support". (While indication may also be required, it seems a
>> consequence of "MUST support".)
>> 
> Session descriptions can't really support these specs, but they can
>        indicate support by including the appropriate lines. I tend to think
>        this is correct as written.

To clarify, it seems like the root requirement is that JSEP implementations MUST support ICE and DTLS/DTLS-SRTP.  The requirement for session descriptions to indicate support is a consequence of that.

> 
>> -5.1.2: " Any profile in the offer matching one of the following MUST be
>> accepted:" Isn't the real requirement that any of the following must be
>> interpreted the same, or otherwise in some specified way? I assume there may be
>> completely unrelated reasons to reject an m-section, but the "MUST be accepted"
>> language seems to overrule that.
>> 
> Yes, perhaps we could be clearer and say
>        "MUST be considered valid" (in reference to the profile).

That would help, thanks.

> 
>> -5.4: "The SDP returned from createOffer or createAnswer MUST NOT be changed
>>    before passing it to setLocalDescription."
>> Is that a requirement on the JSEP implementation, or the javascript
>> application? If the latter, is it appropriate to put normative requirements on
>> the application? It seems like it would be better to normatively state the JSEP
>> implementations's behavior when the application does something incorrect.
>> (Which seems to be done further down the page.)
>> 
> This section is, as you mention, application guidance. Perhaps RFC2119
>      language doesn't make sense here, but the guidance here seems like a good
>      summary for how the application should behave.

My personal preference would be to avoid use of 2119 keywords for things the JSEP implementation does not control.

> 
>> -5.7: " The effect of rollback MUST be the same regardless of whether
>> setLocalDescription or setRemoteDescription is called." Does that mean if I
>> call setLocalDescription, then rollback with a call to setRemoteDescription,
>> _both_ are rolled back?
>> 
> Well, there is only a pending local at this point, and it is rolled back by setRD.

Okay.

> 
>> -5.8.3: The MUSTs in this section seem to be putting requirements on the SDP
>> being parsed. Shouldn't they instead describe the parser's behavior based on
>> that SDP input? The correctness of the SDP being parsed seems beyond the
>> control of the implementation.
>> 
> Yes, the correctness of the SDP is beyond the control of the impl, but,
> the handling of broken SDP is spelled out in the beginning of this section:
> 
>         If any line is not well-formed, or cannot be parsed as
>         described, the parser MUST stop with an error and reject the
>         session description, even if the value is to be discarded. This
>         ensures that implementations do not accidentally misinterpret
>         ambiguous SDP.
> 
> Saying that a specific line MUST have X and Y is then a shorthand for
> repeatedly saying that if the line cannot be parsed properly, the impl MUST
> stop with an error.

Again, I don’t think the use of 2119/8174 keywords for things beyond the control of the implementation fits the sense of those documents. I suggest either stating these without 2119/8174 keywords, or adding something to the boilerplate to the effect of “When used in to describe requirements for SDP inputs, these terms indicate requirements for that SDP to be considered well-formed”.

> 
> 
>> -8, second paragraph: When describing how the javascript is untrusted, it may
>> be worth mentioning that it may have been downloaded from an untrusted source.
>> 
> yes, we could simply say that it is untrustworthy as a result of being
> downloaded from an untrusted source
> 
>> 
>> Editorial and Nits:
>> 
>> 

[…]

>> -3.6.2, 2nd paragraph: s/"may be producing"/"may produce"
>> 
> "may be producing" seems OK to me. Did you have a specific concern?
> 

Just a grammar nit. “may be producing” uses the progressive aspect. That indicates temporary action ongoing at some point of time. Is that the intent?

>> -4.1.2: Please expand LS on first mention. (I can guess from context. Please
>> don't make me :-)  )
>> 
>> - 4.1.6: s/"codec/RTP/RTCP"/"codec, RTC, or RTCP"
>> "for each SDP line, the generation of the SDP will follow the process defined
>> for generating an initial offer from the document that specifies the given SDP
>> line." While I worked out what that means, I found "document" to be ambiguous.
>> At first I interpreted it as the "SDP document" rather than "the RFC". (Note
>> this occurs several times in subsequent sections.)
>> 
>> -4.1.8, third paragraph: ""pranswer" indicates that a description should be
>> parsed as an
>>    answer, but not a final answer"
>> I think it would be more clear to say "... parsed as a provisional answer,
>> rather than a final answer".
>> 
> Parsing has only 2 modes, offers and answers; the provisionality of the
>        answer only affects how it is handled once parsed. I think this should
>        stay as-is.

The existing text already talks about parsing as a final answer. If parsing is the same either way, perhaps it should say “treated as” rather than “parsed as”?

> 
> 
>> -5.2.1: Paragraph starting with: " Each m= section, provided it is not marked
>> as bundle-only, MUST generate..." The m= section is not the real actor here, is
>> it?
>> 
>> 
>> _______________________________________________
>> rtcweb mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/rtcweb
>> 
>