Subject: [codec] draft-ietf-codec-oggopus-07 packet sizes

From: Mark Harris <[email-masked]>
Date: Mon, 4 May 2015 19:45:18 -0700

draft-ietf-codec-oggopus-07 adds some concrete limits to the packet
sizes that must be accepted, which I agree is a good idea, however
there are some issues with the specific text.

Section 6 states:
"Decoders SHOULD reject packets larger than 60 kB per channel, and
display a warning message, and MAY reject packets larger than 7.5 kB
per channel."

It is unclear whether "channel" here refers to output channels,
decoded channels, or physical channels (although output channels makes
the most sense).  Also, while the next paragraph appears to be a
justification for these numbers, the numbers in that paragraph are per
stream and not per channel.  The number of output channels may be
higher or lower than the number of streams so the connection is
unclear.  Additionally it is unclear whether this section applies only
to audio data packets or also to comment packets, which do not have
channels or streams but could be larger than audio data packets.

It is also unclear how this interacts with the Section 5.1.1.5
requirement that an Ogg Opus player play *any* Ogg Opus stream with a
channel mapping family of 0 or 1 (apparently including those with
large packets).  In order to comply with that, does that mean that
large packets may only be rejected when the channel mapping family is
not 0 or 1, or by applications that are not an "Ogg Opus player"?

It appears that the intent is to require Ogg Opus players to play any
valid Ogg Opus stream with channel mapping family 0 or 1 (maximum 8
channels) within limits that should be reasonable for any such stream,
allowing for resource-constrained implementations with fixed buffers
and a reasonably reliable test for ensuring that a particular stream
is likely to play even on resource constrained players as intended,
while still allowing streams that are limited only by what should be
considered malicious which may not be playable on all players.

To address these issues, I propose that the packet size required to be
processed by any implementation be stated as a simple 60 kB size and
be connected to the Section 5.1.1.5 requirement.  This is the size
suggested at the end of Section 6 and simplifies the requirement
without increasing the packet size that could be interpreted to
already be required to be played by a minimal player.  I further
suggest that the larger size at which audio data packets SHOULD be
rejected be set at 60 kB per stream rather than per "channel",
aligning with the largest possible valid packet without padding as
described at the beginning of the second paragraph of Section 6.
Because packets exceeding 60 kB per stream necessarily use padding in
excess of what would be required to make a VBR stream CBR, this also
aligns with Section 6 encoder recommendations and avoids the situation
where decoders SHOULD reject certain packets that an encoder not
violating any SHOULD or MUST might produce.

The new text about decoders displaying a warning message is also
worrying.  Typically a decoder does not have access to the
application's preferred display or localization facilities so this
seems inappropriate.  Even considering the higher layers it seems
unlikely that typical usage, such as background audio decoding in a
portable music player, game engine, or the like would have a facility
for displaying this kind of warning message during decoding.  Also if
a warning should be displayed when packets larger than 60 kB per
channel are rejected, why not when smaller packets are rejected?  For
these reasons, and because applications are already free to indicate
any issues that are important to their users in the manner most
appropriate for their application, I suggest omitting this text.

On another note, it appears that in the packet sizes in Section 6, kB
refers to 1024 bytes and MB refers to 1024*1024 bytes, whereas
everywhere else in the document (including kbps and Mbps in the same
paragraph) the standard SI decimal multiples are intended.  Because
the usage is limited I suggest just sticking with bytes/octets to
avoid confusion, rather than adding a sentence to clarify the mixed
usage.  If a 1024-byte unit is desired I believe that the traditional
designation is KB (or the newer KiB).

Below is a precise proposal for comment:

In Section 5.1.1.5 replace:

   An Ogg Opus player MUST play any Ogg Opus stream with a channel
   mapping family of 0 or 1, even if the number of channels does not
   match the physically connected audio hardware.

with:

   An Ogg Opus player MUST play any valid Ogg Opus stream with a
   channel mapping family of 0 or 1 that contains no packet larger
   than 61,440 octets, even if the number of channels does not match
   the physically connected audio hardware.

