Re: [AVTCORE] I-D Action: draft-ietf-avtcore-clksrc-05.txt

[Magnus Westerlund <[e-mail]>]

WG,

I have reviewed the -05 version and found some minor issues that I hope
the authors will be able to address in the update in progress anyway to
resolve the SDP directorate review. The Offer/answer section looks
acceptable even if I yet again needed to read it twice to make certain
that all cases are covered. The default statements in 6.1 and 6.1.1 I
have a tendency to overlook when I read 6.1.2 and 6.1.3.

1. Section 3:

   source level  : Source level information applies to a RTP media
      stream Source-Specific Media Attributes in the Session Description
      Protocol (SDP) [RFC5576] defines how source-level information is
      included into an SDP session description.


I think this definition looks a bit strange I would propose two
modifications to it to make it be clearer:

NEW:

   source level  : Source level information applies to a specific RTP
                                                         ^^^^^^^^
      media stream.  Source-Specific Media Attributes in the Session
                  ^^
      Description Protocol (SDP) [RFC5576] defines how source-level
      information is included into an SDP session description.


2. Section 4.8 and 5.4:

The ABNF actually is underspecified by lacking definitions. I run it in
BAP (http://tools.ietf.org/tools/bap/abnf.cgi) and got the following
response.

; integer UNDEFINED
; SP UNDEFINED
mediaclk-master = "a=ssrc:" integer SP clk-master-id
clk-master-id = "mediaclk:master-id=" master-id
timestamp-mediaclk-sl = "a:ssrc" integer SP timestamp-mediaclk
timestamp-mediaclk = "a=mediaclk:" mediaclock
mediaclock = sender / refclk / streamid / mediaclock-ext
sender = "sender" sender-ext
; token UNDEFINED
sender-ext = token
; DIGIT UNDEFINED
refclk = "direct" [ "=" 1*DIGIT ] [ rate ] [ direct-ext ]
rate = [ SP "rate=" integer "/" integer ]
direct-ext = token
streamid = ( "master-id=" master-id
streamid = ) / ( "IEEE1722=" avb-stream-id
streamid = ) / streamid-ext
master-id = EUI48
avb-stream-id = EUI64
; HEXDIG UNDEFINED
EUI48 = 5( 2HEXDIG ":" ) 2HEXDIG
EUI64 = 7( 2HEXDIG ":" ) 2HEXDIG
streamid-ext = token
; byte-string UNDEFINED
mediaclock-ext = token [ SP byte-string ]
; mediaclk-master defined but not used
; timestamp-mediaclk-sl defined but not used

As you see this results in a number of undefined labels. This needs to
be resolved. They belong to two categories:

A) The ones defined in RFC 5234. These could be easily resolved. The
most explicit way is to do the following and avoids forking definitions:

SP     = <See RFC 5234>
DIGIT  = <See RFC 5234>
HEXDIG = <See RFC 5234>

B) The second categories are more uncertain and which of there are three
labels:
integer, token and byte-string.

I guess token and byte-string are the definitions from SDP RFC 4566.
Thus they could be solved by:

token = <See RFC 4566>
byte-string = <See RFC 4566>

The integer may also be the definition from RFC 4566, however as the
only usage are in the a=ssrc attribute, I would recommend that one
actually uses the formal grammer from RFC 5576, i.e.:

   ssrc-attr = "ssrc:" ssrc-id SP attribute
   ; The base definition of "attribute" is in RFC 4566.
   ; (It is the content of "a=" lines.)

   ssrc-id = integer ; 0 .. 2**32 - 1

Thus I would suggest that one changes integer to "ssrc-id" and add
ssrc-id = <See RFC 5576>.

I think this will resolve all the issues, but please run the resulting
ABNF through BAP to check that you only get unused indications from the
top level constructs intended and no other indications.

You might have to make changes to this depending on how you are
resolving the SDP directorate review.

The above also applies to the ABNF in Section 5.4.


3. Section 4.8:

Then I am also not a fan of open definitions like the port one:
port = 1*DIGIT

This really means at least 1 Digit up to an infinite number of them. I
do prefer to set some upper limit on it. So far I don't know of a
protocol that uses more than 16-bit port numbers, although that could
change in the future. Considering what is deployed today I think the
following is much more appropriate:

port = 1*5DIGIT ; 0-65535

Or if you want to be more future safe, then
port = 1*10DIGIT
This allows for 32-bit integers



