Subject: [codec] Review of draft-ietf-codec-oggopus-08

From: "Mo Zanaty (mzanaty)" <[email-masked]>
Date: Sun, 15 Nov 2015 03:14:43 +0000

Authors,

I have reviewed draft-ietf-codec-oggopus-08 and have the following comments. The technical content has been well reviewed already, so these comments are mostly for addressing issues that may arise during IESG review.

General
- Both "muxer/demuxer" and "encoder/decoder" appear in normative statements. I would expect RFC 6716 to fully specify Opus encoder/decoder behavior, and this spec to only specify muxer/demuxer behavior. Please check all "encoder/decoder" instances to see if you really mean "muxer/demuxer". If you really mean "encoder/decoder", is the behavior really restricted to Opus only when used in Ogg? If not, shouldn't this update RFC 6716?

Abstract
- The first 2 sentences seem sufficient. Consider moving the rest to the Introduction, since it merely highlights Ogg features.

Section 3. Packet Organization
- 2nd paragraph: The second sentence (on granule position) should be moved to the next section (4. Granule Position).
- 4th paragraph: "The second packet in the logical Ogg bitstream MUST contain the comment header" ... then later ... "It MAY span one or more pages" ... so shouldn't this be MUST not MAY since zero is not allowed, i.e. the comment header is mandatory not optional.
- 8th paragraph: "The first audio data page SHOULD NOT have the 'continued packet' flag set" Why SHOULD NOT rather than MUST NOT? When would it be ok to set this flag on the first audio data page?
- 9th paragraph: Same as above, s/SHOULD/MUST/g. It seems you may be taking the demuxer view here. I think you need to specify strict normative behavior for muxers but allow some lenience in demuxers as already stated in that paragraph: "but implementations need to be prepared to deal with..."

Section 4. Granule Position
- This should start with stating that the granule position MUST be zero for the ID header page and the comment header completion page, but MAY be larger than zero for the first audio data completion page as described in section 4.5. This critical info should be together here instead of spread across other sections.

Section 4.1. Repairing Gaps in Real-time Streams
- 1st paragraph: "a muxer SHOULD emit packets that explicitly request the use of Packet Loss Concealment (PLC) in place of the missing packets." Why SHOULD not MUST? The prior paragraph uses MUST and says "For this to work, there cannot be any gaps." If you want to keep it as SHOULD strength, perhaps state that muxers which fail to do this will cause demuxers to compute incorrect granule positions when seeking forward or backward.
- 3rd paragraph: RFC6716 reference is broken.
- I assume this section does not apply to RFC 7587 (Opus RTP) because in the RTP case, RTP timestamp signals the gap (if using DTX), while Ogg uses the granule position. Correct? If not, should any of this section apply to RFC 7587 and therefore update it?

Section 4.2. Pre-skip
- Same as above, does it apply to RFC 7587?

Section 5. Header Packets
- "An Opus stream" -> "An Ogg Opus logical stream". Otherwise it may be confused with the underlying Opus stream itself containing these headers.

Section 5.1. Identification Header
- 1. to 8. Why *asterisk* around field names? Markdown/bold conversion failure?
- 8. Channel Mapping Table: "It is omitted when the channel mapping family is 0, but REQUIRED otherwise." Be normative in both 0 and >0 cases: "It MUST be omitted...0, but REQUIRED otherwise." Also in the next paragraph.
- Last sentence: "However, implementations MAY reject streams in which the ID header does not complete on the first page." Seems like this should be MUST not MAY based on section 3 which clearly requires this: "MUST complete on that [first] page." Or perhaps you need to specify muxer MUST vs. demuxer MAY. 

Section 5.1.1. Channel Mapping
- 1. to 3. Why *asterisk* again?
- 2. Coupled Stream Count: "...the first M Opus decoders are to be initialized for stereo output..." Is this an intended restriction that all stereo channels must appear first before any mono channels?
- 3. Channel Mapping: "...If 'index' is 2*M or larger, but less than 255, the output MUST be taken from decoding stream ('index'-M) as mono." Should be index-2*M not index-M.
- Channel and stream are used interchangeably, as well as stereo and coupled, stereo and 2-channel, mono and 1-channel. Different meanings in different contexts can cause confusion. If possible, use the same term to mean the same thing in all contexts. If not possible, perhaps add a bit more text to clarify the nuances between streams, channels and stereo in different contexts.

Section 5.1.1.1. Channel Mapping Family 0
- Second sentence: "RTP mapping." Add reference to RFC 7587.

Section 5.1.1.2. Channel Mapping Family 1
- Last paragraph: "'LFE' here refers to a Low Frequency Effects," add "channel" before the comma. 