In Section 6 replace:

   Technically, valid Opus packets can be arbitrarily large due to the
   padding format, although the amount of non-padding data they can
   contain is bounded.  These packets might be spread over a similarly
   enormous number of Ogg pages.  Encoders SHOULD use no more padding
   than is necessary to make a variable bitrate (VBR) stream constant
   bitrate (CBR).  Decoders SHOULD avoid attempting to allocate
   excessive amounts of memory when presented with a very large packet.
   Decoders SHOULD reject packets larger than 60 kB per channel, and
   display a warning message, and MAY reject packets larger than 7.5 kB
   per channel.  The presence of an extremely large packet in the stream
   could indicate a memory exhaustion attack or stream corruption.

with:

   Technically, valid Opus packets can be arbitrarily large due to
   padding, although the amount of non-padding data that an audio data
   packet can contain is bounded.  These packets might be spread over
   a similarly enormous number of Ogg pages.  In an audio data packet,
   encoders SHOULD use no more padding than is necessary to make a
   variable bitrate (VBR) stream constant bitrate (CBR).  Decoders
   SHOULD reject audio data packets larger than 61,440 octets per
   stream; such packets necessarily contain more padding than needed
   for this purpose.  Decoders SHOULD avoid attempting to allocate
   excessive amounts of memory when presented with a very large
   packet, and MAY reject or partially process any packet larger
   than 61,440 octets.  The presence of an extremely large packet
   in the stream could indicate a memory exhaustion attack or stream
   corruption.

Replace:

   In an Ogg Opus stream, the largest possible valid packet that does
   not use padding has a size of (61,298*N - 2) octets, or about 60 kB
   per Opus stream.  With 255 streams, this is 15,630,988 octets
   (14.9 MB) and can span up to 61,298 Ogg pages, all but one of which
   will have a granule position of -1.  This is of course a very extreme
   packet, consisting of 255 streams, each containing 120 ms of audio
   encoded as 2.5 ms frames, each frame using the maximum possible
   number of octets (1275) and stored in the least efficient manner
   allowed (a VBR code 3 Opus packet).  Even in such a packet, most of
   the data will be zeros as 2.5 ms frames cannot actually use all
   1275 octets.  The largest packet consisting of entirely useful data
   is (15,326*N - 2) octets, or about 15 kB per stream.  This
   corresponds to 120 ms of audio encoded as 10 ms frames in either SILK
   or Hybrid mode, but at a data rate of over 1 Mbps, which makes little
   sense for the quality achieved.  A more reasonable limit is
   (7,664*N - 2) octets, or about 7.5 kB per stream.  This corresponds
   to 120 ms of audio encoded as 20 ms stereo CELT mode frames, with a
   total bitrate just under 511 kbps (not counting the Ogg encapsulation
   overhead).  With N=8, the maximum number of channels currently
   defined by mapping family 1, this gives a maximum packet size of
   61,310 octets, or just under 60 kB.  This is still quite
   conservative, as it assumes each output channel is taken from one
   decoded channel of a stereo packet.  An implementation could
   reasonably choose any of these numbers for its internal limits.

with:

   In an Ogg Opus stream, the largest possible valid audio data packet
   that does not use padding has a size of (61,298*N - 2) octets,
   although a valid comment header packet may be larger.  With 255 Opus
   streams, this is 15,630,988 octets and can span up to 61,298 Ogg
   pages, all but one of which will have a granule position of -1.
   This is of course a very extreme packet, consisting of 255 Opus
   streams, each containing 120 ms of audio encoded as 2.5 ms frames,
   each frame using the maximum possible number of octets (1275) and
   stored in the least efficient manner allowed (a VBR code 3 Opus
   packet).  Even in such a packet, most of the data will be zeros as
   2.5 ms frames cannot actually use all 1275 octets.

   The largest audio data packet consisting of entirely useful data is
   (15,326*N - 2) octets.  This corresponds to 120 ms of audio encoded
   as 10 ms frames in either SILK or Hybrid mode, but at a data rate
   of over 1 Mbps, which makes little sense for the quality achieved.

   A more reasonable limit is (7,664*N - 2) octets.  This corresponds
   to 120 ms of audio encoded as 20 ms stereo CELT mode frames, with
   a total bitrate just under 511 kbps (not counting the Ogg
   encapsulation overhead).  For mapping family 1, N=8 provides a
   reasonable upper bound, as it allows for each of the 8 possible
   output channels to be encoded in a separate stereo Opus stream.
   This gives a packet size of 61,310 octets, which is rounded up to a
   multiple of 1024 octets to give the packet size of 61,440 octets
   that is expected to be successfully processed by any implementation.

