Subject: [codec] AD review: draft-ietf-codec-opus-11

From: Robert Sparks <[email-masked]>
Date: Wed, 18 Apr 2012 15:14:23 -0500

Summary: This draft is almost ready for IETF last call, but has a few 
issues to discuss and address with a revised ID before starting that review

1) Section 10 (Copying considerations) appears to be trying to say 
something different from what the copyright notice on the first page 
says. Do we still need this section? Can it be removed at this time?

2) Can we set the license blocks in the code components (the files in 
the gzipped tarball) to use the license in the TLP? (See 
<http://trustee.ietf.org/license-info/IETF-Trust-License-Policy-20091228.pdf> 
section 4.c)

3) The text in section 6.2 leaves the standardization status of Opus 
Custom very unclear. Is this, and the associated code, part of this 
standard? If so, "not supported by the main Opus specification" is very 
confusing. Please clarify this section.

4) The draft claims the test vectors will be in some IETF meeting's 
proceedings. What is the plan to make that true? It would be optimal to 
have that done now, and a reference pointing to where they are in the 
proceedings added.

5) It would help to be very explicit about bit order somewhere early in 
the document. Have I missed where this is discussed already? (See 
section 3.1's description of the "top five bits" - these aren't the most 
significant bits per the diagram). If there's the opportunity for 
ambiguity, please also make sure byte order is explicitly described. For 
instance, this would help the reader figure out which byte is len[1] and 
which is len[0] in section 3.2.1.

6) There are several terms that someone practiced in this field would be 
expected to know, but it would help to add a reference to a definition 
or description. Examples: whitening filter, Hadamard transform, Schur 
algorithm, Viterbi algorithm, Burg's method. Similarly, adding a 
reference to an explanation of denormalized numbers would help.

7) Did the recommendations for using CBR in section 2.1.8 track the 
final conclusions of RFC6562? That RFC indicates VBR is unsafe even for 
unconstrained conversations if the information contained is highly 
sensitive.

8) (nit, but please adjust) Section 3.2.5 speaks of Abitrary numbers of 
frames. It's not arbitrary, it's properly between 2 and 32.

9) The description in 3.2.5 in the paragraph after figure 5 is 
confusing. It conflates padding with framing overhead, and it appears to 
allow saying that I have 254 bits of padding in more than one way. Is 
that intentional?

10) Section 3.4's title says it is going to talk about "Extending Opus". 
Instead, it talks about what packets are well formed, and requires the 
implementation to ignore everything else. It effectively says "All 
malformed packets might be the result of extensions" but doesn't discuss 
what the extension mechanisms are. I suggest retitling the section to 
something like "Receiving malformed packets", and adding a single 
sentence noting that extensions may cause implementations of _this_ spec 
to treat their packets as malformed.

11) I don't think you need 2119 MAYs in section 4.1.3 (Overall, the 
documents use of 2119 keywords is solid).

12) Section 4.1.5.1 and .2 - using a lower case L in these formulas 
(that also contain the digit form of one) is cruel. You have no control 
over what font the reader is going to view these formula with. Please 
consider a symbol that is less likely to be ambiguous.

13) An observation, not necessarily a request for change: It took me 
some time to convince myself that the description in 4.1.5.2 and the 
code in ec_tell_frac did the same thing.

14) Section 4.2.7.5.7 (for future efforts, please consider restructuring 
when you find yourself creating outlines that go this deep) - The 
constant 163838 is not explained in the text, but it is in the code. 
Could you move the explanation into the document? (And please do the 
same with any other constants that have an explanation like this).

15) To help avoid confusion about whether the URIs provided in the 
reference section are informative or normative references, please recast 
them as references and put them in the intended section. See RFC6581 for 
a recent example of using URIs in references.

16) The language in section A.2 will not age well after this is 
published as an RFC. At least add "As of the time of publication of this 
memo". It would also be good to clarify here that any changes in the 
repository and snapshots you are pointing to are NOT changes to the 
standard.

17) Similarly, references to the active project from inside the code 
contained here will not age well. The github reference in the README for 
instance will almost certainly be wrong in a few years, and it 
introduces ambiguity about whether what it's pointing to is part of the 
standard. Please consider making a version of the README that's specific 
to this particular snapshot (and say "this RFC" or "this memo" there 
instead of "this draft")

18) The sed in the draft for extracting the archive uses a gnu 
extension. It will not work for posix sed. There, \s matches a 
lower-case S. This would be more portable:
s/^...###//