Section 8. Implementation Status
- Remove the reference to [1] and insert the URL directly here, so when the RFC Editor removes this entire section there will be no orphaned reference in section 14.3.

Section 14.3. URIs
- Remove this entire section per the above comment.

Section 9. Security Considerations
- "Malicious payloads MUST NOT cause the decoder to overrun its allocated memory or to take an excessive amount of resources to decode.  Although problems in encoders are typically rarer, the same applies to the encoder.  Malicious audio streams MUST NOT cause the encoder to misbehave..." Replace "misbehave" with the firmer decoder text about memory and resources.

Section 10. Content Type
- The reference to RFC 6381 should be changed to RFC 5334.
-- RFC 6381 defines the optional parameter "codecs=" for ISO MIME types (audio/mp4, video/mp4, etc.).
-- RFC 5334 defines "codecs=" for Ogg MIME types (audio/ogg, video/ogg, application/ogg).
-- Notably, the "codecs=" name space format differs between RFC 6381 (4 character code for ISO) vs. RFC 5334 (4-8 character codes are mapped to longer codec names for Ogg).
- This document should indicate it updates RFC 5334 to add "opus" to the "codecs=" parameter.
- This document should update the IANA Media Types registry to reference itself alongside RFC 5334 for application/ogg, audio/ogg and video/ogg.

Section 11. IANA Considerations
- This document should update the IANA Media Types registry per the above comment.

Section 13. Copying Conditions
- This is strange for IETF specs. Why?

Cheers,
Mo (as Document Shepherd)

---

From: Ralph Giles <[email-masked]>
Date: Tue, 17 Nov 2015 15:56:59 -0800

On 2015-11-14 7:14 PM, Mo Zanaty (mzanaty) wrote:

> I have reviewed draft-ietf-codec-oggopus-08 and have the following 
> comments.

Thanks for the review. Responses inline below.

> General - Both "muxer/demuxer" and "encoder/decoder" appear in 
> normative statements. I would expect RFC 6716 to fully specify
> Opus encoder/decoder behavior, and this spec to only specify
> muxer/demuxer behavior.

It's true that the draft uses encoder/decoder both to refer to an
implementation of RFC 6716 and more generally to and encoder/decoder
application. Such an application would include a codec encoder/decoder
as described by RFC 6716, an implementation of Ogg Opus logical
bitstream encapsulation/decapsulation (this draft) and an Ogg
muxer/demuxer (RFC 3533 plus Opus-specific additions from this draft).

I can change references to the more general application to
"muxer/demuxer" but that's no more precise. The draft struggles with
this elsewhere, referring to "players" and "non-playing decoders" when
offering guidance for direct audio playback or file-based output.
Overloading "encoder/decoder" was intended to be a generalization of tha
t.

Mark suggested 'an implementation (of this specification)' as a way to
disabiguate.

> Please check all "encoder/decoder" instances to see if you really
> mean "muxer/demuxer". If you really mean "encoder/decoder", is the
> behavior really restricted to Opus only when used in Ogg? If not, 
> shouldn't this update RFC 6716?

I don't think there's anything here which should update RFC 6716, given
the decision to make that document just about the audio compression
layer. Drafts describing encapsulation necessarily contain additional
constraints and techiniques on how to use an RFC 6716 encoder/decoder
for their particular application.

> Abstract - The first 2 sentences seem sufficient. Consider moving
> the rest to the Introduction, since it merely highlights Ogg
> features.

I think it's useful to include some motivation for why a draft is
interesting in the draft in the abstract. The overview of Ogg's features
is really just defining terms for the last paragraph. I'll see if I can
come up with something better.

> Section 3. Packet Organization - 2nd paragraph: The second
> sentence (on granule position) should be moved to the next section
> (4. Granule Position).

Done, although some re-writing was required to be clear about the
distinction between granulepos=0 and granulepos=-1 for pages with
header data, as you note below.

> - 4th paragraph: "The second packet in the logical Ogg bitstream
> MUST contain the comment header" ... then later ... "It MAY span
> one or more pages" ... so shouldn't this be MUST not MAY since zero
> is not allowed, i.e. the comment header is mandatory not optional.

How about, "It MAY span multiple pages, beginning on the second page of
the logical stream." Avoids the math/English ambiguity.

> - 8th paragraph: "The first audio data page SHOULD NOT have the
> 'continued packet' flag set" Why SHOULD NOT rather than MUST NOT? 
> When would it be ok to set this flag on the first audio data page?
> -

A naive editing application would generate such streams if it didn't
take care to strip the truncated packet data from the first page(s).
That's considerably more machinery than just cutting the stream at page
boundaries.

