Re: [bfcpbis] AD Review of draft-ietf-bfcpbis-rfc4583bis-24

[Christer Holmberg <[e-mail]>]

Hi,

I have updated the pull request. It should now have addressed everything except the proto value issue.

https://github.com/cdh4u/draft-bfcp-4583bis/pull/10

Regards,

Christer

-----Original Message-----
From: Adam Roach [mailto:[e-mail]] 
Sent: 10 September 2018 23:42
To: [e-mail]
Cc: [e-mail]
Subject: AD Review of draft-ietf-bfcpbis-rfc4583bis-24

This is my AD review of draft-ietf-bfcpbis-rfc4583bis. Thanks to everyone who worked on this document. It looks generally in good shape, but there are two issues that I think need to be addressed before placing the document into IETF last call. I believe these should be pretty easy to address.

NOTE: If you think I am wrong about either blocking point, please speak up. It is entirely possible that I've misunderstood something, and that the things I believe are issues are actually okay.


---------------------------------------------------------------------------

§4 (BLOCKING):

 >  This document defines five values for the proto field: TCP/BFCP,  >  TCP/DTLS/BFCP, TCP/TLS/BFCP, UDP/BFCP, and UDP/TLS/BFCP.

Generally, having more ways to do the same things in a protocol leads to less interoperability rather than more. While the rationale for the four-way split caused by TLS-versus-plaintext and UDP-versus-TCP is pretty self evident, there appears to be no rationale in this document for having both TCP/DLTS/BFCP and TCP/TLS/BFCP; more importantly, the document offers no guidance to implementors regarding which to use. This is likely to lead to some implementations choosing one encoding and others choosing the other somewhat arbitrarily. This decreases the chances of the protocol interoperating.

Minimally, please include guidance regarding which of these two variations implementations should use, and under which conditions. On a first glance, it would appear that the guidance might be that non-ICE uses should use TCP/TLS/BFCP for maximal compatibility with RFC 4583 implementations, and that ICE uses need to use TCP/DTLS/BFCP, as outlined in section 9.


---------------------------------------------------------------------------

§7.1 (BLOCKING):

 >  When the existing TCP connection is closed and re-established  >  following the rules in [16], the floor control client MUST send an  >  offer towards the floor control server in order to re-establish the  >  connection.

The behavior here is ambiguous in the case that "a=floorctrl:c-s" is negotiated.
This needs to be clarified. Perhaps something like:

    If the peers are acting as both floor control client and floor control
    server (i.e., if "a=floorctrl:c-s" has been negotiated), then the peer that
    most recently sent a successfully answered offer MUST send an offer.

---------------------------------------------------------------------------



The remaining comments are editorial, and can be handled as part of IETF last call, or prior to last call, at the authors' convenience.

---------------------------------------------------------------------------

Abstract:

 >  This document obsoletes RFC 4583.  Changes from RFC 4583 are  >  summarized in Section 15.

Nit: update to "Section 14."

---------------------------------------------------------------------------

§1:

 >  Following this model, a
 >  BFCP connection is described as any other media stream by using an  >  SDP 'm' line, possibly followed by a number of attributes encoded in  >  'a' lines.

It's probably worth including 'c' and 'b' lines in here as well, or possibly phrasing it to be more general ("...possibly followed by additional SDP lines that also apply to the BFCP connection")

---------------------------------------------------------------------------

§2:

Please update this section to match the boilerplate in RFC 8174.

---------------------------------------------------------------------------

§3:

 >  conference server will act as floor control server. However, there  >  are scenarios where both endpoints would be able to act as floor  >  control server.

Nit: "...act as a floor control server."
                 ^

---------------------------------------------------------------------------

§4:

 >  RFC4571 such that the length field defined in RFC4571 precedes each

Nit: "RFC 4571" (twice). See RFC 7322 §3.5, third bullet.

---------------------------------------------------------------------------

§4:

 >  Similarly, UDP/BFCP is used when BFCP runs directly on top of UDP,  >  and UDP/TLS/BFCP is used when BFCP runs on top of DTLS, which in turn  >  runs on top of UDP.

I'm sure there's rationale behind why this is "UDP/TLS/BFCP" rather than "UDP/DTLS/BFCP". It would benefit the reader to include a one-sentence explanation.

---------------------------------------------------------------------------

§5.1:

 >  This section defines the SDP 'floorctrl' media-level attribute.  The  >  attribute is used to determine the floor control role(s) that the  >  endpoints can take for the BFCP-controlled media streams. As  >  described in Section 5.1

Nit: "As described in this section..."

---------------------------------------------------------------------------

§5.1, §5.2, §5.3, §5.4:

 >      The Augmented BNF syntax [RFC5234] for the attribute is:
...
 >        ;DIGIT is defined in [RFC5234] ...
 >        ;token is defined in [RFC4566]

The syntax for references in this document are of the style [2] rather than [RFC5234]. Please either update the references in these sections to match the rest of the document, or (my suggestion) include the following directive in your XML source:

<?rfc symrefs="yes" ?>

---------------------------------------------------------------------------

§5.3:

 >     Although the example was non-normative, it is implemented by some  >     vendors and occurs in cases where the endpoint is willing to act  >     as an server.

Nit: "...as a server."

---------------------------------------------------------------------------

§9:

 >  multiple valid candidate pairs, and if BFCF media is shifted between

Nit: BFCP

---------------------------------------------------------------------------

§10:

 >     Note: The use of source-specific SDP parameters [18] is not  >     defined to BFCP streams.

Nit: "...defined for BFCP streams."

---------------------------------------------------------------------------

§10.1:

 >  o  MUST associate an SDP 'label' attribute (Section 5.3) with the 'm'
 >     line of each BFCP-controlled media stream.

This is a little confusing, as Section 5.3 does not define the 'label'
attribute. I would suggest replacing "(Section 5.3)" with a reference to RFC 4574.

---------------------------------------------------------------------------

§10.2:

 >  When the answerer receives an offer, which contains an 'm' line  >  describing a BFCP stream, the answerer MUST check whether it supports

"Contains an 'm' line describing a BFCP stream" appears to be a restrictive clause rather than a parenthetical one. This calls for the use of "that" 
rather
than "which," and the removal of a comma:

    When the answerer receives an offer that contains an 'm' line
    describing a BFCP stream, the answerer MUST check whether it supports

---------------------------------------------------------------------------

§10.4:

 >  o  If the BFCP stream is carried on top of TCP, and if the offerer  >     does not want to re-establish an existing TCP connection, the  >     offerer MUST associate an SDP connection attribute with an  >     'existing' value, with the 'm' line; and

Nit: remove quotes around "existing", and add quotes around "connection".

/a