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

[Jean-Marc Valin <[e-mail]>]

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