> 9th paragraph: Same as above, s/SHOULD/MUST/g. It seems you may be 
> taking the demuxer view here. I think you need to specify strict 
> normative behavior for muxers but allow some lenience in demuxers
> as already stated in that paragraph: "but implementations need to
> be prepared to deal with..."

Our intent was that both muxer and demuxer authors consider the
implications of streams which violate the SHOULD, which I think is
consistent with the RFC 2119 meaning of the term.

Saying "Muxers MUST to X. Demuxers MUST handle streams not compliant
with that MUST," is confusing without offering better precision.

> Section 4. Granule Position - This should start with stating that
> the granule position MUST be zero for the ID header page and the
> comment header completion page, but MAY be larger than zero for the
> first audio data completion page as described in section 4.5. This
> critical info should be together here instead of spread across
> other sections.

Ok.

> Section 4.1. Repairing Gaps in Real-time Streams - 1st paragraph:
> "a muxer SHOULD emit packets that explicitly request the use of
> Packet Loss Concealment (PLC) in place of the missing packets." Why
> SHOULD not MUST?

IIRC Tim wanted to allow implementations to emit zero-length packets
instead, updating the granulepos without generating packet data to
request PLC from the decoder.

A muxer following the SHOULD benefits naive players which feed packets
spanning the gap to the decoder blindly. More sophisticated players
would implement their own version of this muxer SHOULD to handle the gap
when the muxer did not.

> The prior paragraph uses MUST and says "For this to work, there
> cannot be any gaps." If you want to keep it as SHOULD strength, 
> perhaps state that muxers which fail to do this will cause
> demuxers to compute incorrect granule positions when seeking
> forward or backward.

> - 3rd paragraph: RFC6716 reference is broken.

I don't know why that reference didn't get a hyperlink in the
tools.ietf.org version. Bug with <xref> at the beginning of a <t>?
It's fine in the xml2rfc html output.

> - I assume this section does not apply to RFC 7587 (Opus RTP)
> because in the RTP case, RTP timestamp signals the gap (if using
> DTX), while Ogg uses the granule position. Correct? If not, should
> any of this section apply to RFC 7587 and therefore update it?

Correct. Ogg does not allow discontinuous transmission (or rather
treats it as damage) so it is helpful for muxers to fill the gap with
generated packets. RTP implementations are more likely to take
advantage of Opus FEC redundancy and have their own PLC algorithms to
deal with packet loss.

> Section 4.2. Pre-skip - Same as above, does it apply to RFC 7587?

RTP applications typically start playback with the first audio packet
received to minimize latency. I suppose the summary of encoder delay
could still be useful in that context, but there's no signalling
mechanism for the pre-skip field in RTP, and the primary use of
marking sample-accurate edit points isn't relevant there.

> Section 5. Header Packets - "An Opus stream" -> "An Ogg Opus
> logical stream". Otherwise it may be confused with the underlying
> Opus stream itself containing these headers.

Good point, thanks.

> Section 5.1. Identification Header - 1. to 8. Why *asterisk*
> around field names? Markdown/bold conversion failure?

This is what xml2rfc does with <spanx style="strong"> We added these
to improve readability in the html output. I thought the txt behavioru
was 'as intended' in the spirit of ASCII art.

I've removed the markup from the field headings, but left the inline
<spanx style="emph">not</spanx> => _not_ under the 'Input Sample Rate'
description as we feel emphasis is important there, and not just for
readability. Implementor feedback has included a lot of confusion on
this point.

> - 8. Channel Mapping Table: "It is omitted when the channel mapping
> family is 0, but REQUIRED otherwise." Be normative in both 0 and >0
> cases: "It MUST be omitted...0, but REQUIRED otherwise."

Thanks, fixed.

> Also in the next paragraph. - Last sentence: "However,
> implementations MAY reject streams in which the ID header does not
> complete on the first page." Seems like this should be MUST not MAY
> based on section 3 which clearly requires this: "MUST complete on
> that [first] page." Or perhaps you need to specify muxer MUST vs.
> demuxer MAY.

This one is clearly "demuxer MAY".

> Coupled Stream Count: "...the first M Opus decoders are to be 
> initialized for stereo output..." Is this an intended restriction 
> that all stereo channels must appear first before any mono
> channels?

Yes. That way we don't have to signal mono/stereo for each substream,
just how many of each we have.

> - 3. Channel Mapping: "...If 'index' is 2*M or larger, but less
> than 255, the output MUST be taken from decoding stream ('index'-M)
> as mono." Should be index-2*M not index-M.

Hmm. I think this is correct as written, and you've confused
streams/decoders with channels.