I also found that the fragment identifiers are no longer valid in the
informative references to the Vorbis specification.  This fixes those
and makes the host name consistent.

In Section 14.2 replace:

   https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-800004.3.9

with:

   https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-810004.3.9

Replace:

   https://xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-130000A.2

with:

   https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-132000A.2

 - Mark

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Tue, 12 May 2015 15:49:09 -0700

>     the physically connected audio hardware.

This should probably forward-reference Section 6, i.e., simply by adding 
a "(See Section 6)" after "61,440 octets." Also, I assume you meant this 
restriction to only apply to audio data packets (in keeping with your 
new text in Section 6)?

>     variable bitrate (VBR) stream constant bitrate (CBR).  Decoders

Talking about doing something in a single audio data packet that affects 
an entire stream seems a little tortured. How about, "Encoders SHOULD 
pad audio data packets no more than necessary to make a variable bitrate 
(VBR) stream constant bitrate (CBR)"?

>     than 61,440 octets.  The presence of an extremely large packet

This limit is reasonable for channel mapping families 0 and 1 (7.5 kB 
per stream for each of a maximum of 8 non-useless streams), but not for 
channel mapping family 255, which may carry many more channels (and is 
currently the only way to do so). Suggest, "...and MAY reject or 
partially process any packet larger than 61,440 octets when used with 
channel mapping family 1, or a packet larger than 7,680 octets per 
stream when used with channel mapping family 255."

We could also put in a smaller MAY limit for channel mapping family 0, 
but I don't think that's that useful, as I suspect most players will a) 
only support mapping families 0 and 1, and b) use a fixed-size buffer 
independent of the file they're decoding.

>     output channels to be encoded in a separate stereo Opus stream.

Suggest "decoded from" instead of "encoded in" to make this sound more 
reasonable.

>     that is expected to be successfully processed by any implementation.

Just wordsmithing, how about: "This gives a limit of 61,310 octets, 
rounded up to a multiple of 1024 to yield the maximum audio data packet 
size of 61,440 octets that any implementation is expected to be able to 
process successfully."

>     https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-132000A.2

Good catch, and thanks for all the review. If what I wrote above sounds 
fine, and there are no further comments/objections, I will make the 
changes by the end of the week.

---

From: Mark Harris <[email-masked]>
Date: Wed, 13 May 2015 00:44:47 -0700

Timothy B. Terriberry wrote:
> This should probably forward-reference Section 6, i.e., simply by adding a
> "(See Section 6)" after "61,440 octets."

Sounds good.

> Also, I assume you meant this
> restriction to only apply to audio data packets (in keeping with your new
> text in Section 6)?

Actually my proposed text in Section 6 permits rejecting or partial
processing of any packet larger than 61,440 octets (including a
comment packet), although I'm certainly open to other suggestions.  It
was unclear whether the previous text applied to a comment packet, but
if the intent is to allow implementations with a fixed-size packet
buffer then it seems reasonable that the same buffer would be used for
all packets and that the same limit should apply.

> Talking about doing something in a single audio data packet that affects an
> entire stream seems a little tortured. How about, "Encoders SHOULD pad audio
> data packets no more than necessary to make a variable bitrate (VBR) stream
> constant bitrate (CBR)"?

