Subject: [Gen-art] Gen-ART LC review of draft-ietf-mmusic-sdp-cs-17

From: Alexey Melnikov <[email-masked]>
Date: Tue, 12 Feb 2013 13:20:08 +0000

I am the assigned Gen-ART reviewer for this draft. For background on 
Gen-ART, please see the FAQ at 
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments 
you may receive.

Document: draft-ietf-mmusic-sdp-cs-17
Reviewer: Alexey Melnikov
Review Date: 2013-02-12
IETF LC End Date: 2013-02-01
IESG Telechat date: N/A

Summary: This draft is ready for publication as a Standard Track RFC 
(with some nits/minor issues):

Major issues:

Minor issues:

5.2.2.  Media Descriptions

    When "RTP/AVP" is used in the <proto> field, the <fmt> subfield
    contains the RTP payload type numbers.  We use the <fmt> subfield to
    indicate the list of available codecs over the circuit-switched
    bearer, by re-using the conventions and payload type numbers defined
    for RTP/AVP.  The RTP audio and video media types, which, when
    applied to PSTN circuit-switched bearers, represent merely an audio
    or video codec.  The endpoint SHOULD only use those payload type
    whose corresponding codecs is available for PSTN media streams.

Why is this a SHOULD? What are possible reasons for violating it?

5.6.1.  Generating the Initial Offer

    The <fmt> subfield carries the payload type number(s) the endpoint is
    wishing to use.  Payload type numbers in this case refer to the
    codecs that the endpoint wishes to use on the PSTN media stream. For
    example, if the endpoint wishes to use the GSM codec, it would add
    payload type number 3 in the list of codecs.  The list of payload
    types SHOULD only contain those codecs the endpoint is able to use on
    the PSTN bearer.

Again, a SHOULD similar to the one above. Can you please explain the 
reasoning for it?

    If the Offerer is only able to become the passive party in the
    circuit-switched bearer setup, it MUST add the "callerid", "uuie"
    and/or "dtmf" subfields,

This text is a bit awkward as you specify this field as extensible. So I 
think you need to add a choice for extensions here.

Similar text in section 5.6.2

    but MUST NOT specify values for those
    subfields.

5.6.2.  Generating the Answer

    If the Answerer becomes the active party, it MUST add any of the
    "callerid", "uuie" or "dtmf" subfield values.

8.3.  Registration of new "addrtype" values

    Note to IANA: The current "addrtype" sub-registry structure does not
    capture the fact that a given addrtype is defined in the context of a
    particular "nettype".  The sub-registry structure should be to
    correct that as part of this registration.

Did you mean "should be corrected as part ..."

Nits/editorial comments:

5.3.3.  Considerations on correlations

    Passive endpoints should expect an incoming CS call for setting up
    the audio bearer.  Passive endpoints MAY suppress the incoming CS
    alert during a certain time periods.  Additional restrictions can be
    applied, such as the passive endpoint not alerting incoming calls
    originated from the number that was observed uduring the offer/answer

Typo: during

    negotiation.

5.7.  Formal Syntax

    The following is the formal Augmented Backus-Naur Form (ABNF)
    [RFC5234] syntax that supports the extensions defined in this
    specification.  The syntax is built above the SDP [RFC4566] and the
    tel URI [RFC3966] grammars.  Implementations according to this
    specification MUST be compliant with this syntax.

    Figure 2 shows the formal syntax of the extensions defined in this
    memo.

            ; extension to the connection field originally specified
            ; in RFC 4566

            connection-field   =  [%x63 "=" nettype SP addrtype SP
            connection-address CRLF]
            ;nettype and addrtype are defined in RFC 4566

            connection-address /=  global-number-digits / "-"
            ; global-number-digits specified in RFC 3966

This is good, but you don't specify where CRLF, HEXDIG are defined. I 
can guess, but it would be better if a reader doesn't have to guess.

            ;subrules for correlation attribute
            attribute          /= cs-correlation-attr
            ; attribute defined in RFC 4566
            cs-correlation-attr = "cs-correlation:" corr-mechanisms
            corr-mechanisms    = corr-mech *(SP corr-mech)
            corr-mech          = caller-id-mech / uuie-mech /
                                 dtmf-mech / ext-mech
            caller-id-mech     = "callerid" [":" caller-id-value]
            caller-id-value    = "+" 1*15DIGIT
            uuie-mech          = "uuie" [":" uuie-value]
            uuie-value         = 1*65(HEXDIG HEXDIG)
                                 ;This represents up to 130 HEXDIG
                                 ; (65 octets)
                                 ;HEXDIG defined in RFC5234
                                 ;HEXDIG defined as 0-9, A-F
            dtmf-mech          = "dtmf" [":" dtmf-value]
            dtmf-value         = 1*32(DIGIT / %x41-44 / %x23 / %x2A )
            ;0-9, A-D, '#' and '*'
            ext-mech           = ext-mech-name [":" ext-mech-value]
            ext-mech-name      = token
            ext-mech-value     = token
            ; token is specified in RFC4566