There are N Opus (sub)streams within each Ogg Opus logical stream,
each fed to one of the N corresponding Opus decoders. The first M of
those decoders are configured to output stereo (two decoder channels
each). The remaining N-M are configured to output mono (one decoder
channel each).

So there are 2*M + 1*(N-M) = M+N decoder channels available. The
channel mapping stable describes how to route these to the C output
channels.

For each output channel index from 0 to C-1, the channel mapping table
gives the index of the decoded channel to route to that output channel
index. These are implicitly stacked from 0..2*M-1 for the M stereo
decoders, and 2*M..M+N-1 for the N-M mono decoders. So while there are
M+N *channel* indicies, there are only N decoders. For indicies in the
mono range, we subtract 2*M to get the offset to the 0th mono decoder
output channel. But the index of the *decoder* is M + (index - 2*M) =
(index - M).

> - Channel and stream are used interchangeably, as well as stereo
> and coupled, stereo and 2-channel, mono and 1-channel. Different
> meanings in different contexts can cause confusion. If possible,
> use the same term to mean the same thing in all contexts. If not
> possible, perhaps add a bit more text to clarify the nuances
> between streams, channels and stereo in different contexts.

I'll see if I can clean this up a bit. There definitely aren't enough
distinct terms for all the layers here.

> Section 5.1.1.1. Channel Mapping Family 0 - Second sentence: "RTP 
> mapping." Add reference to RFC 7587.

Done.

> Section 5.1.1.2. Channel Mapping Family 1 - Last paragraph: "'LFE' 
> here refers to a Low Frequency Effects," add "channel" before the 
> comma.

Ok.

> Section 8. Implementation Status - Remove the reference to [1] and 
> insert the URL directly here, so when the RFC Editor removes this 
> entire section there will be no orphaned reference in section
> 14.3. ... Section 14.3. URIs - Remove this entire section per the
> above comment.

I couldn't figure out how to do this in xml2rfc, which generates the
reference entries. I've added removing the reference entry to the
rfc-editor request.

> Section 9. Security Considerations - "Malicious payloads MUST NOT 
> cause the decoder to overrun its allocated memory or to take an 
> excessive amount of resources to decode.  Although problems in 
> encoders are typically rarer, the same applies to the encoder. 
> Malicious audio streams MUST NOT cause the encoder to
> misbehave..." Replace "misbehave" with the firmer decoder text
> about memory and resources.

Ok.

> Section 10. Content Type - The reference to RFC 6381 should be 
> changed to RFC 5334.

RFC 5334 cites RFC 4281 for the definition of the "codecs=" mime-type
extension. RFC 6381 says it obsoletes RFC 4281, and defines the codecs
parameter as well as the extensions for mp4.

Can we cite both RFC 6381 (for the format) and RFC 5334 (for
ogg-specifics) here?

> -- Notably, the "codecs=" name space format differs between RFC
> 6381 (4 character code for ISO) vs. RFC 5334 (4-8 character codes
> are mapped to longer codec names for Ogg).

My BNF is rusty, but it looks like RFC 6381 defines "codecs="
generically, in a separate section from the ISO Base Media Format
Namespace.

> to the "codecs=" parameter.

Ok. We can say that.

Note that the table in RFC 5334 section 4 isn't intended to be
complete and instead cites
https://wiki.xiph.org/index.php/MIMETypesCodecs which has already been
updated.

 - This document should update the IANA Media
> Types registry to reference itself alongside RFC 5334 for 
> application/ogg, audio/ogg and video/ogg.

Indeed. Do I just write that it does, and magic happens? RFC 6838 is a
little vague on what's expected of document authors.

> Cheers, Mo (as Document Shepherd)

Whew! Thanks again for all the comments. I'll ask my co-authors if
they agree with the changes and publish an updated draft.

 -r

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Tue, 17 Nov 2015 17:41:34 -0800

> Mark suggested 'an implementation (of this specification)' as a way to

I've never been particularly happy with our usage of encoder/decoder 
(without a definition, even, though to be clear I'm responsible for the 
current state of this text, since I wrote it). Mark's suggestion sounds 
like a solid improvement to me.

> for their particular application.

I agree here. RFC 6716 describes a "one frame in, one frame out" 
encoder/decoder. A complete encoder application has to deal with audio 
at the sample level, and potentially with multiple streams, which is the 
source of most of the additional requirements.

> come up with something better.

And in particular the reason I added them was to motivate why someone 
might need this draft over and above RFC 6716. But I've no objection to 
cutting them if you don't manage to come up with something better.

> the logical stream." Avoids the math/English ambiguity.

