[opus] [PATCH] Support for Ambisonics and Projection API.
Timothy B. Terriberry
tterribe at xiph.org
Wed Mar 7 02:25:59 UTC 2018
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
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
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.
More information about the opus