Subject: Re: [payload] WGLC for draft-ietf-payload-rtp-opus-04

From: "Timothy B. Terriberry" <[email-masked]>
Date: Wed, 03 Dec 2014 07:27:34 -0800

Ali C. Begen (abegen) wrote:
> This is to start WGLC for the opus draft.
> https://datatracker.ietf.org/doc/draft-ietf-payload-rtp-opus/
>
> The WGLC will end Dec. 1st. Please review the draft and send an email to the list even if you dont have any comments.

Passing along some comments from Martin Thomson (who's not subscribed to 
payload):

---

This document looks like it's in pretty good shape, but I have a
surprisingly large number of small nits.  None of these really rise to
the level of issues, though I think a couple do need to be addressed
before progressing.

Section 2.1 doesn't need an entire section; and the table seems to
have two captions.

The definition of fullband (FB) appears to have an error.  Either the
bandwidth is 24kHz, or the sample rate is 40kHz, or something, but
Nyquist's Theorum suggests one or other needs to give.  The supposed
upper limit on human hearing isn't really relevant for this case.

I'm also not sure that the table needs to suggest that bandwidth is a range.

Section 3.1 s|Opus supports all bitrates from 6 kb/s to 510 kb/s|Opus
supports bitrates from 6 kb/s to 510 kb/s|
  (at this level of detail, the fact that this is continuous and not
discrete isn't that relevant and the implication that there are no
bitrates outside this range would be unfortunate)

Section 3.2 (complexity) should note that this applies to the encoder.
I'm assuming that the choices are far more limited on the decoder
side.

Section 3.3 "whether the receiving decoder has indicated it can take
advantage of "in-band" FEC information" might benefit from an
informative reference on how this signal might be conveyed.

"The receiver can then
    configure its decoder to decode the FEC data from the packet rather
    than the regular audio data."  might be better as "instead of
performing loss concealing for a missing packet, the receiver can then
    configure its decoder to decode the FEC data from the next packet."
  And drop the following sentence.

Unlike stereo, it's not clear whether a decoder is capable of decoding
FEC packets.  I'd assume that they have to not choke if they are
present.  Might be worth pointing out.

Section 3.4 "Any implementation of the Opus decoder MUST be capable of
receiving stereo signals" -- isn't this something that RFC 6716 says?
Rather than a normative statement here, simply state this as fact: "An
Opus decoder is capable of handling a stereo encoding, but an
application might only be capable of consuming a single audio
channel."  Then the SHOULD regarding don't send if you know it's not
being used.

Section 4.1 "The payload MAY be padded by an integer number of octets
according to [RFC3550]." probably needs to mention that Opus padding
is preferred.

"The RTP payload type for Opus has not been assigned statically and is
expected to be assigned dynamically." -- this isn't an expectation,
it's required, at least for the RTP profiles currently in existence.
Can this be made more definite?

Table 2 is a little insulting.  The text is sufficient (I would have
been even more sparse, noting that the sample rate can be dynamically
adjusted on a per frame basis and that timestamps increment at a
constant 48kHz anyway).

Section 4.2, Figure 1: s/Payload/Packet (use consistent terminology)

Section 6.1, sprop-maxcapturerate: Please use "audio bandwidth" here
rather than "bandwidth" to avoid confusion.

m(ax|in)ptime: does this "The decoder MUST be capable of accepting any
allowed packet sizes to ensure maximum compatibility." imply that if I
set a maxptime of 10, I have to deal with 20 anyway?  What then is the
point of the attribute?

maxaveragebitrate: in SDP this is going to replicate b= functionality;
has that been discussed by the working group?  And how is this value
measured?  Does it include the RTP header, does it include headers at
lower layers of the stack as well?  This needs more definition.

no sprop-cbr for senders that would prefer resistance against traffic 
analysis?

"Gateways or senders that are sending the same encoded audio to
multiple destinations SHOULD NOT use an audio bandwidth higher than
necessary to represent audio sampled at "maxplaybackrate", [...]"
should be "[...] sampled at the highest current value of
"maxplaybackrate" of recipients" or something like that.  Similar
language doesn't exist for the bitrate, but might be necessary as
well.

Section 7: The Opus payload format does not have any built-in security
mechanisms." ... so padding doesn't count?

Section 8 can be removed.

---

From: Jean-Marc Valin <[email-masked]>
Date: Mon, 08 Dec 2014 14:33:24 -0500

Here's a new version that should address Martin's comments below.

> Section 2.1 doesn't need an entire section; and the table seems to 

Now all in section 2.

> The definition of fullband (FB) appears to have an error.  Either
> the bandwidth is 24kHz, or the sample rate is 40kHz, or something,
> but Nyquist's Theorum suggests one or other needs to give.  The
> supposed upper limit on human hearing isn't really relevant for
> this case.
> 

