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

[Robert Sparks <[e-mail]>]

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.