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

[Drew Allen]

Hello Tim et all,

Attached is a revised patch for opusfile which more gracefully addresses
the listed concerns. Note that this patch is dependent on the most recent
patch I sent out for opus (
http://lists.xiph.org/pipermail/opus/2018-March/004146.html)

Awaiting your feedback. :)

Cheers,
Drew



On Tue, Mar 6, 2018 at 9:14 PM Drew Allen <bitllama at google.com> wrote:

> 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/20180309/15406086/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: opusfile-Support-for-ambisonics.patch
Type: text/x-patch
Size: 21264 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20180309/15406086/attachment-0001.bin>