19) I've asked several people to sanity check the build. Here is some 
important feedback I've received:
*) The Makefile uses GNU extensions extensively, and hardcodes the use 
of gcc (in spite of the claim that the code should compile with any C89 
or C99 compiler). It shouldn't be hard to remove the gnuisms from the 
makefile so that it would work on a strictly posix system (and more 
likely work with windows systems).
*) The compile generates warnings:
celt/bands.c:204: warning: unused parameter ‘CC’
src/opus_decoder.c:533: warning: ‘count’ may be used uninitialized in 
this function
*) run_vectors.sh relies on seq which doesn't exist as seq on many 
platforms. You can get it on OS/X through ports as gseq. This should be 
documented at least.
*) The build leaves .o files lying around in the source.

20) Consider adjusting the comment about being sure in ./celt/bands.c 
compute_band_energies

21) Throughout the code there are printfs and fprintfs of a debugging 
nature commented out (sometimes inconsistently). Why have these been 
left as is? Are you absolutely certain that there is no path to one of 
the statements that hasn't been commented out in the library code that 
would cause a runtime write to stdout or stderr? celt/fixed_debug.h is 
particular is pretty inconsistent with what's commented out and what isn't.

22) celt/cwrs.c contains nearly two pages of exposition in a comment, 
complete with references. Why isn't this in the main draft text instead?

23) celt/ecintrin.h contains a discussion of patents and prior art 
around abs(). An RFC isn't the place for this discussion - can this part 
of the comment be adjusted?

24) celt/entdec.c contains a very long comment that contains an opinion 
about encumberment of the range decode algorithm. It's not clear who the 
speaker is where the comment says "to my knowledge". Please adjust this 
taking publication in an RFC into consideration.

25) celt/kiss_fft.h has a block starting ATTENTION! that is 
advertisement for a set of files in a tools/ directory that does not 
appear to be part of this snapshot.

26) In celt/vq.c exp_rotation, there are some large blocks commented out 
that appear to be debugging diagnostics. Do they need to remain in the 
code? If so, could they be guarded with ifdefs instead of comments to 
make it clearer what's going on?

27) In celt/vq.c exp_rotation, there's a comment saying "I _think_ it is 
bit-exact". It's not clear who the speaker is. Is there still 
uncertainty about this?

28) In celt/vq.c exp_rotation, there's a comment that asks "Do we have 
sufficient accuracy here?" It's not clear if that's a question about 
whether the code is being accurate enough. If so, does that uncertainty 
still exist? Please adjust the comment appropriately.

29) include/opus_types.h contains a comment saying /* opus_types.h taken 
from libogg */. Please clarify - I don't think you meant to say this 
file was taken from the libogg source tree. What is it trying to say?

30) include/opus_defines.h says 'Best for "standard" 
VoIP/videoconference applications where listening quality and 
intelligibility matter most'. What does "standard" mean here?

31) silk/float/noise_shape_analysis_FLP.c : Similar to point 6) above, 
but there's no analogous text in the body of the draft to point to: What 
is a "monic" filter?

32) silk/MacroCount.h defines silk_PrintCount() but nothing seems to be 
using it. Is this another bit of debugging code that is (effectively) 
commented out? Does it need to be part of this snapshot?

33) Is "private" in the filenames of src/opus_private.h and the various 
silk/resampler_private files anything more than a historic artifact?

34) in src/opus_private.h, is this bit of documentation still accurate?:
* @note This interface is currently more aspiration than actuality. It's
* ultimately expected to bias an automatic signal classifier, but it 
currently
* just shifts the static bitrate to mode mapping around a little bit.

35) John Ridges reported an unreachable line on the list on Apr 13.

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Wed, 18 Apr 2012 13:44:58 -0700

Thank you for the detailed review. While we will of course address all 
the comments, I wanted to make one brief point now.

> complete with references. Why isn't this in the main draft text instead?

The exposition here primarily concerns a specific set of optimizations, 
designed to minimize the required ROM usage at the expense of more 
complex code. These are not the only way to implement these functions. 
For example, the code I posted at 
http://www.ietf.org/mail-archive/web/codec/current/msg02140.html simply 
uses a moderately-sized lookup table, and requires none of these 
explanations. It reimplements the entire decoding half of cwrs.c, 
roughly 300 lines of code, in one function that is less than 20 lines, 
including comments.

