[opus] [PATCH] Support for Ambisonics and Projection API.

[Timothy B. Terriberry]

Drew Allen wrote:
> Please feel free to ask any questions or give any feedback you might
> have. :)

A few comments from a quick glance through the opusfile patch:

You unconditionally #include <opus_projection.h> in the public header, 
but you haven't updated the libopus version requirement in configure.ac. 
Ideally, of course, libopusfile would continue to work with older 
versions of libopus, just with a reduced feature set. The best would be 
if libopus itself provided a #define that its users could check at 
compile time to see if the projection API was available. See the 
definition of OP_SOFT_CLIP in opusfile's src/internal.h for the hacks 
I've had to do to detect other libopus API additions (hopefully we can 
avoid things like that here).

The addition of fields to the OpusHead struct is a breaking ABI change 
(specifically, all callers of opus_head_parse() would need to be rebuilt 
from source).

Is OPUS_DEMIXING_MATRIX_SIZE_MAX future-proof? I seem to recall much 
larger matrices being allowed according to the spec. You'll note that 
OPUS_CHANNEL_COUNT_MAX is 255, even though opusfile currently only 
supports mappings with up to 8 channels (see the private 
OP_NCHANNELS_MAX used internally, for comparison). The idea was that I 
did not want to break ABI when we added support for new mappings.

One way to go about this without breaking ABI might be to add support 
for "extended" mapping data. I.e., we could create an 
opus_head_parse_ext() that takes an extra buffer to return the extended 
mapping data into. Since it looks like you're not parsing this matrix 
anyway, but returning it as a chunk of binary data, hopefully this 
wouldn't be too unergonomic. The limit on the caller's buffer size could 
be the limit imposed by the size of an Ogg page. That should be 
future-proof.

I'm not sure if it would make sense to have the existing 
opus_head_parse() work for the ambisonics mapping families, or just fail 
cleanly with an error. I'm not sure what you could usefully do without 
the matrix (specifically, what would you put in the mapping[] array that 
existing callers would do something sane with?).

Similarly, changing the argument to op_decode_cb_func also breaks ABI. 
One way to avoid this might be to add an 
op_set_projection_decode_callback() function, which takes a new callback 
function pointer that takes the OpusProjectionDecoder instead. Then an 
application would register both callbacks, and libopusfile would call 
the correct one for the current link in the file. If you have an 
ambisonics link, but no projection callback registered, then it simply 
wouldn't call a decode callback for packets in that link. That behavior 
might be a little surprising to existing applications, but since 
previously libopusfile couldn't decode those links at all, hopefully it 
wouldn't be too bad.

There are a few more style consistency issues (sort system includes 
alphabetically, always use braces for if bodies, unless the if clause 
and the body both fit on the same line, prefix function names with op_ 
instead of _). In general, try to follow the local style.

I haven't reviewed the implementation changes in detail, but let's work 
out the high-level things first.