Subject: Re: [bfcpbis] Draft new version: draft-ietf-bfcpbis-rfc4583bis-19 - Alan's comments

From: Christer Holmberg <[email-masked]>
Date: Fri, 23 Mar 2018 16:10:03 +0000

Hi Alan,

Thank You for your comments! Please see inline.

>Great stuff! The new splits and the rewording really make the document much easier to follow.

Thank You! :)

A few concerns/comments:

> 1) Section 5 now features the word “stream” standalone in three places:
>
>   The attribute is used to determine the floor control role for a given
>   stream ( represented by an SDP media description).
>…
>   An endpoint includes the attribute to indicate the role(s) it would
>   be willing to perform for the stream associated with the attribute:
>…
>   Endpoints compliant with [16] might not include the 'floorctrl'
>   attribute in offers and answerer.  For a given stream, if the
>   'floorctrl' attribute is not present the offerer will act as floor
>   control client and the answerer will act as floor control server.
>
> For clarity’s sake, can you make these three say “BFCP stream”, to ensure it’s not causing confusion with associated media streams.

The attribute is not used to negotiate the role for the BFCP stream, but the role(s) the endpoints can take for individual BFCP-controlled streams.

However, when looking at text, it may need a little more changes. See pull request.

---

>Section 8 also suffers from two new instances of this; all other instances of “stream” that I can find are refined appropriately.

Correct. 

Also, as this applies to the BFCP stream, we don't need the "For a given stream" part.

OLD: 
    
    "For a given stream, if the
   'bfcpver' attribute is not present, the default values are inferred
   from the transport specified in the 'm' line (Section 3) associated
   with the stream."

NEW:

    "If the 'bfcpver' attribute is not present, the default values are inferred
   from the transport specified in the 'm' line (Section 3) associated
   with the BFCP stream."

---

>2) Going back to my concern over:
>
>   floor-id-attribute = "a=floorid:" token [" mstrm:" token *(SP token)]
>
>I notice that in S13.1, we say:
>
>   o  MUST associate an SDP 'label' attribute (Section 7), with the 'm'
>      line; and
>
> Given a label is a MUST, then it infers that the mstrm attribute must be a MUST, so should not be optional in the ABNF above, i.e.
>
>   floor-id-attribute = "a=floorid:" token " mstrm:" token *(SP token)

If usage of mstrm is always mandated in the procdures then I agree it should be mandatory also in the ABNF.

I will fix as suggested.

---

>2a) The text is weird for that MUST anyway; in both 13.1 and 13.2 we say:
>
>   o  MUST associate an SDP 'label' attribute (Section 7), with the 'm'
>      line; and
>
> But the SDP label attribute is for the media stream not the BFCP stream.

Correct.

The text shall say:

"o  MUST associate an SDP 'label' attribute (Section 7), with the 'm' line of each BFCP-controlled data stream; and"

>   o  MUST associate an SDP 'label' attribute (Section 7), with the 'm'
>      line of the associated media stream(s); and
>
> And for clarity, every other one should probably be changed from: ‘m’ line -> ‘m’ line of the BFCP stream

Section 13 does say:

"The procedures apply to an 'm' line describing a BFCP stream.", so we don't need to repeat it in every instance.

But, as the 'label' attribute is for other m lines, the text above could be modified to:

"Unless explicitly stated otherwise, the procedures apply to an 'm' line describing a BFCP stream."

---

>3) In Section 5, can we refine to 2119 language, i.e.
>
>   An offerer can indicate multiple attribute value for a given session.
>   An answerer can only indicate one attribute value.
>
>Change to:
>
>   An offerer MAY indicate multiple attribute value for a given session.
>   An answerer MUST indicate only one attribute value.

I commented on this. To me it is unclear whether "MUST" means that one MUST include the attribute, or that one MUST include the attribute and MUST indicate only one attribute value.

I suggest:

    "When inserted in an offer, the offerer MAY indicate multiple attribute values. 
     When inserted in an answer, the answerer MUST indicate only one attribute value."

I have created a pull request with the suggested changes:

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

Regards,

Christer

---

From: Alan Ford <[email-masked]>
Date: Fri, 23 Mar 2018 22:09:58 +0000

Thanks for the clarifications, I think we’ve converged on something very clear now - the PR looks good.

Cheers,
Alan

> On 23 Mar 2018, at 16:10, Christer Holmberg <[email-masked]> wrote:
> 
> Hi Alan,
>

---

From: Christer Holmberg <[email-masked]>
Date: Sat, 24 Mar 2018 07:08:16 +0000

Thanks!

I will wait for comments from others, then I will merge the pull request and submit a new version of the draft.

Regards,

Christer

-----Original Message-----
From: Alan Ford [mailto:[email-masked]] 
Sent: 24 March 2018 00:10
To: Christer Holmberg <[email-masked]>
Cc: Charles Eckel (eckelcu) <[email-masked]>; [email-masked]
Subject: Re: [bfcpbis] Draft new version: draft-ietf-bfcpbis-rfc4583bis-19 - Alan's comments

Cheers,
Alan

>>  The attribute is used to determine the floor control role for a 
>> given  stream ( represented by an SDP media description).
>> …

---

From: Victor Pascual Avila <[email-masked]>
Date: Sat, 24 Mar 2018 08:46:38 +0100

fine with me

On Sat, Mar 24, 2018 at 8:08 AM, Christer Holmberg <
[email-masked]> wrote:

> >> For clarity’s sake, can you make these three say “BFCP stream”, to
> ensure it’s not causing confusion with associated media streams.
> >
> > The attribute is not used to negotiate the role for the BFCP stream, but
> the role(s) the endpoints can take for individual BFCP-controlled streams.
> >
> >> Section 8 also suffers from two new instances of this; all other
> instances of “stream” that I can find are refined appropriately.
> >
> > Correct.
> >
> >> Given a label is a MUST, then it infers that the mstrm attribute must
> be a MUST, so should not be optional in the ABNF above, i.e.
> >>
> > "o  MUST associate an SDP 'label' attribute (Section 7), with the 'm'
> line of each BFCP-controlled data stream; and"
> >
> > "The procedures apply to an 'm' line describing a BFCP stream.", so we
> don't need to repeat it in every instance.
> >
> > I commented on this. To me it is unclear whether "MUST" means that one
> MUST include the attribute, or that one MUST include the attribute and MUST
> indicate only one attribute value.
> >
> > I suggest:
> >
> _______________________________________________
> bfcpbis mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/bfcpbis
>

-- 
Victor Pascual Ávila