opus - Mar 2018 - [PATCH] Support for Ambisonics and Projection API.

Hello all,

Attached are patches for libopusenc, opusfile and opus-tools in order to
provide support for Ambisonics and the Projection API for encoding/decoding
channel mapping 253 and 254.

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

Cheers,
Drew
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20180306/ea47482b/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libopusenc-Support-for-Ambisonics-and-Projection-API.patch
Type: text/x-patch
Size: 19762 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20180306/ea47482b/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: opusfile-Support-for-Ambisonics-and-Projection-API.patch
Type: text/x-patch
Size: 12344 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20180306/ea47482b/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: opustools-Support-for-Ambisonics-and-Projection-API.patch
Type: text/x-patch
Size: 4419 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20180306/ea47482b/attachment-0005.bin>

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.

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>

Hi Drew,

Had a look at your libopusenc patch and I think overall it's on the
right track. I have a few comments though:
1) It's changing the meaning of MAX_HEADER_SIZE. That value isn't meant
to mean the max size of the Opus header, but rather it's the max size of
an Ogg page header. It should not be changed here.

2) In general, I think you should use dynamic allocation for the Ogg
header, because libopusenc *could* be linked to a newer version of
libopus that supports order-10 projection encoding. libopusenc should
not make assumptions about what libopus version it is linked against.

3) Not sure on that one, but wouldn't the stuff you do in
_generic_encoder_fetch_header_data() be cleaner if you moved it to
_ope_opus_header_to_packet()? Considering point 2) above, maybe it's
possible to avoid the dynamic allocation by copying the data straight to
the packet?

4) I *think* there's a way to make a _generic_encoder_ctl() through the
use of a macro. The macro would look something like (untested):
#define _generic_encoder_ctl(st, request) ((st->family == 253) ?
opus_projection_encoder_ctl(st->pr, request) :
opus_multistream_encoder_ctl(st->ms, request))
The macro would of course have a simpler definition when ambisonics
support is disabled.
Based on a quick experiment I made, I think the _generic_encoder_ctl()
above should work even with multiple-argument requests.

	Jean-Marc

On 03/06/2018 06:42 PM, Drew Allen wrote:
> Hello all,
> 
> Attached are patches for libopusenc, opusfile and opus-tools in order to
> provide support for Ambisonics and the Projection API for
> encoding/decoding channel mapping 253 and 254.
> 
> Please feel free to ask any questions or give any feedback you might
> have. :)
> 
> Cheers,
> Drew
> 
> 
> _______________________________________________
> opus mailing list
> opus at xiph.org
> http://lists.xiph.org/mailman/listinfo/opus
>