From nobody Sat Nov 14 19:14:49 2015 Return-Path: X-Original-To: codec@ietfa.amsl.com Delivered-To: codec@ietfa.amsl.com Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 9C1381B3126 for ; Sat, 14 Nov 2015 19:14:47 -0800 (PST) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -14.511 X-Spam-Level: X-Spam-Status: No, score=-14.511 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jlptZ2IlV2Lo for ; Sat, 14 Nov 2015 19:14:45 -0800 (PST) Received: from rcdn-iport-8.cisco.com (rcdn-iport-8.cisco.com [173.37.86.79]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 9A1FE1B3124 for ; Sat, 14 Nov 2015 19:14:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=6655; q=dns/txt; s=iport; t=1447557285; x=1448766885; h=from:to:subject:date:message-id:references:in-reply-to: content-transfer-encoding:mime-version; bh=Pqh/Hu+feDQFJleZIesLiDfgJsE4VJuNs5SFGH7/lVA=; b=Vpd9WnBfUmWdrYmo01vCQVscdArsr3d1z0zkgLf3zVH6KVng52AzaWs4 ZKAtAFufCpQWCLzY8BL4jEZAYu7g3EUZ3Nvlsji9clT011Lx22HN0RHY4 wKDfggvRb5Tshj77V+ML5GKbBjxWTC9GtNOfy3dDgdAtbgWatKTUgjy8Y Y=; X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0AAAgBd90dW/5tdJa1egzvADQENgWSGE?= =?us-ascii?q?AKBJzgUAQEBAQEBAYEKhDUBAQQnE08CAT4QMiUCBAGIQLZ1AQEBAQEBAQEBAQE?= =?us-ascii?q?BAQEBAQEchlSCEIJuiCSBFQWNG4ktAXuMK4FbhECHNYYhgluFeAEfAQFChASGP?= =?us-ascii?q?QEBAQ?= X-IronPort-AV: E=Sophos;i="5.20,296,1444694400"; d="scan'208";a="45029673" Received: from rcdn-core-4.cisco.com ([173.37.93.155]) by rcdn-iport-8.cisco.com with ESMTP; 15 Nov 2015 03:14:44 +0000 Received: from XCH-ALN-005.cisco.com (xch-aln-005.cisco.com [173.36.7.15]) by rcdn-core-4.cisco.com (8.14.5/8.14.5) with ESMTP id tAF3Einp022220 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Sun, 15 Nov 2015 03:14:44 GMT Received: from xch-aln-005.cisco.com (173.36.7.15) by XCH-ALN-005.cisco.com (173.36.7.15) with Microsoft SMTP Server (TLS) id 15.0.1104.5; Sat, 14 Nov 2015 21:14:43 -0600 Received: from xch-aln-005.cisco.com ([173.36.7.15]) by XCH-ALN-005.cisco.com ([173.36.7.15]) with mapi id 15.00.1104.000; Sat, 14 Nov 2015 21:14:43 -0600 From: "Mo Zanaty (mzanaty)" To: "codec@ietf.org" , "draft-ietf-codec-oggopus@tools.ietf.org" Thread-Topic: Review of draft-ietf-codec-oggopus-08 Thread-Index: AQHRH1L8aHUttDT+9E+XdfD9vADa1J6caOZm Date: Sun, 15 Nov 2015 03:14:43 +0000 Message-ID: <6EEDB488-D9D1-4CDD-8C0A-73D2244DCE3E@cisco.com> References: In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Archived-At: Subject: [codec] Review of draft-ietf-codec-oggopus-08 X-BeenThere: codec@ietf.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Codec WG List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Nov 2015 03:14:47 -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 a= re 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, a= nd this spec to only specify muxer/demuxer behavior. Please check all "enco= der/decoder" instances to see if you really mean "muxer/demuxer". If you re= ally 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 In= troduction, 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 conta= in 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 co= mment 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 behavi= or 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 larg= er than zero for the first audio data completion page as described in secti= on 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 u= se of Packet Loss Concealment (PLC) in place of the missing packets." Why S= HOULD not MUST? The prior paragraph uses MUST and says "For this to work, t= here cannot be any gaps." If you want to keep it as SHOULD strength, perhap= s state that muxers which fail to do this will cause demuxers to compute in= correct 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 th= e RTP case, RTP timestamp signals the gap (if using DTX), while Ogg uses th= e granule position. Correct? If not, should any of this section apply to RF= C 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 con= fused 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 fail= ure? - 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 MUS= T 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 M= UST 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. demuxe= r MAY.=20 Section 5.1.1. Channel Mapping - 1. to 3. Why *asterisk* again? - 2. Coupled Stream Count: "...the first M Opus decoders are to be initiali= zed for stereo output..." Is this an intended restriction that all stereo c= hannels must appear first before any mono channels? - 3. Channel Mapping: "...If 'index' is 2*M or larger, but less than 255, t= he output MUST be taken from decoding stream ('index'-M) as mono." Should b= e index-2*M not index-M. - Channel and stream are used interchangeably, as well as stereo and couple= d, stereo and 2-channel, mono and 1-channel. Different meanings in differen= t 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 cont= exts. 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 "chan= nel" before the comma.=20 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 m= emory or to take an excessive amount of resources to decode. Although prob= lems in encoders are typically rarer, the same applies to the encoder. Mal= icious audio streams MUST NOT cause the encoder to misbehave..." Replace "m= isbehave" 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=3D" for ISO MIME types (= audio/mp4, video/mp4, etc.). -- RFC 5334 defines "codecs=3D" for Ogg MIME types (audio/ogg, video/ogg, a= pplication/ogg). -- Notably, the "codecs=3D" name space format differs between RFC 6381 (4 c= haracter code for ISO) vs. RFC 5334 (4-8 character codes are mapped to long= er codec names for Ogg). - This document should indicate it updates RFC 5334 to add "opus" to the "c= odecs=3D" parameter. - This document should update the IANA Media Types registry to reference it= self 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 c= omment. Section 13. Copying Conditions - This is strange for IETF specs. Why? Cheers, Mo (as Document Shepherd) From nobody Tue Nov 17 15:57:07 2015 Return-Path: X-Original-To: codec@ietfa.amsl.com Delivered-To: codec@ietfa.amsl.com Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 525291B320C for ; Tue, 17 Nov 2015 15:57:06 -0800 (PST) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: 0.8 X-Spam-Level: X-Spam-Status: No, score=0.8 tagged_above=-999 required=5 tests=[BAYES_50=0.8, DKIM_SIGNED=0.1, DKIM_VALID=-0.1] autolearn=ham Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9qKlBm9gx8gg for ; Tue, 17 Nov 2015 15:57:02 -0800 (PST) Received: from mail-pa0-x22e.google.com (mail-pa0-x22e.google.com [IPv6:2607:f8b0:400e:c03::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id D800B1B3202 for ; Tue, 17 Nov 2015 15:57:01 -0800 (PST) Received: by pacej9 with SMTP id ej9so23952979pac.2 for ; Tue, 17 Nov 2015 15:57:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=thaumas_net.20150623.gappssmtp.com; s=20150623; h=from:subject:to:references:message-id:date:user-agent:mime-version :in-reply-to:content-type:content-transfer-encoding; bh=P9YaB9Lez6TKZAgtvPCyUol7+TJOYv1mSnrbYQzO7Y8=; b=BoiduKTIi3dHPMyMzLMpE5kmc8Ar+bzCS8D7JgIxhjD6GmmmxeuRTO8wP+BrJieKPx 4V0tjaubzFKIxKbv8a8UShuppf69IqDYi3O9Iv8a88aHgpEmVgFFNaPKPck2SwYBbdnG thWujaX1xj8VFLAygTJFXsOFCO7iweQ2yC5eqltGvovsjs5LSyE0VxIvUkY47uh7lw8N FlAlamgOVLUrF8T3ToheQ6OuBnjgo3U680edP/WzE4LRDPI4i4uYfGeIaNYlWwFdneJ5 Uy18YMQV2nPe5e8AbfyAelegKFFOmX4I/FKk5JBflQHaXMovaAMrxzz8N34h89grng3q cW3Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=P9YaB9Lez6TKZAgtvPCyUol7+TJOYv1mSnrbYQzO7Y8=; b=SzLUllya9biUcdf+I0ghRstdr4vgege9AOM/vO5v8KLczYObwqNtS1bVG64tYuku5V 01OvAT8lF9cq9w7wCrjkf+vTmIc8ZznbyZvdKvyOv/1ymQj9ZtMit4if4hQ4TYSJXJeS U2UnKM3yk4sMctKYIv4F4PsKOyEQLWr6bWDJpffCPTnkLW3X2SYUiyRG5PjbfyYz0cmS nEz9vYasQyXnkuX2od/JUxjb7Mm3HHWOlQ4x7tZBz2U6UVPwstand646FhFb/vjhZU5O DKInb+MJdSe20xwlDE5kCfLsGDvz2qawChW2q0eUpPBNy65M5twN6AyZ6rFFn0phd7Wv V2oA== X-Gm-Message-State: ALoCoQmQU+RyX0Iq6VCb1MhpPE19JufrNiaMWY8+gu5VliI00yazmR8zOwdXGRfZzWyZUpvqf+Rj X-Received: by 10.66.240.4 with SMTP id vw4mr68287747pac.9.1447804620823; Tue, 17 Nov 2015 15:57:00 -0800 (PST) Received: from tamias.local ([2607:ffb0:3001:224:695f:9b7e:5f0e:20ff]) by smtp.googlemail.com with ESMTPSA id kj3sm45029437pbc.59.2015.11.17.15.56.59 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 17 Nov 2015 15:57:00 -0800 (PST) From: Ralph Giles To: "Mo Zanaty (mzanaty)" , "codec@ietf.org" , "draft-ietf-codec-oggopus@tools.ietf.org" References: <6EEDB488-D9D1-4CDD-8C0A-73D2244DCE3E@cisco.com> X-Enigmail-Draft-Status: N1210 Message-ID: <564BBECB.4050603@thaumas.net> Date: Tue, 17 Nov 2015 15:56:59 -0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <6EEDB488-D9D1-4CDD-8C0A-73D2244DCE3E@cisco.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Archived-At: Subject: Re: [codec] Review of draft-ietf-codec-oggopus-08 X-BeenThere: codec@ietf.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Codec WG List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Nov 2015 23:57:06 -0000 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 at the beginning of a ? 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 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 not => _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. > - This document should indicate it updates RFC 5334 to add "opus" > 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 nobody Tue Nov 17 17:41:41 2015 Return-Path: X-Original-To: codec@ietfa.amsl.com Delivered-To: codec@ietfa.amsl.com Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 898EC1B371C for ; Tue, 17 Nov 2015 17:41:38 -0800 (PST) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -5.313 X-Spam-Level: X-Spam-Status: No, score=-5.313 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, HELO_MISMATCH_ORG=0.611, HOST_MISMATCH_COM=0.311, RCVD_IN_DNSWL_HI=-5, SPF_SOFTFAIL=0.665] autolearn=ham Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Hh9CAERbdCM5 for ; Tue, 17 Nov 2015 17:41:35 -0800 (PST) Received: from smtp.mozilla.org (mx1.scl3.mozilla.com [63.245.214.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 45D381B371E for ; Tue, 17 Nov 2015 17:41:35 -0800 (PST) Received: from localhost (localhost6.localdomain [127.0.0.1]) by mx1.mail.scl3.mozilla.com (Postfix) with ESMTP id D2547C22EE; Wed, 18 Nov 2015 01:41:34 +0000 (UTC) X-Virus-Scanned: amavisd-new at mozilla.org Received: from smtp.mozilla.org ([127.0.0.1]) by localhost (mx1.mail.scl3.mozilla.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qTB25YdIiYA3; Wed, 18 Nov 2015 01:41:34 +0000 (UTC) Received: from [10.252.26.14] (corp.mtv2.mozilla.com [63.245.221.32]) (Authenticated sender: tterriberry@mozilla.com) by mx1.mail.scl3.mozilla.com (Postfix) with ESMTPSA id B96E7C1C7A; Wed, 18 Nov 2015 01:41:34 +0000 (UTC) Message-ID: <564BD74E.8080200@xiph.org> Date: Tue, 17 Nov 2015 17:41:34 -0800 From: "Timothy B. Terriberry" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 SeaMonkey/2.26 MIME-Version: 1.0 To: Ralph Giles , "Mo Zanaty (mzanaty)" , "codec@ietf.org" , "draft-ietf-codec-oggopus@tools.ietf.org" References: <6EEDB488-D9D1-4CDD-8C0A-73D2244DCE3E@cisco.com> <564BBECB.4050603@thaumas.net> In-Reply-To: <564BBECB.4050603@thaumas.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Archived-At: Subject: Re: [codec] Review of draft-ietf-codec-oggopus-08 X-BeenThere: codec@ietf.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Codec WG List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2015 01:41:38 -0000 Ralph Giles wrote: > Mark suggested 'an implementation (of this specification)' as a way to > disabiguate. 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. > 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. 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. > 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. 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. >> - 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. That captures the intent (i.e., that the header *might* span more than one page). Works for me. > Saying "Muxers MUST to X. Demuxers MUST handle streams not compliant > with that MUST," is confusing without offering better precision. I agree. C.f., , I don't think we should add a muxer MUST unless we're prepared to have demuxers actually fail on said streams. >> 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. 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. > 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. 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. >> 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. 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. >> - 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 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. >> 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. 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. >> 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". 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). >> 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. 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 > ogg-specifics) here? That sounds reasonable to me. From nobody Mon Nov 23 20:59:04 2015 Return-Path: X-Original-To: codec@ietf.org Delivered-To: codec@ietfa.amsl.com Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 489F51B2D0E; Mon, 23 Nov 2015 20:59:02 -0800 (PST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit From: internet-drafts@ietf.org To: X-Test-IDTracker: no X-IETF-IDTracker: 6.11.0 Auto-Submitted: auto-generated Precedence: bulk Message-ID: <20151124045902.23632.70812.idtracker@ietfa.amsl.com> Date: Mon, 23 Nov 2015 20:59:02 -0800 Archived-At: Cc: codec@ietf.org Subject: [codec] I-D Action: draft-ietf-codec-oggopus-09.txt X-BeenThere: codec@ietf.org X-Mailman-Version: 2.1.15 List-Id: Codec WG List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Nov 2015 04:59:02 -0000 A New Internet-Draft is available from the on-line Internet-Drafts directories. This draft is a work item of the Internet Wideband Audio Codec Working Group of the IETF. Title : Ogg Encapsulation for the Opus Audio Codec Authors : Timothy B. Terriberry Ron Lee Ralph Giles Filename : draft-ietf-codec-oggopus-09.txt Pages : 32 Date : 2015-11-23 Abstract: This document defines the Ogg encapsulation for the Opus interactive speech and audio codec. This allows data encoded in the Opus format to be stored in an Ogg logical bitstream. The IETF datatracker status page for this draft is: https://datatracker.ietf.org/doc/draft-ietf-codec-oggopus/ There's also a htmlized version available at: https://tools.ietf.org/html/draft-ietf-codec-oggopus-09 A diff from the previous version is available at: https://www.ietf.org/rfcdiff?url2=draft-ietf-codec-oggopus-09 Please note that it may take a couple of minutes from the time of submission until the htmlized version and diff are available at tools.ietf.org. Internet-Drafts are also available by anonymous FTP at: ftp://ftp.ietf.org/internet-drafts/ From nobody Mon Nov 23 20:59:47 2015 Return-Path: X-Original-To: codec@ietfa.amsl.com Delivered-To: codec@ietfa.amsl.com Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 9440E1B2D0E for ; Mon, 23 Nov 2015 20:59:45 -0800 (PST) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -3.414 X-Spam-Level: X-Spam-Status: No, score=-3.414 tagged_above=-999 required=5 tests=[BAYES_40=-0.001, HELO_MISMATCH_ORG=0.611, HOST_MISMATCH_COM=0.311, RCVD_IN_DNSWL_HI=-5, SPF_SOFTFAIL=0.665] autolearn=ham Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Q-Tj9W03Uv26 for ; Mon, 23 Nov 2015 20:59:44 -0800 (PST) Received: from smtp.mozilla.org (mx1.scl3.mozilla.com [63.245.214.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 305B71B29DC for ; Mon, 23 Nov 2015 20:59:44 -0800 (PST) Received: from localhost (localhost6.localdomain [127.0.0.1]) by mx1.mail.scl3.mozilla.com (Postfix) with ESMTP id DC272C2076; Tue, 24 Nov 2015 04:59:43 +0000 (UTC) X-Virus-Scanned: amavisd-new at mozilla.org Received: from smtp.mozilla.org ([127.0.0.1]) by localhost (mx1.mail.scl3.mozilla.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fikDt_ydfQrw; Tue, 24 Nov 2015 04:59:43 +0000 (UTC) Received: from [172.17.0.37] (50-78-100-113-static.hfc.comcastbusiness.net [50.78.100.113]) (Authenticated sender: tterriberry@mozilla.com) by mx1.mail.scl3.mozilla.com (Postfix) with ESMTPSA id 8DC00C0096; Tue, 24 Nov 2015 04:59:43 +0000 (UTC) Message-ID: <5653EEBF.5060803@xiph.org> Date: Mon, 23 Nov 2015 20:59:43 -0800 From: "Timothy B. Terriberry" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 SeaMonkey/2.26 MIME-Version: 1.0 To: "codec@ietf.org" , "draft-ietf-codec-oggopus@tools.ietf.org" References: <6EEDB488-D9D1-4CDD-8C0A-73D2244DCE3E@cisco.com> <564BBECB.4050603@thaumas.net> <564BD74E.8080200@xiph.org> In-Reply-To: <564BD74E.8080200@xiph.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Archived-At: Subject: Re: [codec] Review of draft-ietf-codec-oggopus-08 X-BeenThere: codec@ietf.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Codec WG List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Nov 2015 04:59:45 -0000 Timothy B. Terriberry wrote: > 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. 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. >>> 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. > > 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. Moved. > 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. 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." > 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). 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.