opus - Nov 2017 - [PATCH] Support for Channel Mapping 253.

On 11/09/2017 01:58 PM, Drew Allen wrote:
> Attached is a quick patch that addresses a bug when exporting the matrix
> from the encoder.
Actually, I don't see what your encoder change is supposed to do. Are
there cases where demixing_matrix->rows != nb_output_streams ?

Cheers,

	Jean-Marc

> Cheers,
> Drew
> 
> On Wed, Nov 8, 2017 at 4:44 PM Drew Allen <bitllama at google.com
> <mailto: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
>     <mailto: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>
>         > <mailto: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>
>         >     <mailto: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>>
>         >         > <mailto: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>>>
>         >         >         > <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,
>         >         >         >
>         >         >         >     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>>>>
>         >         >         >     > <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 <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>>>>>
>         >         >         >     >     >
<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 <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>>>>>
>         >         >         <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 <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>>>>
>         >         >         <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
>         >         >         >     >     >
>         >         >         >     >
>         >         >         >
>         >         >
>         >         >
>         >
>

Hi Drew,

Some additional comments on your mapping family 253 changes:

 1) mapping_matrix_get_data: The computed pointer is not correct;
matrix should be cast to char * before the addition to avoid
incrementing in units of the structure size.

 2) mapping_matrix_get_size: "rows * cols * sizeof(opus_int16)" may
not have the necessary alignment.  There may be alignment padding
after the struct and also after the opus_int16 array and this function
is assumed to return a size that includes both.  Also the
multiplication and return value should use opus_int32 since int may
not be large enough for order 11 to 14.  A complication is that
align() currently takes and returns "int".

 3) In several places including the intermediate multiplication in the
computation of "tmp" in mapping_matrix_multiply_short, and even in the
MATRIX_INDEX macro, multiplication may overflow if int is 16 bits.
One of the operands of the multiplication should be cast to opus_int32
in such cases to ensure a 32-bit multiply and avoid overflow.

 4) get_multistream_decoder: align() must be applied separately to
sizeof(OpusProjectionDecoder) and st->demixing_matrix_size_in_bytes
since they may each have padding.  Including the alignment padding in
demixing_matrix_size_in_bytes as in (5) avoids the second call to
align().  Similarly in opus_projection_decoder_get_size, align()
should just be applied to sizeof(OpusProjectionDecoder); in that case
the other sizes may have their own alignment padding but it is already
included in their size assuming (2) is addressed.

 5) opus_projection_decoder_init: demixing_matrix_size_in_bytes
doesn't include the MappingMatrix struct or alignment but is used to
determine the offset of the decoder; it should be obtained from
mapping_matrix_get_size() and the field should be opus_int32.

 6) opus_projection_decoder_init: demixing_matrix_size is in bytes but
is used as a parameter to ALLOC() which expects the number of
elements, so twice as much memory is allocated.

 7) opus_projection_ambisonics_encoder_init: It appears that
uninitialized values are used from the matrices if order_plus_one is 1
or 5 to 15.

 8) opus_projection_ambisonics_encoder_init:
mixing_matrix_size_in_bytes and demixing_matrix_size_in_bytes should
be opus_int32.

 - Mark

Hi Mark,

Attached are corrections based on your comments. I will extend these to the
patch I recently submitted to fix memory issues once that is resolved as
well.

Cheers,
Drew

On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <mark.hsj at gmail.com> wrote:
> Hi Drew,
>
> Some additional comments on your mapping family 253 changes:
>
>  1) mapping_matrix_get_data: The computed pointer is not correct;
> matrix should be cast to char * before the addition to avoid
> incrementing in units of the structure size.
>
> Done.
>  2) mapping_matrix_get_size: "rows * cols * sizeof(opus_int16)"
may
> not have the necessary alignment.  There may be alignment padding
> after the struct and also after the opus_int16 array and this function
> is assumed to return a size that includes both.  Also the
> multiplication and return value should use opus_int32 since int may
> not be large enough for order 11 to 14.  A complication is that
> align() currently takes and returns "int".
>
> We specify in the IETF doc that we are limited to a max matrix size of
65025 bytes (and with 2 bytes per cell, that limits us to only order 11 for
a full-order square matrix). I can recast align before summing them though.

>  3) In several places including the intermediate multiplication in the
> computation of "tmp" in mapping_matrix_multiply_short, and even
in the
> MATRIX_INDEX macro, multiplication may overflow if int is 16 bits.
> One of the operands of the multiplication should be cast to opus_int32
> in such cases to ensure a 32-bit multiply and avoid overflow.
>
> Done.
>  4) get_multistream_decoder: align() must be applied separately to
> sizeof(OpusProjectionDecoder) and st->demixing_matrix_size_in_bytes
> since they may each have padding.  Including the alignment padding in
> demixing_matrix_size_in_bytes as in (5) avoids the second call to
> align().  Similarly in opus_projection_decoder_get_size, align()
> should just be applied to sizeof(OpusProjectionDecoder); in that case
> the other sizes may have their own alignment padding but it is already
> included in their size assuming (2) is addressed.
>
> Done.
>  5) opus_projection_decoder_init: demixing_matrix_size_in_bytes
> doesn't include the MappingMatrix struct or alignment but is used to
> determine the offset of the decoder; it should be obtained from
> mapping_matrix_get_size() and the field should be opus_int32.
>
> This value is only meant to represent the "data" portion of the
mappingmatrix struct. Note that we include the struct size when computing
the multistream decoder pointer in get_multistream_decoder().
I will make it opus_int32 though.

 6) opus_projection_decoder_init: demixing_matrix_size is in bytes
but
> is used as a parameter to ALLOC() which expects the number of
> elements, so twice as much memory is allocated.
>
> Done.
>  7) opus_projection_ambisonics_encoder_init: It appears that
> uninitialized values are used from the matrices if order_plus_one is 1
> or 5 to 15.
>
I will rename the function that validates the correct order before this is
called (It fails and hence the init() func fails if order_plus_one is < 2
or > 4.)
>
>  8) opus_projection_ambisonics_encoder_init:
> mixing_matrix_size_in_bytes and demixing_matrix_size_in_bytes should
> be opus_int32.
Done.
>
>  - Mark
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20171121/22db2c25/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Resolve-align-issues-in-Projection-API.patch
Type: application/octet-stream
Size: 9733 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20171121/22db2c25/attachment.obj>