I'm not real happy with that wording, which seems as though it could
be misinterpreted as saying that encoders SHOULD pad audio data
packets to make a VBR stream CBR, but not use any more padding than
needed for that.  The intent was to just add a clause to the previous
text to make it clear that it is not restricting the use of other
kinds of padding, such as padding at the end of the comment packet in
a stream that is already CBR.  How about: "Encoders SHOULD limit the
use of padding in audio data packets to no more than is necessary to
make a variable bitrate (VBR) stream constant bitrate (CBR)"?

> This limit is reasonable for channel mapping families 0 and 1 (7.5 kB per
> stream for each of a maximum of 8 non-useless streams), but not for channel
> mapping family 255, which may carry many more channels (and is currently the
> only way to do so). Suggest, "...and MAY reject or partially process any
> packet larger than 61,440 octets when used with channel mapping family 1, or
> a packet larger than 7,680 octets per stream when used with channel mapping
> family 255."

Does the 7,680 octets per stream apply to the comment packet?

It is my impression that no decoders are currently required to process
any packets at all from a stream with channel mapping 255.  If that is
not the intention, then perhaps this is what should be clarified.  Is
it the act of starting to process a stream with mapping family 255
that obligates a decoder to process the remaining packets?  That is,
is it your intention that a decoder implementation that can handle
packets up to 1 MB be required to immediately reject and not even
attempt to process a stream with channel mapping 255 and 137 Opus
streams, because it is possible (although unlikely) that it may
contain a packet larger than 1 MB that it would be required to
process?  The text that I proposed was intended to allow such an
implementation to successfully process a stream that does not actually
exceed the implementation's packet size limit.

>
> We could also put in a smaller MAY limit for channel mapping family 0, but I
> don't think that's that useful, as I suspect most players will a) only
> support mapping families 0 and 1, and b) use a fixed-size buffer independent
> of the file they're decoding.

Also, even in mapping family 0 (or mapping family 255 with 1 stream)
it is reasonable to have a comment packet with album cover art.

> Suggest "decoded from" instead of "encoded in" to make this sound more

Ok.

> Just wordsmithing, how about: "This gives a limit of 61,310 octets, rounded
> up to a multiple of 1024 to yield the maximum audio data packet size of
> 61,440 octets that any implementation is expected to be able to process
> successfully."

This is showing the close connection between a reasonable maximum
packet size for an audio data packet in a stream with mapping family 1
(61,310 octets using the above definition of reasonable) and the
packet size given earlier (for any kind of packet) that any conforming
decoder is expected to be able to process (61,440 octets).  At least
from the point of view of decoder requirements, the 61,440 octet
number is more of a minimum (minimum packet buffer size) than a
maximum.  The purpose of making the connection is to help a decoder
implementor make a better informed decision on whether to support more
than the minimum requirement, and help an encoder implementor
determine how much concern they should have about producing a stream
that may not play as intended on all conforming decoders.  Because
they depend on the point of view I avoided using words like limit,
minimum, and maximum here, but perhaps that made the wording awkward.

 - Mark

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Wed, 13 May 2015 05:46:29 -0700

> comment packet), although I'm certainly open to other suggestions.  It

Yes, I don't think that's a good idea. Or rather, I certainly don't 
think rejecting a comment packet of that size is a good idea. Since they 
can contain multiple album art pictures, etc., it is not too hard to 
make one larger than this limit, and such files are commonly found in 
the wild.

