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