[opus] [PATCH] Support for Channel Mapping 253.

Drew Allen bitllama at google.com
Thu Nov 9 18:58:38 UTC 2017


Hi all,

Attached is a quick patch that addresses a bug when exporting the matrix
from the encoder.

Cheers,
Drew

On Wed, Nov 8, 2017 at 4:44 PM Drew Allen <bitllama at google.com> wrote:

> Sure, ill send that asap
> On Wed, Nov 8, 2017 at 4:44 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:
>
>> Hi Drew,
>>
>> Your ambisonics patch is already merged. Can you send a patch that
>> applies to master?
>>
>>         Jean-Marc
>>
>> On 11/08/2017 07:05 PM, Drew Allen wrote:
>> > Hey Jean-Marc,
>> >
>> > I found a bug regarding exporting the matrix that wasn't always grabbing
>> > the correct values, causing incorrect mixing behavior. This patch
>> > resolves that issue.
>> >
>> > Cheers,
>> > Drew
>> >
>> > On Tue, Nov 7, 2017 at 3:04 PM Drew Allen <bitllama at google.com
>> > <mailto:bitllama at google.com>> wrote:
>> >
>> >     Awesome!!!! Ill think about a strategy for that soon. Glad it could
>> >     finally make it in.
>> >
>> >     On Tue, Nov 7, 2017 at 3:01 PM Jean-Marc Valin <jmvalin at jmvalin.ca
>> >     <mailto:jmvalin at jmvalin.ca>> wrote:
>> >
>> >         Hi Drew,
>> >
>> >         Thanks for the update. Your patch is now in master. Now, it
>> would be
>> >         good if you could think of a way to reduce the stack usage as we
>> >         discussed.
>> >
>> >         Cheers,
>> >
>> >                 Jean-Marc
>> >
>> >         On 11/07/2017 04:28 PM, Drew Allen wrote:
>> >         > Here's another patch. Cheers!
>> >         >
>> >         > On Mon, Nov 6, 2017 at 10:08 AM Drew Allen
>> >         <bitllama at google.com <mailto:bitllama at google.com>
>> >         > <mailto:bitllama at google.com <mailto:bitllama at google.com>>>
>> wrote:
>> >         >
>> >         >     Ok great. I'll make those changes you suggested and get
>> >         back to you
>> >         >     this morning.
>> >         >
>> >         >     Cheers,
>> >         >     Drew
>> >         >
>> >         >
>> >         >
>> >         >         On 11/03/2017 02:51 PM, Drew Allen wrote:
>> >         >         > Here's another one.
>> >         >         >
>> >         >         > On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin
>> >         <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>
>> >         <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>
>> >         >         > <mailto:jmvalin at jmvalin.ca
>> >         <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca
>> >         <mailto:jmvalin at jmvalin.ca>>>> wrote:
>> >         >         >
>> >         >         >     Hi Drew,
>> >         >         >
>> >         >         >     We're getting there... Some minor comments:
>> >         >         >
>> >         >         >     1) The public header file should not have an
>> >         >         >     #ifdef ENABLE_EXPERIMENTAL_AMBISONICS
>> >         >         >     since that would require the user code to
>> define it.
>> >         >         >
>> >         >         > Done
>> >         >         >
>> >         >         >     2) Why do you have #define MAPPING_MATRIX_C ?
>> >         >         >
>> >         >         > No idea. Fixed.
>> >         >         >
>> >         >         >
>> >         >         >     3) Looks like MAPPING_MATRIX_MAX_SIZE is not
>> >         longer useful, right?
>> >         >         >
>> >         >         > Yup. Removed.
>> >         >         >
>> >         >         >
>> >         >         >     4) Even though it's not strictly necessary here,
>> >         please add parentheses
>> >         >         >     to the definition of MATRIX_INDEX() to avoid
>> >         nasty surprises in the
>> >         >         >     future (e.g. MATRIX_INDEX(...)*sizeof(foo) would
>> >         be really bad).
>> >         >         >
>> >         >         > Done.
>> >         >         >
>> >         >         >
>> >         >         >     Cheers,
>> >         >         >
>> >         >         >             Jean-Marc
>> >         >         >
>> >         >         >
>> >         >         >     On 10/31/2017 04:10 PM, Drew Allen wrote:
>> >         >         >     > Hi Jean-Marc,
>> >         >         >     >
>> >         >         >     > Thanks so much for your review. Attached are
>> >         my comments and an
>> >         >         >     updated
>> >         >         >     > patch.
>> >         >         >     >
>> >         >         >     > On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc
>> Valin
>> >         >         >     <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>
>> >         <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>
>> >         >         <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca
>> >
>> >         <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>>
>> >         >         >     > <mailto:jmvalin at jmvalin.ca
>> >         <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca
>> >         <mailto:jmvalin at jmvalin.ca>>
>> >         >         <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca
>> >
>> >         <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>>>>
>> wrote:
>> >         >         >     >
>> >         >         >     >     Hi Drew,
>> >         >         >     >
>> >         >         >     >     I've had some time to dig more deeply into
>> >         your
>> >         >         patch. Here's
>> >         >         >     some more
>> >         >         >     >     in-depth comments:
>> >         >         >     >
>> >         >         >     >     1) I note that your OpusMSEncoder struct
>> in
>> >         >         private.h adds a
>> >         >         >     >     subframe_mem[] that's not in
>> >         >         opus_multistream_encoder.c. I
>> >         >         >     assume it's
>> >         >         >     >     due to a merge problem (that field was
>> >         removed some
>> >         >         time ago),
>> >         >         >     but can
>> >         >         >     >     you confirm/fix the issue?
>> >         >         >     >
>> >         >         >     > Done. Yes, this is a merge conflict.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     2) I noticed your patch adds many
>> OPUS_EXPORT
>> >         >         declarations.
>> >         >         >     OPUS_EXPORT
>> >         >         >     >     is only meant for functions exposed to the
>> >         API (and
>> >         >         in the
>> >         >         >     include/
>> >         >         >     >     directory), but I see several of these in
>> >         >         mapping_matrix.h,
>> >         >         >     which I
>> >         >         >     >     think is incorrect.
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     3)
>> >         opus_projection_ambisonics_encoder_init() (which
>> >         >         should be
>> >         >         >     >     OPUS_EXPORT) is missing from
>> opus_projection.h
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     4) I believe mapping_matrix_get_size()
>> >         should be
>> >         >         returning
>> >         >         >     >     align(sizeof(MappingMatrix)) + rows *
>> cols *
>> >         >         sizeof(opus_int16)
>> >         >         >     >     rather than:
>> >         >         >     >     align(sizeof(MappingMatrix) + rows * cols
>> *
>> >         >         sizeof(opus_int16))
>> >         >         >     >     to match what mapping_matrix_get_data()
>> does
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     5) I'm not sure I understand the purpose
>> of
>> >         >         >     mapping_matrix_validate().
>> >         >         >     >     Can the user really cause the matrix not
>> to be
>> >         >         valid? If yes,
>> >         >         >     then can't
>> >         >         >     >     this cause problems even earlier in the
>> >         code? If
>> >         >         not, then
>> >         >         >     maybe an
>> >         >         >     >     assertion would be better. Also, is the
>> >         size of the
>> >         >         matrix the
>> >         >         >     only
>> >         >         >     >     thing you can validate?
>> >         >         >     >
>> >         >         >     > Removed it. You're right, it's functionally
>> >         impossible
>> >         >         for the user to
>> >         >         >     > create an invalid matrix with the current API.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     6) mapping_matrix_init() returns an error
>> >         code, but
>> >         >         the code
>> >         >         >     that's
>> >         >         >     >     calling it never checks it. Considering
>> >         that it's an
>> >         >         internal
>> >         >         >     call, I
>> >         >         >     >     would say it's probably better not to
>> return
>> >         >         anything, but
>> >         >         >     instead to
>> >         >         >     >     assert in the function.
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     7) Same for
>> >         mapping_matrix_multiply_short() and
>> >         >         >     >     mapping_matrix_multiply_float(), the
>> >         return value isn't
>> >         >         >     checked. So it
>> >         >         >     >     should either be replaced by an asssert or
>> >         (if the
>> >         >         user can
>> >         >         >     cause a
>> >         >         >     >     failure) actually checked.
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >     8) I see there's a "gain" field in the
>> >         matrix, but I
>> >         >         can't see
>> >         >         >     it used
>> >         >         >     >     anywhere. Did I miss something?
>> >         >         >     >
>> >         >         >     > Gain should be pulled out by the user via
>> >         >         >     > a  OPUS_PROJECTION_GET_DEMIXING_MATRIX_GAIN
>> >         call and add
>> >         >         it to the
>> >         >         >     > overall output gain. We assume the mixing
>> >         matrix gain is
>> >         >         always
>> >         >         >     zero and
>> >         >         >     > that this only matters for the output gain
>> >         from the
>> >         >         demixing matrix.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     9) In get_streams_from_channels(), I think
>> >         it'd be
>> >         >         simpler to just
>> >         >         >     >     replace:
>> >         >         >     >     *streams = channels / 2 + (channels % 2
>> == 1);
>> >         >         >     >     with:
>> >         >         >     >     *streams = (channels+1) / 2;
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     10) In
>> >         opus_projection_ambisonics_encoder_init(),
>> >         >         you to see
>> >         >         >     if streams
>> >         >         >     >     and coupled_streams are NULL, but unless I
>> >         missed
>> >         >         something I
>> >         >         >     don't
>> >         >         >     >     think there's any valid case where you
>> >         wouldn't want
>> >         >         to get those
>> >         >         >     >     values. If you don't have them, then you
>> >         have no way of
>> >         >         >     knowing what
>> >         >         >     >     mapping the encoder used, so no way of
>> >         decoding.
>> >         >         Instead, I
>> >         >         >     would just
>> >         >         >     >     return OPUS_BAD_ARG if they're NULL.
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     11) So one issue I just noticed is that
>> >         >         >     opus_projection_encode() and
>> >         >         >     >     opus_projection_encode_float() (same for
>> the
>> >         >         decoder) use
>> >         >         >     arbitrarily
>> >         >         >     >     large amounts of stack memory for "buf".
>> In
>> >         >         >     opus_multistream_encode()
>> >         >         >     >     that's avoided by converting just two
>> >         channels at a
>> >         >         time, but
>> >         >         >     here it's
>> >         >         >     >     not quite clear how to do that without
>> >         duplicating a
>> >         >         lot of the
>> >         >         >     >     multistream code. If we can't address the
>> >         issue with
>> >         >         this
>> >         >         >     patch, the
>> >         >         >     >     least would be to abort when trying to use
>> >         these
>> >         >         calls with
>> >         >         >     >     NONTHREADSAFE_PSEUDOSTACK
>> >         >         >     >
>> >         >         >     > Done. Let me know if we should try designing
>> >         something
>> >         >         around this.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     12) I think opus_projection_decoder_ctl()
>> >         is missing a
>> >         >         >     va_end() call.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     > Done.
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     Cheers,
>> >         >         >     >
>> >         >         >     >             Jean-Marc
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >
>> >         >         >     >     On 10/12/2017 05:44 PM, Drew Allen wrote:
>> >         >         >     >     > thanks for all your feedback. here's the
>> >         revised
>> >         >         patch:
>> >         >         >     >     >
>> >         >         >     >     > On Wed, Oct 11, 2017 at 2:20 PM Timothy
>> >         B. Terriberry
>> >         >         >     >     <tterribe at xiph.org
>> >         <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org
>> >         <mailto:tterribe at xiph.org>>
>> >         >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>
>> >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>
>> >         >         >     <mailto:tterribe at xiph.org
>> >         <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org
>> >         <mailto:tterribe at xiph.org>>
>> >         >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>
>> >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>>
>> >         >         >     >     > <mailto:tterribe at xiph.org
>> >         <mailto:tterribe at xiph.org>
>> >         >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>
>> >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>
>> >         >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org
>> >>>
>> >         >         >     <mailto:tterribe at xiph.org
>> >         <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org
>> >         <mailto:tterribe at xiph.org>>
>> >         >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>
>> >         <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>>>>
>> wrote:
>> >         >         >     >     >
>> >         >         >     >     >     Jean-Marc Valin wrote:
>> >         >         >     >     >     > I think you'll want something
>> like:
>> >         >         >     >     >     >
>> >         >         (opus_int16)((unsigned)demixing_matrix[2*i+1] << 8)
>> >         >         >     >     >     > (though you might want to check it
>> >         too)
>> >         >         >     >     >
>> >         >         >     >     >     FWIW, we use the construct
>> >         >         >     >     >        int s = buf[2*i + 1] << 8 |
>> buf[2*i];
>> >         >         >     >     >        s = ((s & 0xFFFF) ^ 0x8000) -
>> 0x8000;
>> >         >         >     >     >
>> >         >         >     >     >     to manually sign-extend the result
>> in
>> >         >         opus_compare (and also
>> >         >         >     >     opus_demo).
>> >         >         >     >     >     If you ultimately cast s to
>> >         (opus_int16), then I'm
>> >         >         >     pretty sure
>> >         >         >     >     most
>> >         >         >     >     >     compilers will turn the second line
>> >         into a
>> >         >         no-op and
>> >         >         >     optimize
>> >         >         >     >     it away.
>> >         >         >     >     >
>> >          _______________________________________________
>> >         >         >     >     >     opus mailing list
>> >         >         >     >     >     opus at xiph.org <mailto:opus at xiph.org
>> >
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>
>> >         >         <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>>
>> >         >         >     <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>
>> >         >         <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>>>
>> >         >         <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>
>> >         >         >     <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>>
>> >         >         >     >     <mailto:opus at xiph.org
>> >         <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto:
>> opus at xiph.org>>
>> >         >         <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto: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>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>
>> >         >         <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>>
>> >         >         <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto:opus at xiph.org <mailto:opus at xiph.org>>
>> >         >         >     <mailto:opus at xiph.org <mailto:opus at xiph.org>
>> >         <mailto: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/20171109/7bb62065/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-matrix-export-via-CTL-func.patch
Type: text/x-patch
Size: 2308 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171109/7bb62065/attachment-0001.bin>


More information about the opus mailing list