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

[Drew Allen]

Excellent, thanks Tim for writing back so quickly!

Cheers,
Drew

On Tue, Mar 6, 2018 at 6:25 PM, Timothy B. Terriberry <tterribe at xiph.org>
wrote:

> 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).
>
I think this is relatively easy to add to libopus so I'll wrap the function
calls and the #include around this #define. This will also allow us to not
have to explicitly enable ambisonics. :)

>
> 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.
>
> I will change the define to make it future-proof as well as make it a
separate struct (OpusHeadExt, for example) and make the call the
opus_head_parse_ext() in order not to break the ABI.


> 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?).
>
I would think that it's not a good idea to try to decode the streams
without handling the demixing matrix, so I would lean towards a graceful
fail in that case.

>
> 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.
>
> Make sense and an easy fix.

> 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'll take note of these suggestions and cleanup the style accordingly. :)


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

Thanks again Tim, I'll send over another patch once all of these have been
addressed.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20180306/cd960a96/attachment.html>