[opus] [PATCH] Support for Ambisonics

Tue Mar 20 16:36:20 UTC 2018

Attached is an updated patch based on Jean-Marc and Mark's comments. :)

Cheers,
Drew

On Tue, Mar 20, 2018 at 9:20 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:

> On 03/20/2018 11:51 AM, Drew Allen wrote:
> > Just to confirm, I would use opeint_* for all the
> > OpusGenericEncoder-related functions?
>
> Correct. Or you can also use oge_ if you like. Just don't use something
> that starts with an underscore.
>
>
> > On Tue, Mar 20, 2018 at 8:38 AM Jean-Marc Valin <jmvalin at jmvalin.ca
> > <mailto:jmvalin at jmvalin.ca>> wrote:
> >
> >     Hi Mark, Drew,
> >
> >     On 03/20/2018 02:40 AM, Mark Harris wrote:
> >     > + int _oge_use_projection(int channel_mapping);
> >     >
> >     > These functions are part of libopusenc, so I'd expect them to have
> an
> >     > ope prefix like the other functions in the libopusenc library.
> >
> >     I'd like to avoid using the ope_ prefix for functions that's aren't
> in
> >     the public API. Right now there are other functions with a leading
> >     underscore, so we'll have to fix them as well (not in this patch of
> >     course). Maybe an "opeint_" prefix would do the job here (unless
> anyone
> >     has a better idea)?
> >
> >     > +int ope_encoder_deferred_init_with_mapping(OggOpusEnc *enc, int
> >     > family, int streams,
> >     >      int coupled_streams, const unsigned char *mapping) {
> >     >    int ret;
> >     >    int i;
> >     >
> >     > This code is allowing family 253 for a deferred init, but does not
> >     > create a projection encoder in that case, so it looks like it will
> >     > fail when writing the id header since it won't be able to get the
> >     > demixing matrix.  It should probably not be allowing family 253
> here.
> >
> >     Actually, in the case of ope_encoder_deferred_init_with_mapping(), I
> >     think it's probably best to just keep the existing code (not use
> >     wrappers), since that function cannot be used by the projection
> encoder
> >     at all.
> >
> >             Jean-Marc
> >
> >
> >     >  - Mark
> >     >
> >     >
> >     > On Mon, Mar 19, 2018 at 3:00 PM, Drew Allen <bitllama at google.com
> >     <mailto:bitllama at google.com>> wrote:
> >     >> Hi Jean-Marc,
> >     >>
> >     >> I've modified my patches for libopus and libopusenc based on your
> >     >> suggestions.
> >     >>
> >     >> Cheers,
> >     >> Drew
> >     >>
> >     >> On Mon, Mar 19, 2018 at 2:05 PM Jean-Marc Valin
> >     <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> wrote:
> >     >>>
> >     >>> Hi Drew,
> >     >>>
> >     >>> I think the libopusenc patch is better, but there's still a few
> >     issues
> >     >>> left:
> >     >>> 1) The static MAX_PACKET_BUFFER_SIZE value is still problematic
> >     because
> >     >>> if you link libopusenc with a new version of libopus that
> supports
> >     >>> higher order projection or just more projection channels for
> >     order 3,
> >     >>> then you will overflow the buffer. I think what you'd want is a
> >     >>> _ope_opus_header_get_size() call that would return how large the
> >     header
> >     >>> *actually* is. Then you can use that value instead of
> >     >>> MAX_PACKET_BUFFER_SIZE in init_stream()
> >     >>> 2) I think the remaining if()s in ope_encoder_ctl() can also be
> >     removed
> >     >>> by adding another ctl() macro (like _oge_ctl()) with an extra
> >     argument.
> >     >>> In the case of OPUS_MULTISTREAM_GET_ENCODER_STATE_REQUEST, you
> can
> >     >>> simply use _oge_ctl(enc->st,
> >     >>> OPUS_MULTISTREAM_GET_ENCODER_STATE(stream_id, value))
> >     >>> 3) On libopus itself, why "#define OPUS_HAVE_OPUS_PROJECTION_H
> 9000"
> >     >>> instead of just "#define OPUS_HAVE_OPUS_PROJECTION_H"?
> >     >>>
> >     >>> Cheers,
> >     >>>
> >     >>>         Jean-Marc
> >     >>>
> >     >>> On 03/19/2018 02:53 PM, Drew Allen wrote:
> >     >>>>
> >     >>>> On Mon, Mar 19, 2018 at 11:52 AM Drew Allen
> >     <bitllama at google.com <mailto:bitllama at google.com>
> >     >>>> <mailto:bitllama at google.com <mailto:bitllama at google.com>>>
> wrote:
> >     >>>>
> >     >>>>     Hello all,
> >     >>>>
> >     >>>>     Sorry for the delay (got really sick last week).
> >     >>>>
> >     >>>>     Attached are updated patches for libopus, libopusenc,
> >     opusfile and
> >     >>>>     opus-tools.
> >     >>>>
> >     >>>>     Note that the patches for libopusenc, opusfile and
> >     opus-tools are
> >     >>>>     dependent on the patch for libopus.
> >     >>>>
> >     >>>>     Please let me know if you have any additional followup
> >     comments or
> >     >>>>     questions.
> >     >>>>
> >     >>>>     Cheers,
> >     >>>>     Drew
> >     >>>>
> >     >>>>
> >     >>>>
> >     >>>> _______________________________________________
> >     >>>> opus mailing list
> >     >>>> opus at xiph.org <mailto:opus at xiph.org>
> >     >>>> http://lists.xiph.org/mailman/listinfo/opus
> >     >>>>
> >     >>
> >     >>
> >     >> _______________________________________________
> >     >> opus mailing list
> >     >> opus at xiph.org <mailto:opus at xiph.org>
> >     >> http://lists.xiph.org/mailman/listinfo/opus
> >     >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20180320/2131faf5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: libopusenc-Support-for-Ambisonics.patch
Type: text/x-patch
Size: 17918 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20180320/2131faf5/attachment-0001.bin>