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

[Jean-Marc Valin]

Hi Drew,

In which order do these patches apply?

	Jean-Marc

On 11/20/2017 08:57 PM, Drew Allen wrote:
> 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
> <mailto: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
> 
> 
> 
> _______________________________________________
> opus mailing list
> opus at xiph.org
> http://lists.xiph.org/mailman/listinfo/opus
>