Subject: [payload] AD review of draft-ietf-payload-g7110-02

From: Richard Barnes <[email-masked]>
Date: Mon, 12 May 2014 22:01:18 +0200

I have reviewed this draft in preparation for IETF LC.  I have some
concerns below that I would like to see addressed before LC (MAJOR).

Thanks,
--Richard

MAJOR:

S4.2.3.
H2 is a little unclear.  Is the array indexing zero-based?  If so, why are
you skipping the first octet?  If you mean it to be zero-based, and not to
skip the first octet, set P=-1 in H1.

S4.2.3.
What happens in H5 if the G.711.0 decoder encounters an improperly
formatted octet sequence?  E.g., if the prefix code octet specifies that
there are more samples than could be contained in the packet.  I'm
concerned that the process as-is could lead to reading past the end of the
packet buffer.

S4.2.4.
What happens if the M is not a multiple of N?  Does the receiver discard
the packet?

MINOR:

S4.2.3.
The "heuristic" seems more like an "algorithm".  Or "process", as the
section header says.

S5.1.
Is there any risk of G.7110 being defined, and needing a payload format
that would collide with this identifier?

S5.1.
Can we specify that "channels" is an integer?

S5.4.*
Can someone verify that the spaces in the examples are valid?  E.g., "complaw
= al" and "ptime: 20".

S6.
It seems like the format described here can only hold single-channel,
right?  If so, this format seems fine, but you do need to clarify that it's
single-channel only. If not, it seems like you need to (1) delimit packets,
and (2) specify the number of channels at the start.

EDITORIAL:

S4.2.4.
"the optional parameter is a MUST" -> "the optional 'channels' parameter is
REQUIRED"

S4.2.3.
It seems like it would be simpler to state the decoding algorithm as while
loop with an if statement, since it seems like that's how you would
implement it in most normal programming languages.  (See, e.g., the python
snippet below).  If you really want a state machine like representation, it
would be nice to at least have a picture of it.

      +-----------+
      |           |
      V           |
H1 -> H2 -> H3 -> H4 -> STOP
      ^     |            ^
      |     |            |
      |     +---> H5 ----+
      |           |
      |           |
      +-----------+

def decode_g711_0_payload(payload):
  # H1
  P = 0
  N = len(payload)
  internal = []
  output = []
  while P < N:
    # H2
    internal = packet[ P+1 : P+1+min(321,N-P) ]
    # H3
    if internal[0] == '\x00':
      # H4
      P += 1
    else:
      # H5
      (symbols, Q) = g711_0_decode(internal)
      output.extend(symbols)
      K += len(symbols) # == M
      P += Q
  return output

---

From: "Michael Ramalho (mramalho)" <[email-masked]>
Date: Tue, 27 May 2014 02:23:42 +0000

Richard,

My apologies, I didn’t see your email.

I had a death in the family on May 10 (Saturday) and I wasn’t made aware of your email (which you sent the Monday afterwards) until recently.

My comments are below (prefaced with “MAR:”). I have copied the co-authors on the draft for their commentary (they can comment via email separately).

Thank you very much for your very thorough review. It was my belief that the LC ended, but I can see value in incorporating some of your comments.

Regards,

Michael Ramalho

From: payload [mailto:[email-masked]] On Behalf Of Richard Barnes
Sent: Monday, May 12, 2014 4:01 PM
To: [email-masked]; [email-masked]
Subject: [payload] AD review of draft-ietf-payload-g7110-02

I have reviewed this draft in preparation for IETF LC.  I have some concerns below that I would like to see addressed before LC (MAJOR).

Thanks,
--Richard

MAJOR:

S4.2.3.
H2 is a little unclear.  Is the array indexing zero-based?  If so, why are you skipping the first octet?  If you mean it to be zero-based, and not to skip the first octet, set P=-1 in H1.

MAR: Noboru may have another way to address your point (and can comment in another email), but P, K and N are counters as defined in the previous paragraphs – not array indexes.

MAR: H2 says to read the minimum of 321 or (N-P) octets into an internal buffer from the (P+1)st octet. Since P starts at zero in H1, we never skip the first octet; on the first pass the first octet is placed in the internal buffer.

S4.2.3.
What happens in H5 if the G.711.0 decoder encounters an improperly formatted octet sequence?  E.g., if the prefix code octet specifies that there are more samples than could be contained in the packet.  I'm concerned that the process as-is could lead to reading past the end of the packet buffer.

MAR: For a given pass through H5, the internal buffer will at most have all the remaining octets in the RTP payload by virtue of the “Read min {321,(N-P)} octets” in H2. Stated another way, there is no way the internal buffer can have more octets than was originally in the RTP payload.

MAR: If the ITU-T specified decoder could (due to bit error) read past the end of the data intended in the internal buffer, it would decode the (apparently) random data after that to the end of the internal buffer into something – but that something would be of limited length by virtue of the decoding process. This point is also made in the Security Considerations section – that data posing as a desired G.711.0 payload could not be turned into a denial-of-service attack (as the G.711.0 processing has a worst case processing path and completes in a finite amount of processing, no loops, etc.).