I know Rockbox, for example, has code to skip over very large comment 
headers (which I suppose would fall under your definition of 'partially 
process'), which I think is fine for embedded devices that have a 
limited ability to display album art anyway, but given that the normal 
behavior of failing to decode a valid comment header is to reject the 
entire file, I think telling people they can reject comment headers over 
60 kB will lead to interoperability problems. I don't have good advice 
for a limit to specify here for rejection, but that doesn't mean we 
should give bad advice.

> make a variable bitrate (VBR) stream constant bitrate (CBR)"?

No objection from me.

> Does the 7,680 octets per stream apply to the comment packet?

I don't think it should, for the above reasons.

> not the intention, then perhaps this is what should be clarified.  Is

Certainly we do not obligate people to implement support for channel 
mapping 255, and certainly do not obligate support for 255 channels. But 
I think it is a mistake to look at this from the decoder's perspective. 
The point of this limit is to tell an _encoder_ what it can do an expect 
to have it work. Telling an encoder, "Well, there may be a packet size 
limit in the decoder, but we don't know what it is, and something may 
decide to play your stream with too small of a buffer, and fail in the 
middle, or not," is not exactly helpful. If we're going to do that, we 
might as well not specify any limits at all.

---

From: Mark Harris <[email-masked]>
Date: Mon, 25 May 2015 23:59:45 -0700

I've incorporated the earlier comments and made separate limits for
the comment header, clarifying that comment header packets exceeding
61,440 octets may be partially processed but not rejected entirely
unless they exceed a much larger size.  This size was chosen somewhat
arbitrarily to be 120 * 2^20 octets, which should be sufficient for
any reasonable album art in addition to other comments.

with:

   An Ogg Opus player MUST play any valid Ogg Opus stream with a
   channel mapping family of 0 or 1 that contains a comment header
   no larger than 125,829,120 octets (see Section 5.2), and no audio
   data packet larger than 61,440 octets (see Section 6), even if the
   number of channels does not match the physically connected audio
   hardware.

In Section 5.2, immediately before Section 5.2.1, insert:

   The comment header can be arbitrarily large and might be spread
   over a large number of Ogg pages.  Decoders SHOULD avoid attempting
   to allocate excessive amounts of memory when presented with a very
   large comment header.  To accomplish this, decoders MAY reject a
   comment header larger than 125,829,120 octets, and MAY ignore
   individual comments that are not fully contained within the first
   61,440 octets of the comment header or that would otherwise have
   no impact.

Replace Section 6 with:

6. Audio Data Packet Size Limits

   Technically, valid audio data packets can be arbitrarily large due
   to the padding format, although the amount of non-padding data they
   can contain is bounded.  These packets might be spread over a
   similarly enormous number of Ogg pages.  Encoders SHOULD limit the
   use of padding in audio data packets to no more than is necessary
   to make a variable bitrate (VBR) Ogg Opus stream constant bitrate
   (CBR).  Decoders SHOULD reject audio data packets larger than
   61,440 octets per Opus stream; such packets necessarily contain
   more padding than needed for this purpose.  Decoders SHOULD avoid
   attempting to allocate excessive amounts of memory when presented
   with a very large packet.  Decoders MAY reject or partially process
   audio data packets larger than 61,440 octets in an Ogg Opus stream
   with channel mapping family 1, or in any Ogg Opus stream if the
   packet is also larger than 7680 octets per Opus stream.  The
   presence of an extremely large packet in the stream could indicate
   a memory exhaustion attack or stream corruption.

   In an Ogg Opus stream, the largest possible valid audio data packet
   that does not use padding has a size of (61,298*N - 2) octets.
   With 255 Opus streams, this is 15,630,988 octets and can span up to
   61,298 Ogg pages, all but one of which will have a granule position
   of -1.  This is of course a very extreme packet, consisting of 255
   Opus streams, each containing 120 ms of audio encoded as 2.5 ms
   frames, each frame using the maximum possible number of octets
   (1275) and stored in the least efficient manner allowed (a VBR code
   3 Opus packet).  Even in such a packet, most of the data will be
   zeros as 2.5 ms frames cannot actually use all 1275 octets.

   A more reasonable audio data packet size limit is (7,664*N - 2)
   octets.  This corresponds to 120 ms of audio encoded as 20 ms
   stereo CELT mode frames, with a total bitrate just under 511 kbps
   (not counting the Ogg encapsulation overhead).  For mapping family
   1, N=8 provides a reasonable upper bound, as it allows for each of
   the 8 possible output channels to be decoded from a separate stereo
   Opus stream.  This gives a size of 61,310 octets, which is rounded
   up to a multiple of 1024 octets to yield the audio data packet size
   of 61,440 octets that any implementation is expected to be able to
   process successfully.

In Section 14.2 replace:

Replace:

 - Mark