The text in the spec tries to limit itself to the simplest mathematical 
definition of the process, for ease of understanding, leaving specific 
optimizations to the implementor. It also references the same paper the 
code does.

---

From: Ron <[email-masked]>
Date: Thu, 19 Apr 2012 22:31:28 +0930

Hi Robert,

I'll add my warm thanks for the obvious effort and attention that you've
put into reviewing this draft too.  This is a non-trivial contribution
that I'm very happy to see.

> says. Do we still need this section? Can it be removed at this time?

The intention of this section was to make explicit that the authors are
granting additional rights to those of the TLP.  Similar to the grants
that were added in, for example:

http://tools.ietf.org/html/rfc3492#appendix-B
http://tools.ietf.org/html/rfc4501#section-8
http://tools.ietf.org/html/rfc5215#section-11
http://tools.ietf.org/html/rfc5334#section-12

The main difference here was a desire to be clear that the source code
components were not included in this additional grant and remained under
the usual IETF BSD licence for such parts.

While the TLP is fairly clear that the IETF avoids becoming involved in
making such additional grants and it is up to the document authors to
do so if they wish, there is no desire for this clause to add confusion;
so if there is better language that we should use there which fits with
the intent, then I'd welcome suggestions for how it might be improved.

I requested that this be added so that we could include this text in the
supporting documentation of the Debian packages for libopus, which we
would not be able to do without this additional grant of rights.

Best,
Ron

---

From: Jean-Marc Valin <[email-masked]>
Date: Tue, 24 Apr 2012 01:54:41 -0400

Hi Robert,

Thank you very much for spending the time to go through this draft. I've
attached the list of changes made by all the authors to address the
issues you raised.

Cheers,

	Jean-Marc

On 18/04/12 04:14 PM, Robert Sparks wrote:
> _______________________________________________
> codec mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/codec

---

From: Robert Sparks <[email-masked]>
Date: Thu, 26 Apr 2012 14:37:31 -0500

I've requested IETF LC for -12.

Some continued discussion on a few points.

On 4/24/12 12:54 AM, Jean-Marc Valin wrote:

> This paragraph was rewritten. The new text is:
>
> "Opus Custom is an OPTIONAL part of the specification that is defined to
> handle special sample rates and frame rates that are not supported by the
> main Opus specification. Use of Opus Custom is discouraged for all but 
> very
> special applications for which a frame size different from 2.5, 5, 10, 
> or 20&nbsp;ms is
> needed (for either complexity or latency reasons). Because Opus Custom is
> optional, applications using that part of the specification may not be 
> compatible
> with other applications implementing Opus. In Opus Custom operation,
> only the CELT layer is available, using the opus_custom_* function
> calls in opus_custom.h."
I think what you're trying to say (especially with the next to last 
sentence there) is that
a stream encoded using Opus Custom cannot be expected to be decodable by 
all Opus implementations.
Is there signaling inband about its use? If not, can the text say 
something about not using it unless you've
told the recipient about it out of band?
> This is probably due to the author's mis-understanding of the conventions
> involved in these bit diagrams. In every other context, bit 0 is the least
> significant, and bit 7 the most significant of a byte (and indeed this
> seemed to be true from the examples in other drafts, but these were bad
> examples). The diagrams have been updated to conform to the convention (as
> near as can be reverse engineered) from, e.g., RFC 796, and an explanatory
> note has been added to the beginning of section 3. Hopefully this makes
> the diagrams clearer. This also required some changes in the text (to
> reflect the ordering of fields, or where bits were referred to by number).
This is a big chunk of change to the text, and done impressively quickly.
It looks correctly executed to me. WG members should review this during 
IETF LC
_very_ carefully.
> Changed the text to be clearer:
> "As of the time of publication of this memo, up-to-date source code 
> implementing
> this standard is available in a Git repository." The section was also 
> renamed
> "Up-to-date Implementation".
Which still leaves confusion. The reference implementation is the code 
in the RFC. If
the code in git changes (and you obviously expect it to change), it is 
no longer the
reference implementation for this standard (it is a conforming 
implementation).
It needs to be clearer that the code that's in this document remains 
normative.
(if changes become necessary, they will be done with another RFC).
> Replaced draft with "RFC" and marked the git repository as being the
> "latest implementation of this standard at the time of publication"
Same issue as above. Please make it clear that the RFC _remains_ the 
definition of the standard.
> Yes, this was taken (and modified) from the libogg source tree (was
> originally called ogg_types).
Probably should say "based on" instead of "taken from" then?

