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

[Drew Allen]

Hi Jean-Marc,

Both apply to master branch. I'll have to address  Mark's comments on that
latest memory patch as well.

On Mon, Nov 20, 2017 at 6:45 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:

> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171121/c4ecffb5/attachment.html>