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

Jean-Marc Valin jmvalin at jmvalin.ca
Tue Nov 7 23:00:59 UTC 2017


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>> 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>>> 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>>>> 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>>>>> 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>>>>
>         >     >     >     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>>>
>         >     >     > http://lists.xiph.org/mailman/listinfo/opus
>         >     >     >
>         >     >
>         >
> 
> 


More information about the opus mailing list