4. Section 5.2:
   The signalling optionally indicates a media clock offset value.  The
   offset indicates the RTP timestamp value at the epoch (time of
   origin) of the reference clock.  If no offset is signalled, the
   offset can be inferred at the receiver by examining RTCP sender
   reports which contain NTP and RTP timestamps which combined define a
   mapping.

This text actually raises two questions with me.

First, I am not certain that I understand the offset. So the indicated
value is an RTP timestamp, i.e. the integer representation of the 32-bit
value of the RTP timestamp. So the epoch time is dependent on the clock
system, but the current epoch for an NTP reference clock would be 1 jan
at 0:00 year 1900. Thus you need to take all the leap seconds etc into
account to determine how many time has actually passed since then and
then calculate how many wrapping of the timestamp this means. This seems
error prone, at least without being more explicit of how to do it for
the various reference clocks.

Secondly, what are the relative precedence between the SDP signaled
value and any values received in RTCP SR? Is it an initial date point
that then are replaced by any RTCP SR information? If not, does it
blocks the usage of the RTCP SR to update the relation between the "NTP"
field and the RTP TS value? This needs to be made more explicit.

5. Section 5.2 and Figure 7:

   A rate modifier may be specified.  The modifier is expressed as the
   ratio of two integers and modifies the rate specified or implied by
   the media description by this ratio.  If omitted, the rate is assumed
   to be the exact rate specified or implied by the media format.  For
   example, without a rate specification, the media clock for an 8 kHz
   G.711 audio stream will advance exactly 8000 units for each second
   advance in the reference clock from which it is derived.

and looking at this example in Figure 7:

          m=audio 5004 RTP/AVP 96
          a=rtpmap:96 L24/44100/2
          a=sendonly
          a=ts-refclk:ptp=IEEE1588-2008:39-A7-94-FF-FE-07-CB-D0:0
          a=mediaclk:direct=963214424 rate=1000/1001

I realize that I don't understand what the rate modifier means. Without
the rate modifier there would be 44100 RTP timestamp ticks per second of
reference clock. Does the rate modifier multiply with the nominal rate
so that this example means 441000*1000/1001 RTP TS ticks per second of
reference clock?

Some clarification may be required here.

6. Section 5.3:

   In a given system, master clock identifiers must be unique.  Such
   identifiers MAY be manually configured, however 17 octet string
   identifiers SHOULD be generated according to the "short-term
   persistent RTCP CNAME" algorithm as described in RFC6222 [RFC6222].

As draft-ietf-avtcore-6222bis are approved and will soon be published, I
think you should update this reference.
Please also check that the update of RFC 6222 does not cause any issue
for this.

7. Section 8.3:
When creating registries it is quite common to make it clear what
information  is the minimal required in a registration request.
Looking at these registries I would assume any registration would be
required to provide:

- Value
- Long name
- Reference
- Contact for registration

Maybe you should make this explicit.


8. Section 8.5.2:

   The source-level SDP attribute "master-id" defined by this document
   is registered with the IANA registry of SDP Parameters as follows:

   SDP Attribute ("att-field (source level)"):

     Attribute name:     master-id

     Long form:          Media clock source

     Type of name:       att-field

     Type of attribute:  source level

     Subject to charset: No

     Purpose:            See section 5 of this document

     Reference:          This document

     Values:             EUI48


I wonder if this part is needed at all. What I can read to in the
specification all source level usage with the master ID will have it
encapsulated in the mediaclk attribute, i.e. one will specify
a=ssrc:12345678 mediaclk:master-id=<foobar>

rather than

a=ssrc:12345678 master-id=<foobar>

Thus I wonder why there is a registation of master-id?

9. Section 9:

I wonder if not the classificiation of normative references are a bit
wrong. From my reading of this doc, RFC 3261 is not a normative
reference as the only statement related to SIP is the statement that SIP
supports renogitation and that may be used, but really are just an example.

Then I think the clock system/transports which has identifiers in this
specification are actual normative references, so RFC 5905, IEEE 1588-X
etc are in the wrong category.

Cheers

Magnus Westerlund

----------------------------------------------------------------------
Multimedia Technologies, Ericsson Research EAB/TVM
----------------------------------------------------------------------
Ericsson AB                | Phone  [phone]
Färögatan 6                | Mobile [phone]
SE-164 80 Stockholm, Sweden| mailto: [e-mail]
----------------------------------------------------------------------