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)
_______________________________________________
codec mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/codec