MAR: If we really, really, really want to ensure that the G.711.0 decoder didn’t even read junk in the internal buffer past the intended G.711.0 data – we could modify the “Read min {321,(N-P)} octexts” in H2 to explicitly state that any unused octets in the 321 byte internal buffer after the read must be set to 0x00 (which would be guaranteed not to produce any unintended G.711 source symbols). We could easily do this by modifying H2 to read:

Read internal buffer: “Set all octets in the internal buffer to zero (0x00), then read min(321,(N-P)} octexts  into the internal buffer from the (P+1) octet in the RTP Payload”.

MAR: I would defer to Noboru here.

S4.2.4.
What happens if the M is not a multiple of N?  Does the receiver discard the packet?

MAR: Good point. Of course this should never happen with a correctly formatted payload, but it wouldn’t hurt to explicitly mention that it is probably wise to discard the packet.

MAR: As an aside, you should have reviewed RFC 3551 – as it is silent on the point of what happens if there aren’t enough samples to complete a sample-based encoding (Section 4.3).

MAR: Assuming my co-authors and WG chairs agree, we can draft such a statement.

MINOR:

S4.2.3.
The "heuristic" seems more like an "algorithm".  Or "process", as the section header says.

MAR: If my co-authors and WG chairs agree, I would be happy to change heuristic to process at all relevant occurrences.

S5.1.
Is there any risk of G.7110 being defined, and needing a payload format that would collide with this identifier?

MAR: Paul Jones (ITU-T SG16 Rapporteur and co-author here) can comment … but the G-series numbering ran out a while ago and they began to label things with a “dot” nomenclature (e.g., G.722.1). Thus I doubt that there will be a G.7110 standard in the G series for media codecs.

S5.1.

MAR: I have no issue, but I can’t find any mention in RFC4566 or any other RTP format definition that explicitly mentions non-integer channels.

MAR: “5.1 surround” isn’t really 5.1 channels ;-) ).

S5.4.*
Can someone verify that the spaces in the examples are valid?  E.g., "complaw = al" and "ptime: 20".

S6.
It seems like the format described here can only hold single-channel, right?  If so, this format seems fine, but you do need to clarify that it's single-channel only. If not, it seems like you need to (1) delimit packets, and (2) specify the number of channels at the start.

MAR: Yes, the storage mode defined in Section 6.3 with version octet “0x00” only supports a single channel.

MAR: As the text describes, if there is a future need for other formats to support things like multiple channels or metadata … they can be defined elsewhere (in another IETF document or in another SDO) with a version number other than “0x00”.

EDITORIAL:

S4.2.4.
"the optional parameter is a MUST" -> "the optional 'channels' parameter is REQUIRED"

MAR: Good catch.

S4.2.3.
It seems like it would be simpler to state the decoding algorithm as while loop with an if statement, since it seems like that's how you would implement it in most normal programming languages.  (See, e.g., the python snippet below).  If you really want a state machine like representation, it would be nice to at least have a picture of it.

      +-----------+
      |           |
      V           |

MAR: I wrote the prose more or less to mimic the words in the ITU-T G.711.0 specification.

MAR: However, I would have no issue with a picture or pseudo code. I completely defer to Paul Jones or Noboru Harada here.

MAR: My most sincere thanks for your through review.

---

From: "Paul E. Jones" <[email-masked]>
Date: Wed, 28 May 2014 05:09:30 +0000

Guys,

Comments to items left for me:

>MAR: Paul Jones (ITU-T SG16 Rapporteur and co-author here) can comment 
>… but the G-series numbering ran out a while ago and they began to 
>label things with a “dot” nomenclature (e.g., G.722.1). Thus I doubt 
>that there will be a G.7110 standard in the G series for media codecs.
>

I would not say the number space ran out, as the ITU can (and does) 
number documents using more than 3 digits.  There are documents like 
G.7713.1, for example.  Would there ever be a G.7110?  I cannot dismiss 
that as a possibility, but it's highly unlikely the ITU would elect to 
use that number to refer to a voice codec, as it would make it difficult 
to speak about that codec without confusing some people with G.711.0.  
So not only would it be an issue for the IANA registration, but for 
basic human conversation.

But even if the number was re-used for a voice codec, this is an IANA 
registration and the IETF/IANA control the conflict management.  I doubt 
this will ever be an issue, but if it is one day far into the future, 
the conflict will be addressed in the IETF and, therefore, not an issue 
that cannot be easily resolved.

>S4.2.3.
>
>It seems like it would be simpler to state the decoding algorithm as 
>while loop with an if statement, since it seems like that's how you 
>would implement it in most normal programming languages.  (See, e.g., 
>the python snippet below).  If you really want a state machine like 
>representation, it would be nice to at least have a picture of it.
>
>
>
>       +-----------+
>
>       |           |
>
>       V           |
>
>MAR: However, I would have no issue with a picture or pseudo code. I 
>completely defer to Paul Jones or Noboru Harada here.
>
>
>

