Re: [apps-discuss] Appsdir review of draft-camarillo-rai-media-policy-dataset-01

[Gonzalo Camarillo <[e-mail]>]

Hi Henry,

thanks for your review. It is great that somebody with a good
understanding of XML has reviewed the document. My answers inline:

On 31/05/2012 4:13 PM, Henry S. Thompson wrote:
> I have been selected as the Applications Area Directorate reviewer for
> this draft (for background on appsdir, please see
> http://trac.tools.ietf.org/area/app/trac/wiki/ApplicationsAreaDirectorate).
> 
> Please resolve these comments along with any other Last Call comments
> you may receive. Please wait for direction from your document shepherd
> or AD before posting a new version of the draft.
> 
> Document: draft-camarillo-rai-media-policy-dataset-01
> Title: A User Agent Profile Data Set for Media Policy
> Reviewer: Henry S. Thompson
> Review Date: 31 May 2012
> IETF Last Call Date: 2012-06-11
> 
> Summary: This draft is almost ready for publication as a Proposed
>          Standard but has a few issues that should be fixed
>          before publication
> 
> Major Issues: 
> 
> 1 A bit more context is needed, given that section 3 dives right in to
> defining attributes.  We need to know Why this XML format
> is _needed_ -- why aren't session definitions (per RFC4566)
> sufficient?  Just a brief explanation of motivation should do the job.

The first paragraph of the Introduction references the framework
document, which describes everything related to session policies in SIP.
All the background information needed to understand the whole mechanism
is there. This is actually a document bundle made up of three documents:
the framework, the event package, and the document format (i.e., the
document you reviewed).


> 5.3/4/5/6 "Multiple <media-types-allowed> elements MAY only be present
> in a container element if each applies to a different set of streams
> (e.g., one <media-types-allowed> element for incoming and one for
> outgoing streams)." -- How is this possible?  The only 'container
> element' allowed by the schema is <session-policy>.  How does an
> application know _which_ stream is meant to be constrained by _which_
> allowed set???  Simlarly for all the other allowed/excluded elts, and
> various max/min settings if they appear under <session-policy> rather
> than <session>.

Yes, Sections 5.3, 5.4, 5.5, 5.6, 6.3, 6.4, and 6.5 all contain a
similar paragraph. Those elements can be present in a <session-policy>
container element, and they can have a direction attribute. If there is
more than one element (e.g., more than one <media-types-allowed> element
within a <session-policy> element), they will be distinguished by their
direction attribute. That is, one <media-types-allowed> element will
apply to outgoing media streams (i.e., sendonly direction attribute) and
the other will apply to incoming media streams (i.e., recvonly direction
attribute):

http://tools.ietf.org/html/draft-camarillo-rai-media-policy-dataset-01#section-3.3.2


> 6.5 max-stream-bw: It's not clear if it's an error == "MUST ignore" if
> the media-type attribute and the label attribute conflict, i.e. the
> stream picked out by the label isn't for the named media.  In a
> similar vein, it's not clear to what extent merging is restricted to
> pairs whose label and/or media match. . .

As usual, this draft specifies what to do in the most common error
situations but does not describe every possible error that may occur.
This error will not typically occur because if the bandwidth limit
applies to a given stream, it will simply have a label attribute, and
not a media-type attribute. The media-type attribute is used when the
bandwidth limit applies, in general, to all streams of a given media
type (e.g., to all audio streams). Therefore, both attributes are not be
used together.


> Similar issues arise for <qos-dscp>.

Same as above.


> Minor Issues: 
> 
> 3.1 It's a bit odd to reference XML 3rd edition -- XML 5e [1] has been
> available for over 3 years now. . .

I have updated the reference per your suggestion.


> 3.3.4 'media-type' attribute -- Are more specific values than those
> listed allowed?  Probably the most common understanding of the phrase
> 'media type' is exemplified by text/plain, image/svg or
> application/xml -- if these are _not_ allowed, say so.  If they are,
> include one or two in the "such as" list.

In the SIP context, media types are understood as defined in SDP spec.
Accordingly, that is how this draft uses them:

http://tools.ietf.org/html/rfc4566#section-8.2.1


> 4.1 Wrt codecs, the two MUST statements wrt description
> vs. offer/answer are superficially contradictory.  This para. needs to
> be split into two sub-cases, so the processing for single
> descr. vs. pair of descrs is clear.

Good point. I have rephrased the paragraph so that the handling of both
cases is explained separately and explicitly.


> 4.1 It would be nice to see two generic templates demonstrating the
> mapping rules for the one- and two-description cases, i.e. something
> along the lines of
> 
>    c=IN IP4 h                       <session-info>
>    m=m1 p1 RTP/AVP c11 c12 . . .      <streams>
>    a=label:l1                           <stream label="l1">
>    a=rtpmap:c11 t11		          <media-type>m1</media-type>
>    a=rtpmap:c12 t12		          <codec q="0.9">
>    a=rtpmap:. . .		           <media-type-subtype>t11</media-type-subtype>
>    m=m2 p2 RTP/AVP c21 c22 . . .          </codec>
>    a=label:l2			          <codec q="0.8">
>    a=rtpmap:c21 t21		           <media-type-subtype>t12</media-type-subtype>
>    a=rtpmap:c22 t22		          </codec>
>    a=rtpmap:. . .		          . . .
>    . . .			          <local-host-port>h:p1</local-host-port>
> 				        </stream>
> 				        <stream label="l2">
> 				          <media-type>m2</media-type>
> 				          <codec q="0.9">
> 				           <media-type-subtype>t21</media-type-subtype>
> 				          </codec>
> 				          <codec q="0.8">
> 				           <media-type-subtype>t22</media-type-subtype>
> 				          </codec>
> 				          . . .
> 				          <local-host-port>h:p2</local-host-port>
> 				        </stream>
> 				        . . .
> 				      </streams>
>                                     </session-info>
> 
> and similarly for the two-input case.

Section 7.2 already contains both examples.


> 4.1 The fact that the specified mapping is lossy in both directions
> should be called out, e.g. v= and t= lines are lost in one direction,
> and <context> is lost in the other.

I have added the following text:

      Note
      that these mapping rules do not include rules for all elements
      that need to be present in a session info document or in an SDP
      description. That is, some of those elements are generated
      following their associated general rules (e.g., the general
      rules to generate SDP v= and t= lines).


> 5.1 Wouldn't this be better at the _end_ of section 5, once the
> elements to be merged have been introduced?

It may be a bit clearer, but I would like to avoid this type of
structural changes at this point if possible.


> 5.1.2
> 
>     The two </codecs> end tags should be
>     </codecs-excluded> and </codecs-allowed> respectively.

Fixed.


> 7.1 The <session-policy> element needs
>           xmlns="urn:ietf:params:xml:ns:mediadataset"

Fixed.


>     The end tags </media-types> and </codecs> should be
>     </media-types-allowed> and </codecs-excluded> respectively.

Fixed.


> 7.2 Both <session-info> elements need
>           xmlns="urn:ietf:params:xml:ns:mediadataset"

Fixed.


>     All the <codec> elements need 'q' attributes according to the
>     text.  The schema does not require this.  Shouldn't it do so?

<codec> elements need to have 'q' attributes when they are used within
an <stream> element. However, when they are used in other context (e.g.,
within a <codecs-excluded> element), they do not need to. So, the schema
should not require. Nevertheless, you are right that some of the
examples were missing the 'q' attributes. I have fixed them.


> 8 Schema
> 
> ElementInfoContext is undefined.  I presume something along the lines
> of 
> 
>            <define name="ContextModel">
>              <interleave>
>                    <optional>
>                      <element name="info">
>                        <data type="string" />
>                      </element>
>                    </optional>
>                     <optional>
>                     <element name="policy-server-URI">
>                        <data type="string" />
>                      </element>
>                    </optional>
>                     <optional>
>                     <element name="token">
>                        <data type="token" />
>                      </element>
>                    </optional>
>                     <zeroOrMore>
>                      <element name="contact">
>                         <data type="string" />
>                      </element>
>                    </zeroOrMore>
>              </interleave>
>            </define>
> 
>            <define name="ElementContext">
>                <element name="context">
>                    <interleave>
>                     <ref name="ContextModel"/>
>                    </interleave>
>                </element>
>            </define>
> 
>            <define name="ElementInfoContext">
>             <element name="context">
>              <interleave>
>               <ref name="ContextModel"/>
>               <optional>
> 		<element name="request-URI">
> 		  <data type="string" />
> 		</element>
> 	       </optional>
>              </interleave>
>             </element>
>            </define>
> 
> is what you meant, if I read the text correctly.

Actually, we were supposed to remove that element from this version of
the draft but failed to do so. Now I have removed it. Thanks for
catching this.

> With the above fixes to schema and instances, the examples are all valid.
> 
> Nits:
> 
> 3.1 "namespace URIs for schemas" --> "namespace URIs for elements", I
> would prefer. . .

Fixed.


> 4 "<session-info><\session-info>" --> "<session-info></session-info>"

Fixed.


> 5.1.2 "encoding of profile of the codec" --> "encoding or profile of
>                                               the codec"
>       [I guess. . .]

Fixed.


>  -----------
>       "Other encoding or profiles of the same codec are unaffected." -->
>       "Other encodings or profiles of the same codec are unaffected."
>  -----------

Fixed.


>       "apply all containers it has received one after the other the
>        set of media- types/codecs it supports." -->
> 
>       "apply all containers it has received one after the other to the
>        set of media- types/codecs it supports."
> 
>       [I guess. . .]

Fixed.


> ht
>
> [1] http://www.w3.org/TR/2008/REC-xml-20081126/

I have also added you name to the Acknowledgments Section.

I have just submitted a new version of the draft that includes all your
comments above:

http://www.ietf.org/id/draft-camarillo-rai-media-policy-dataset-02.txt

Thanks,

Gonzalo