Re: [bfcpbis] WGLC for draft-ietf-bfcpbis-rfc4583bis

[Alan Ford <[e-mail]>]

Hi Charles, all,

Further comments inline…

> On 3 Aug 2017, at 23:19, Charles Eckel (eckelcu) <[e-mail]> wrote:
> 
> Please see inline [cue]
>  
> From: Alan Ford <[e-mail] <mailto:[e-mail]>>
> Date: Sunday, July 23, 2017 at 1:01 AM
> To: Charles Eckel <[e-mail] <mailto:[e-mail]>>
> Cc: "[e-mail] <mailto:[e-mail]>" <[e-mail] <mailto:[e-mail]>>, Tom Kristensen <[e-mail] <mailto:[e-mail]>>, Roman Shpount <[e-mail] <mailto:[e-mail]>>, Gonzalo Camarillo <[e-mail] <mailto:[e-mail]>>, Christer Holmberg <[e-mail] <mailto:[e-mail]>>
> Subject: Re: [bfcpbis] WGLC for draft-ietf-bfcpbis-rfc4583bis
>  
> Hi Charles, all, 
>  
> S3
>  
> - TCP transport indent is hard to parse. Suggest changing sentence to "Depending on the value of the 'setup' attribute (discussed in Section 8.1), the port field contains either the port to which the remote endpoint will direct BFCP messages, or in the case where the endpoint will initiate the connection towards the remote endpoint, should be set to a value of 9."
>  
> [cue] works for me
>  
>  
> S4.1
>  
> - "The offerer includes this attribute to state all the roles it would be willing to perform”: add “once negotiation is complete”, for clarity.
>  
> [cue] good idea
>  
> - Here, plus S11.1 and S11.2, floorctrl is optional with inferred state of c-only for an offerer if it is not specified. I would advocate taking this opportunity to get rid of ambiguity and making the presence of floorctrl a MUST. Everyone does it already, but it’s one of the most problematic fields for interop as it is.
>  
> - Also, given in an answer only one role is present, but in an offer multiple can be present, can this be clearer in the ABNF? Is it possible to specify two ABNFs, one for the offer and one for the answer (the answer being without the *)? If that’s not possible, then could we at least emphasise this point near the start of the section (again coming at this from too many interop issues in this area).
>  
> [cue] Christer seemed to agree with you. I have been a proponent of not breaking backward compatibility with RFC 4583 unnecessarily. Anyone from the original RFC 4583 days able to shed some light on this?

I agree with Christer here - it’s not a break in backward compatibility to mandate it from a offerer's point of view in the new spec, since this is all which is changing. An old answerer can cope. The only issue is a new-spec-compliant answerer with an old receiver. To be fully belt-and-braces here, the receiver should be able to cope without one and infer it, but the sender MUST always send it.

BTW just to flag up my second point there was unrelated to the first, which you answered… I’m not sure how best to present this information but I think it’s important it’s clearer near the ABNF definition that multiple roles are possible in an offer.


> S6
>  
> - “…identified by an SDP label attribute”. For consistency, please put ‘label’ in single quotes.
>  
> [cue] good idea
>  
> - Why is mstrm: optional in the ABNF, and optional in the paragraph beginning “The floorid attribute…”, yet is mandatory in the “Endpoints that use the offer/answer…” section? If there is a genuine reason for it to be optional please specify this, however I think it would be easier just to make it mandatory.
>  
> [cue] The floorid is defined such that one can specify a floorid yet not associate it with any media stream, thus the mstrm is optional. This is how it was defined in RFC 4583. I am not sure if anyone makes use of this, but that is how it was defined.

It would be useful to understand what the use case is for this, but whether or not it’s required the text is still inconsistent. "It defines a floor identifier and, possibly, associates it with one or more media streams.” makes it sound optional, whereas "A floor control server acting as an offerer or as an answerer MUST include these attributes in its session descriptions.” says “these attributes” which I took to mean both label and floorid, and once you’re including label the inference is that mstrm: should be added.

I think it would be beneficial to clarify one way or the other. Either mstrm: is a MUST, or clarify why and how it could be used optionally.



>  
> S11
>  
> Is is required to have a BUNDLE table like this now? (I presume given draft-ietf-mmusic-sdp-mux-attributes-16 is in MISSREF it’s too late to make the updates there). As Christer mentioned, It think this would be better in S7. OR, a separate subsection under S11 for “BUNDLE Considerations” that makes it clearer that this is an update to draft-ietf-mmusic-sdp-mux-attributes-16.
>  
> [cue] agreed
>  
> I assume we don’t want to go down the rabbit hole of defining BUNDLE for BFCP at this stage? :-)  Since if we did, it probably wouldn’t be too hard - you can demux on confid + userid.
>  
> [cue] We decided previously to leave BUNDLE out of scope. I am happy to keep it that way.
>  
>  
> S14.2
>  
> The “Allowed attribute values” here is inconsistent compared to others by being ABNF rather than just “A token” or “Tokens”. It’s also wrong - it implies only one value, not multiple as is the case. Suggest either fixing the ABNF or saying “one or more of…” or similar.
>  
> [cue] This is how it was defined in RFC 4583. As I understand it, “1*” means it requires at least one, so can have multiple.
> 

Quite right, brain fart :)

Regards,
Alan