Having some form of pseudo code is nice, but I'm not expert in Python, 
so I had to work about as hard to digest this a I did the prose.  I have 
no objection to inserting python code, but we should have a python 
person double-check it.  I do have some concern (noted below) with the 
code clearly aligning with the steps.  Python allows for nice 
"shortcuts" not possible in C, for example.

I noted that in the python code that "K" was not defined.  I think that 
should be set to 0 to avoid an error. That said, I don't think K is even 
needed in Python, the output string is just concatenated with whatever 
symbols that are returned (i.e., output.extend(symbols)).  Taking this 
short cut, though, makes it even more challenging to align the logic 
with the description.

Again, not being a python expert, I have to question this line:

First, shouldn't this be "payload", rather than "packet"?

The spec says to "read min{320+1, (N-P)} octets into the internal buffer 
from the (P+1) octet of the RTP payload".

However, I believe that python indexes strings in such a way that, if I 
have the string hello="Hello" and I want the first two octets, I have to 
use the hello[0:2].  That suggests that the array indices to read the 
data from the payload would need to be [ P : P+min(321,N-P) ].  I think. 
  Initially, P is zero and the initial read needs to read the first byte, 
so I think the first index would be just "P".  The second index would be 
P + ever how many octets we will read this time.  I think.  Please 
correct me if I'm wrong.

Michael, I noted that Q is defined as "the number of octets contained in 
the G.711.0 frame being processed", but then H5 says this:

"The G.711.0 decoder will have consumed some number of packets, Q, in 
the internal buffer to produce the M G.711 symbols."

I think "packets" there should be octets.

Paul

---

From: "Michael Ramalho (mramalho)" <[email-masked]>
Date: Wed, 28 May 2014 12:16:58 +0000

Re: "The G.711.0 decoder will have consumed some number of packets, Q, in the internal buffer to produce the M G.711 symbols. …I think "packets" there should be octets.”

Yes, you are correct Paul. Good catch (I’d read that a thousand times without noticing it).

Ali/Roni: Should I make another pass through the document, or wait until the post-LC editing process to incorporate these relatively easy edits (all but the possible inclusion of the Python code are easy/trivial)?

I personally don’t think the Python code is essential, because the prose in the draft mimics existing reference code in the ITU-T standard (if someone wanted to look at code they could look at real operational code there). But I wouldn’t object to it being included in an Appendix of the payload format draft either (which is where I think it would most logically belong if we desire to include it).

Michael Ramalho

From: Paul E. Jones [mailto:[email-masked]]
Sent: Wednesday, May 28, 2014 1:09 AM
To: Michael Ramalho (mramalho); Richard Barnes; [email-masked]; [email-masked]
Cc: Paul Jones (paulej); [email-masked]; Miaolei ([email-masked]); Muthu Arul Mozhi Perumal (mperumal); Ali C. Begen (abegen); [email-masked]; Roni Even
Subject: Re: [payload] AD review of draft-ietf-payload-g7110-02

Guys,

S5.1.

I would not say the number space ran out, as the ITU can (and does) number documents using more than 3 digits.  There are documents like G.7713.1, for example.  Would there ever be a G.7110?  I cannot dismiss that as a possibility, but it's highly unlikely the ITU would elect to use that number to refer to a voice codec, as it would make it difficult to speak about that codec without confusing some people with G.711.0.  So not only would it be an issue for the IANA registration, but for basic human conversation.

But even if the number was re-used for a voice codec, this is an IANA registration and the IETF/IANA control the conflict management.  I doubt this will ever be an issue, but if it is one day far into the future, the conflict will be addressed in the IETF and, therefore, not an issue that cannot be easily resolved.

S4.2.3.

      +-----------+
      |           |
      V           |

Having some form of pseudo code is nice, but I'm notexpert in Python, so I had to work about as hard to digest this a I did the prose.  I have no objection to inserting python code, but we should have a python person double-check it.  I do have some concern (noted below) with the code clearly aligning with the steps.  Python allows for nice "shortcuts" not possible in C, for example.

I noted that in the python code that "K" was not defined. I think that should be set to 0 to avoid an error. That said, I don't think K is even needed in Python, the output string is just concatenated with whatever symbols that are returned (i.e., output.extend(symbols)).  Taking this short cut, though, makes it even more challenging to align the logic with the description.

The spec says to "read min{320+1, (N-P)} octets into the internal buffer from the (P+1) octet of the RTP payload".

However, I believe that python indexes strings in such a way that, if I have the string hello="Hello" and I want the first two octets, I have to use the hello[0:2]. That suggests that the array indices to read the data from the payload would need to be [ P : P+min(321,N-P) ].  I think.  Initially, P is zero and the initial read needs to read the first byte, so I think the first index would be just "P".  The second index would be P + ever how many octets we will read this time.  I think.  Please correct me if I'm wrong.

Michael, I noted that Q is defined as "the number of octets contained in the G.711.0 frame being processed", but then H5 says this:

"The G.711.0 decoder will have consumed some number of packets, Q, in the internal buffer to produce the M G.711 symbols."

Paul