---

From: <[email-masked]>
Date: Thu, 28 Feb 2013 07:31:07 +0000

Hello Alexey,

Thank you for the review. Please find my answers inline [SV].

-----Original Message-----
From: ext Alexey Melnikov [mailto:[email-masked]] 
Sent: 12. helmikuuta 2013 15:20
To: [email-masked]
Cc: [email-masked]; The IESG
Subject: Gen-ART LC review of draft-ietf-mmusic-sdp-cs-17

I am the assigned Gen-ART reviewer for this draft. For background on Gen-ART, please see the FAQ at <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments you may receive.

Summary: This draft is ready for publication as a Standard Track RFC (with some nits/minor issues):

Major issues:

Minor issues:

[SV] This is clarified in the following paragraph:

   In some cases, the endpoint is not able to determine the list of
   available codecs for circuit-switched media streams.  In this case,
   in order to be syntactically compliant with SDP [RFC4566], the
   endpoint MUST include a single dash ("-") in the <fmt> subfield.

[SV] Other option is to say that in cases where the endpoint is able to determine the RTP payload numbers, it MUST use those, and in cases when it is not, it MUST use the "dash".

Again, a SHOULD similar to the one above. Can you please explain the reasoning for it?

[SV] See above.

 [SV] We changed the text to read:

	    If the Offerer is only able to become the passive party in
	    the circuit-switched bearer setup, it MUST add at least
	    one of the possible correlation mechanisms, but MUST NOT
	    specify values for those subfields.

[SV] Changed the text in 5.6.2 to read

	 If the Answerer becomes the active party, it MUST 
	 add a value to any of the possible subfields.
	  
	  If the Answerer becomes the passive party, it MUST NOT add
	  any values to the subfields in the "cs-correlation" attribute.

[SV] We discussed this Note to IANA with the ADs, and as a result the note will be removed (and no context will be added for addrtype values in the IANA registry).

Nits/editorial comments:

Typo: during

[SV]. Fixed.

    negotiation.

5.7.  Formal Syntax

[SV] Added comment lines in ABNF that CRLF and DIGIT are defined in RFC5234 (there was already a reference to RFC534 for HEXDIG).

Regards,
Simo

---

From: Alexey Melnikov <[email-masked]>
Date: Thu, 28 Feb 2013 10:36:15 +0000

> Hello Alexey,
Hi Simo,
Thank you for the response.
> [SV] Other option is to say that in cases where the endpoint is able to determine the RTP payload numbers, it MUST use those, and in cases when it is not, it MUST use the "dash".
I think this would be much better!
>      but MUST NOT specify values for those
Looks good.
> 	  any values to the subfields in the "cs-correlation" attribute.
Looks good.
> [SV] Added comment lines in ABNF that CRLF and DIGIT are defined in RFC5234 (there was already a reference to RFC534 for HEXDIG).
Thanks.

---

From: <[email-masked]>
Date: Thu, 28 Feb 2013 10:48:30 +0000

Below one comment [SV].

-----Original Message-----
From: ext Alexey Melnikov [mailto:[email-masked]] 
Sent: 28 February, 2013 12:36
To: Veikkolainen Simo (Nokia-CTO/Espoo)
Cc: [email-masked]; [email-masked]; [email-masked]
Subject: Re: Gen-ART LC review of draft-ietf-mmusic-sdp-cs-17

Hi Simo,

[SV] Ok, we'll change the text as:

     If the endpoint is able to determine the list of available codecs for circuit-switched 
    media streams, it MUST use the corresponding payload type numbers in the <fmt>
    subfield.

> Again, a SHOULD similar to the one above. Can you please explain the reasoning for it?

[SV] In this paragraph, I think it is sufficient just to replace the "SHOULD" with "MUST", i.e.  "The list of payload types MUST only contain..."

Regards,
Simo

> This text is a bit awkward as you specify this field as extensible. So 
> I think you need to add a choice for extensions here.
>
Looks good.
Looks good.
Thanks.