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

[Alan Ford <[e-mail]>]

Hi Charles, all,

Inline…

> On 7 Aug 2017, at 19:56, Charles Eckel (eckelcu) <[e-mail]> wrote:
> 
>  
> From: Alan Ford <[e-mail] <mailto:[e-mail]>>
> Date: Monday, August 7, 2017 at 1:37 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, 
>  
> Further comments inline…
>  
>> On 3 Aug 2017, at 23:19, Charles Eckel (eckelcu) <[e-mail] <mailto:[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.
>  
> [cue] Yes, you and Christer are correct about the backward compatibility part. I had it wrong.
>  
> 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.
>  
> [cue] Section includes this example:
>  
>    The following is an example of a 'floorctrl' attribute in an offer.
>    When this attribute appears in an answer, it only carries one role:
>  
>              a=floorctrl:c-only s-only c-s
>  
> Do you think it would help if we added another role to one of the offers in the examples in section 12?
> e.g. 
>    The following is an example of an offer sent by a conference server
>    to a client.
>  
>    m=application 50000 TCP/TLS/BFCP *
>    a=setup:passive
>    a=connection:new
>    a=fingerprint:sha-256 \
>         19:E2:1C:3B:4B:9F:81:E6:B8:5C:F4:A5:A8:D8:73:04: \
>         BB:05:2F:70:9F:04:A9:0E:05:E9:26:33:E8:70:88:A2
>    a=floorctrl:c-only s-only
>  

Yes, I think that would be beneficial. I would also propose adding the following sentence directly after the ABNF in S4.1: “The offer will include one or more roles in its offer; the answerer MUST include only one role”. (The section then goes on to describe why… But this is probably the top issue in BFCP interop I want to see it totally clear). It may also be worth adding a final sentence that “If the answerer does not support any corresponding role, it MUST zero-port the m-line”.

>> 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.
>  
> [cue] I see now. If “label” is required it seems you need at least one “mstrm”, right?
> So how about if we change as follows:
>  
> OLD : floor-id-attribute = "a=floorid:" token [" mstrm:" token *(SP token)]
> NEW: floor-id-attribute = "a=floorid:" token " mstrm:" token [*(SP token)]
> OLD: It defines a floor identifier and, possibly, associates it with one or more media streams.
> NEW: It defines a floor identifier and associates it with one or more media streams.
>  
> Although I likely botched the ABNF.

My issue is not so much that this is permitted without mstrm:, just that it is inconsistent.

I am not familiar with a scenario where it would be valid without mstrm: … If there is one I have no problem with this being permitted, but then I would suggest changing “A floor control server acting as an offerer or as an answerer MUST include these attributes in its session descriptions.” to be “A floor control server acting as an offerer or as an answerer MUST include the `floorid` attribute, and the `label` attribute if using the `mstrm` parameter, in its session descriptions.”.

If there must always be a mstrm: , then your proposal would be the appropriate fix.

One or the other - at the moment I think the text is a little confusing.

Regards,
Alan