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

From: Jari Arkko <[email-masked]>
Date: Wed, 29 May 2013 22:51:03 +0300

Thank you Alexey very much for the review, and Simo for the updates. I have balloted No-Objection.

Jari

On Feb 28, 2013, at 12:48 PM, [email-masked] wrote:

> 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
> 
> On 28/02/2013 07:31, [email-masked] wrote:
>> Hello Alexey,
> Hi Simo,
> Thank you for the response.
>> 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.
>> 
>> 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?
>> 
>> [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".
> I think this would be much better!
> 
> [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.
> 
>> 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?
>> 
>> [SV] See above.
> 
> [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
> 
>>     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.
>> 
>>  [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.
>> 
>> Similar text in section 5.6.2
>> 
>>     but MUST NOT specify values for those
>>     subfields.
> Looks good.
>> 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.
>> 
>> [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.
> Looks good.
>> 
>> 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 ..."
>> 
>> [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:
>> 
>> 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
>> 
>> [SV]. Fixed.
>> 
>>     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.
>> 
>> [SV] Added comment lines in ABNF that CRLF and DIGIT are defined in RFC5234 (there was already a reference to RFC534 for HEXDIG).
> Thanks.
>> 
>>             ;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
>> 
>> Regards,
>> Simo
>> 
> 
> 
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art