RjS

---

From: Robert Sparks <[email-masked]>
Date: Thu, 26 Apr 2012 14:58:07 -0500

Note that I've requested IETF LC for version -12 (which includes this 
section).

I'd like to continue this conversation.

All the examples below are from before the current TLP (4) was in place.

As you note the TLP does not restrict the authors from making the grants 
you describe.
Why is it important to include this statement as part of _this_ document?

If you are able to do what you need without the section (which is what I 
think you're saying
below), removing it removes any _chance_ of confusion.

If you are not able to do what you want without the section, we need to 
talk about the TLP.
(If that's the case, could you call out the constraint that is in your way?)

RjS

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Thu, 26 Apr 2012 13:43:27 -0700

> It looks correctly executed to me. WG members should review this during

Yes, please. I asked a few others I knew had implemented Opus packet 
parsing about these diagrams and was told, "Oh yeah, I couldn't make 
heads or tails of them, so I just read the code." I'm glad Robert 
actually said something about them.

---

From: Ron <[email-masked]>
Date: Sat, 28 Apr 2012 06:02:02 +0930

On Thu, Apr 26, 2012 at 02:58:07PM -0500, Robert Sparks wrote:
> Note that I've requested IETF LC for version -12 (which includes
> this section).
> 

Ah, that detail had slipped past me, though I did refer to the text of the
current TLP to see if the reasons for adding this were still applicable.

> As you note the TLP does not restrict the authors from making the
> grants you describe.

We could indeed include this as a separate, external, grant statement.
It wasn't clear to me that this would be less confusing than having a
single document with a grant similar to the previous examples, but that
solution would also work for me, yes.

The curious twist with this particular document is that the normative
sections of it are the code, which are BSD licenced and allow the extra
modification rights granted by section 10 - while the informative parts
of the text may not be modified, and so could not be updated to document
any changes made to a particular implementation of the code without this
extra grant.  Since the encoder specification is also not normative, and
there is known scope to make improvements to it, the value of being able
to do this, both to the people making such changes, and to the WG which
may later find reason to fold them into an updated RFC, does seem more
than just whimsical in this case.

> If you are able to do what you need without the section (which is
> what I think you're saying
> If you are not able to do what you want without the section, we need
> to talk about the TLP.

The restrictions on modification in 3.c and 3.d are I think the only part
of the TLP that we'd need to waive to match the intent of section 10.

The crux here (for me) is that Debian (among others) considers the right
to modification an essential element to not spending our entire lives as
developers reinventing the wheels of our fathers from scratch.  Not having
to ask permission (or to charter a WG) before performing some experiment
(of which most will never go anywhere before being abandoned) is a powerful
enabler of progress.  The ones that are successful will naturally seek for
broader acceptance and formalisation (and may then reasonably seek to
engage with or charter a WG to do so).  But being able to do (and publish)
those initial experiments is an important part of the process.

This WG has been a living testament to how the initial informal legwork
to produce two good codecs could then come together in an open and formal
collaboration and produce a result so far outstanding beyond prior art and
expectations, that both doubters and advocates had to compete over whose
jaws have since dropped the most.  Signing off on that, and sealing it as
an interoperable standard now seems correct.  Erecting artificial barriers
to further experimentation down these lines though, I'm less sure about ...
The right to future experimentation does not conflict with the desire for
a fixed standard guaranteeing interoperability today.  Both are essential
if progress is to continue its steady march.

So if there is a possibility of future versions of the TLP finding some
suitable language to accommodate this case, without the need for these
additional grants, and while still meeting the concerns of the IETF about
maintaining the integrity and reliability of its publications, then that
would be a very interesting conversation to continue.  Though it's surely
out of the scope of this WG (and I'm not sure offhand exactly where the
appropriate place for that would be).

A grant which allowed modification, but which prohibited misrepresenting
modified versions as an RFC, or which required attribution to the original
document and source, would remove the blocking constraint.  The former is
the condition we placed on the extra grant in section 10, the latter is
indicated by the current TLP, and would be an acceptable restriction if it
also allowed for producing clearly marked, modified works outside of a
formal working group.

If nothing else, this would permit Debian to include the relevant RFCs in
the documentation of packages which implement them.  But my broader wish
is for something a little more than that, for more than just Debian too.

 Cheers,
 Ron