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

[Robert Sparks <[e-mail]>]

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:


> > 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.
>
> 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 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?
>
>
> > 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.
>
> 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.
>
>
> > 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.
>
> 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).
>
> > 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")
>
> 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.
>
> > 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?
>
> 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