Defined "audio bandwidth" to be the range of frequencies, i.e. not
just the Nyquist rate.

> Section 3.1 s|Opus supports all bitrates from 6 kb/s to 510
> kb/s|Opus supports bitrates from 6 kb/s to 510 kb/s| (at this level
> of detail, the fact that this is continuous and not discrete isn't
> that relevant and the implication that there are no bitrates
> outside this range would be unfortunate)

Fixed.

> Section 3.2 (complexity) should note that this applies to the
> encoder. I'm assuming that the choices are far more limited on the
> decoder side.

Done.

> Section 3.3 "whether the receiving decoder has indicated it can
> take advantage of "in-band" FEC information" might benefit from an 
> "The receiver can then configure its decoder to decode the FEC data
> from the packet rather than the regular audio data."  might be
> better as "instead of performing loss concealing for a missing
> packet, the receiver can then configure its decoder to decode the
> FEC data from the next packet." And drop the following sentence.

Change applied.

> Unlike stereo, it's not clear whether a decoder is capable of
> decoding FEC packets.  I'd assume that they have to not choke if
> they are present.  Might be worth pointing out.

Added text.

> Section 3.4 "Any implementation of the Opus decoder MUST be capable
> of receiving stereo signals" -- isn't this something that RFC 6716
> says? Rather than a normative statement here, simply state this as
> fact: "An Opus decoder is capable of handling a stereo encoding,
> but an application might only be capable of consuming a single
> audio channel."  Then the SHOULD regarding don't send if you know
> it's not being used.

Fixed.

> Section 4.1 "The payload MAY be padded by an integer number of
> octets according to [RFC3550]." probably needs to mention that Opus
> padding is preferred.

Section 3.1.2 already says that Opus padding is preferred, but it is
now repeated here (without 2119 keywords to avoid potential confusion).

> "The RTP payload type for Opus has not been assigned statically and
> is expected to be assigned dynamically." -- this isn't an
> expectation, it's required, at least for the RTP profiles currently
> in existence. Can this be made more definite?

Done

> Table 2 is a little insulting.  The text is sufficient (I would
> have been even more sparse, noting that the sample rate can be
> dynamically adjusted on a per frame basis and that timestamps
> increment at a constant 48kHz anyway).

Table 2 removed.

> Section 4.2, Figure 1: s/Payload/Packet (use consistent

Done.

> Section 6.1, sprop-maxcapturerate: Please use "audio bandwidth"
> here rather than "bandwidth" to avoid confusion.

This name has been subject to much discussion and I'd rather not
re-open that.

> m(ax|in)ptime: does this "The decoder MUST be capable of accepting
> any allowed packet sizes to ensure maximum compatibility." imply
> that if I set a maxptime of 10, I have to deal with 20 anyway?
> What then is the point of the attribute?

As discussed with Martin, the idea here wasn't to change how any of
the ptime are normally handled, so I removed all 2119 keywords and
just defer to RFC. Similarly, minptime was removed since it's not
defined in RFC4566.

> maxaveragebitrate: in SDP this is going to replicate b=
> functionality; has that been discussed by the working group?  And
> how is this value measured?  Does it include the RTP header, does
> it include headers at lower layers of the stack as well?  This
> needs more definition.

As discussed with Martin too, maxaveragebitrate wasn't really
providing anything useful over b=AS, so it's been removed.

> no sprop-cbr for senders that would prefer resistance against
> traffic analysis?

sprop-cbr is not needed since the encoder can unilaterally decide to
keep the bitrate constant.

> multiple destinations SHOULD NOT use an audio bandwidth higher
> than necessary to represent audio sampled at "maxplaybackrate",
> [...]" should be "[...] sampled at the highest current value of 

We have tried avoiding "sampled at" terminology because with Opus
audio bandwidth does not necessarily follow the actual sampling rate
(which is often just 48 kHz internally).

> Section 7: The Opus payload format does not have any built-in
> security mechanisms." ... so padding doesn't count?

Rephrased to "Opus does not provide any confidentiality or integrity
protection."

> Section 8 can be removed.

Added actually acknowledgments instead.

---

From: Jonathan Lennox <[email-masked]>
Date: Tue, 9 Dec 2014 21:13:04 +0000

I’ve reviewed the just-published version -05, it looks good to me.

---

From: "Ali C. Begen (abegen)" <[email-masked]>
Date: Wed, 10 Dec 2014 05:36:18 +0000

Thanks for the update. I will try to finish my chair review soon and then we will ship this.

-acbegen

> On Dec 8, 2014, at 9:33 PM, Jean-Marc Valin <[email-masked]> wrote:
> 
> _______________________________________________
> payload mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/payload