Ballot for draft-ietf-codec-oggopus
Yes
No Objection
Note: This ballot was opened for revision 12 and is now closed.
There has been considerable last call discussion about the inclusion of additional copyright licensing language by the authors. The authors have agreed to progress the draft without such language, as reflected in revision 12. If the authors choose to grant additional rights, they may do so independently. [Thanks to the authors for fixing the xml2rfc source issue. I've removed that bit from these comments] There has also been last call discussion about the fact that this draft is intended to be a proposed standard, but it normatively depends on RFC 3533 (the Ogg container format) , which is informational. In my opinion, this is appropriate because the draft specifies the encapsulation of Opus, which is a proposed standard. (RFC 6716).
-- Section 3 -- A demuxer SHOULD NOT attempt to decode the data for the first packet on a page with the 'continued packet' flag set if the previous page with packet data does not end in a continued packet (i.e., did not end with a lacing value of 255) or if the page sequence numbers are not consecutive, unless the demuxer has some special knowledge that would allow it to interpret this data despite the missing pieces. This "SHOULD NOT" requirement is in a very long sentence with a few "ifs" and "buts", which make it hard to see what's really being required. Rewording to put the conditions up front might help. It'd also be good to explain why it's SHOULD NOT, rather than, say, MUST NOT. Under what conditions might I want to decode the first packet anyway, and what are the consequences of my doing that? It looks like the "unless" clause is there to explain that, in which case "MUST NOT ... unless" seems right. For the rewording, maybe this?: NEW If a page has the "continued packet" flag set and one of the following conditions is also true: - the previous page with packet data does not end in a continued packet (a lacing value of 255) OR - the page sequence numbers are not consecutive, then the demuxer SHOULD NOT attempt to decode the data for the first packet on the page unless the demuxer has some special knowledge that would allow it to interpret this data despite the missing pieces. END ...and then consider whether it really should be "MUST NOT ... unless". You should take similar action with the "SHOULD NOT" sentence in the next paragraph as well, as it has the same convolution problem. -- Section 5.2 -- In bullet 4, might it not be clearer to call it "User Comment List Count", rather than "Length"? In the list in general, I think it'd be clearer and more accurate to group all the "MUST NOT indicate that there are so many..." sorts of things into one one-sentence pargraph that says that the Vendor String and all the User Comments, together, MUST fit into the packet. No? -- Section 9 -- Malicious payloads MUST NOT cause an implementation to overrun its allocated memory or to take an excessive amount of resources to decode. ... Malicious audio input streams MUST NOT cause an implementation to overrun its allocated memory or consume excessive resources It's a small thing, rather at the nit level, but it doesn't make much sense to levy normative requirements on malicious actors. These would be much better if worded as requirements on the implementation, somewhat like this: NEW Malicious payloads and/or input streams can be used to attack codec implementations. Implementations MUST NOT overrun their allocated memory nor take an excessive amount of resources when decoding payloads or processing input streams. END -- Section 11 -- Modifications to this registry follow the "Specification Required with Expert Review" registration policy as defined in [RFC5226]. RFC 5226 does not define a policy with that name. The name is just "Specification Required"; please change this. And thanks for the paragraph giving guidance to the designated expert.
Scott Bradner performed the OPSdir review. There was an ask for a paragraph covering the following That said, I wonder if it would be possible to as a OAM-like ability to support "in-band" condition information - for example by adding an OAM channel to the channel mapping function (i.e. the "Opus Channel Mapping Families" registry) where the definition would be that the channel is for the use of OAM services and is otherwise passed unmodified. Having the ability to have real time "what the customer is seeing" RTCP like, but with more expansion abilities, feedback could be quite helpful operations-wise which has not yet come to conclusion.
- general, (but a nit): there are some odd unexplained numbers here, which is fine ... but odd. (E.g. 125,829,120) It might be nicer to explain to the reader why these values were chosen. I assume there are unstated reasons that an implementer need not know - if an implementer really should grok why one of these odd numbers is used, then that would be better explained. - An example or diagram in the intro or section 3 would maybe have helped. - section 5: Q7.8 is a new one to me. Maybe add a reference? - 5.1.1.5: Could figures 3-8 do with some body text to explain them? That's (having body text that refers to the figures), general good practice and I'm not sure if they're sufficiently clear for an implementer to get right. (But as I'm not coding this, I don't know, so just checking:-) - 5.2: You don't say that the user comment strings must also be UTF8, but you do for the vendor string. Why not? I think it'd be good to call that out. - 5.2.1: [vorbis-comment] is, correctly, a normative reference here but I wonder if a xiph.org URL is a good enough reference for that. Is there anything better, or are we confident enough in that URL? - 5.2.1: From what namespace are the -573 and 111 values here selected? How is that managed? (Just wondering.) - section 9: While I like the MUST NOT here, it's really only wishful thinking and isn't a strictly valid use of 2119 terms. I'm ok with that though. The SHOULD NOT statement is also kind of bogus. Generally this section would be better if it had guidance on where implementers are likely to go wrong in ways that cause security issues. Reference such as are provided in RFC 6562 about the dangers of VBR might also be useful here, not sure.