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

[Drew Allen]

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>