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

Mark Harris mark.hsj at gmail.com
Sun Nov 19 01:47:58 UTC 2017


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


More information about the opus mailing list