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

Jean-Marc Valin jmvalin at jmvalin.ca
Thu Nov 2 16:54:31 UTC 2017


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.

2) Why do you have #define MAPPING_MATRIX_C ?

3) Looks like MAPPING_MATRIX_MAX_SIZE is not longer useful, right?

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).

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>> 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>>> 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>>
>     >     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
>     >
> 



More information about the opus mailing list