That captures the intent (i.e., that the header *might* span more than 
one page). Works for me.

> with that MUST," is confusing without offering better precision.

I agree. C.f., 
<https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00>, I don't 
think we should add a muxer MUST unless we're prepared to have demuxers 
actually fail on said streams.

> request PLC from the decoder.

No, it's impossible to compute the duration of a zero-byte packet, since 
it has no TOC sequence, and this breaks lots of things. The "SHOULD" was 
because there are other alternatives which generate valid streams, but 
less than optimal results, such as simply not incrementing the timestamp 
for the lost packets. That leads to desync if there's video, but simply 
(hopefully) small audio glitches and skips for audio-only applications, 
which might be an acceptable code/complexity trade-off for some 
applications. Another possibility might be to actually re-encode the 
audio that was played out (so that decoding does not depend on having a 
good PLC implementation, but matches what was heard during the call). 
That's more complex to get right than just relying on decoder PLC, but I 
don't see a reason to have a MUST that would disallow it.

> would implement their own version of this muxer SHOULD to handle the gap

That makes computing per-packet timestamps then becomes an ill-posed 
problem, which is not something we should require demuxers to solve. I 
think the document is clear elsewhere that there must be no gaps in the 
timestamps and that packets must be at least one byte, and nothing here 
was meant to relax those requirements.

>> demuxers to compute incorrect granule positions when seeking

Again, violating this SHOULD does not mean you are allowed to mux 
packets with gaps in the timestamps. Perhaps we need to add a MUST NOT 
to reinforce the constraints laid out elsewhere in the document, since 
there seems to be some misunderstanding here.

> Correct. Ogg does not allow discontinuous transmission (or rather

It does, but traditionally only for things like subtitle streams, not 
audio, and the encoded packet data needs to contain enough information 
to unambiguously recover per-packet timestamps. RTP includes per-packet 
sequence number and timestamp values in order to more easily recover 
from losses, and pays a 6-byte per packet overhead to do so. In Ogg the 
unit of loss recovery is the page, and there is only one timestamp 
(granule position) per page, which allows the per-packet overhead to be 
very close to one byte. But that's what leads to the need to be explicit 
about per-packet losses.

> marking sample-accurate edit points isn't relevant there.

Right, I'm not aware of any other codec in RTP that defines 
sample-accurate trimming. If we did decide it was a problem worth 
solving in RTP, we should solve it generically, instead of doing 
something Opus-specific. Sample accurate trimming *is* a feature 
available to other codecs in Ogg (and was one of the original selling 
points of Vorbis over MP3), but because granule position is 
codec-specific in Ogg, it requires an Opus-specific definition in this 
document.

> This one is clearly "demuxer MAY".

I believe the intent of the MAY was to clarify that the preceding 
"...MUST NOT reject it for containing additional data..." did not give 
muxers carte blanche to add an unbounded amount of additional data. I 
would be okay making the RFC 2119 keyword stronger, but it shouldn't be 
stronger than the "..SHOULD reject ID headers which do not contain 
enough data..." earlier in the paragraph (upgrading both to MUST 
wouldn't be unreasonable).

> just how many of each we have.

In practice, the channel mapping can reorder the channels arbitrarily 
after decoding, so it doesn't actually impose any restrictions on what 
can be coupled with what.

> Can we cite both RFC 6381 (for the format) and RFC 5334 (for

That sounds reasonable to me.

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Mon, 23 Nov 2015 20:59:43 -0800

> like a solid improvement to me.

I went through and implemented it. The only remaining uses of 
encoder/decoder should be describing what happens in an RFC 6716 
implementation, without imposing any additional normative requirements 
on such an implementation. This involved downgrading one normative 
statement to non-normative:

 From "Opus uses the pre-skip mechanism for this purpose instead, since 
the encoder MAY introduce more than a single packet's worth of latency, 
and since very large packets in streams with a very large number of 
channels might not fit on a single page."

To "Opus uses the pre-skip mechanism for this purpose instead, since the 
encoder might introduce more than a single packet's worth of latency, 
and since very large packets in streams with a very large number of 
channels might not fit on a single page."

A few encoder/decoder references were also changed to muxer/demuxer 
where this was appropriate.

> cutting them if you don't manage to come up with something better.

Moved.

> there seems to be some misunderstanding here.

I added, "Implementations that fail to do so still MUST NOT increment 
the granule position for a page by anything other than the number of 
samples contained in packets that actually complete on that page."

> wouldn't be unreasonable).

I simply rephrased this as, "...MUST NOT...unless...", which I think 
wound up clearer.

I believe all other comments should be addressed in the -09 version that 
I just submitted.