<div dir="ltr">Hi Jean-Marc,<div><br></div><div>Both apply to master branch. I'll have to address  Mark's comments on that latest memory patch as well.</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 20, 2017 at 6:45 PM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca">jmvalin@jmvalin.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Drew,<br>
<br>
In which order do these patches apply?<br>
<br>
        Jean-Marc<br>
<br>
On 11/20/2017 08:57 PM, Drew Allen wrote:<br>
> Hi Mark, <br>
><br>
> Attached are corrections based on your comments. I will extend these to<br>
> the patch I recently submitted to fix memory issues once that is<br>
> resolved as well.<br>
><br>
> Cheers,<br>
> Drew<br>
><br>
> On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <<a href="mailto:mark.hsj@gmail.com" target="_blank">mark.hsj@gmail.com</a><br>
> <mailto:<a href="mailto:mark.hsj@gmail.com" target="_blank">mark.hsj@gmail.com</a>>> wrote:<br>
><br>
>     Hi Drew,<br>
><br>
>     Some additional comments on your mapping family 253 changes:<br>
><br>
>      1) mapping_matrix_get_data: The computed pointer is not correct;<br>
>     matrix should be cast to char * before the addition to avoid<br>
>     incrementing in units of the structure size.<br>
><br>
> Done.<br>
>  <br>
><br>
>      2) mapping_matrix_get_size: "rows * cols * sizeof(opus_int16)" may<br>
>     not have the necessary alignment.  There may be alignment padding<br>
>     after the struct and also after the opus_int16 array and this function<br>
>     is assumed to return a size that includes both.  Also the<br>
>     multiplication and return value should use opus_int32 since int may<br>
>     not be large enough for order 11 to 14.  A complication is that<br>
>     align() currently takes and returns "int".<br>
><br>
> We specify in the IETF doc that we are limited to a max matrix size of<br>
> 65025 bytes (and with 2 bytes per cell, that limits us to only order 11<br>
> for a full-order square matrix). I can recast align before summing them<br>
> though.<br>
>  <br>
><br>
>      3) In several places including the intermediate multiplication in the<br>
>     computation of "tmp" in mapping_matrix_multiply_short, and even in the<br>
>     MATRIX_INDEX macro, multiplication may overflow if int is 16 bits.<br>
>     One of the operands of the multiplication should be cast to opus_int32<br>
>     in such cases to ensure a 32-bit multiply and avoid overflow.<br>
><br>
> Done.<br>
>  <br>
><br>
>      4) get_multistream_decoder: align() must be applied separately to<br>
>     sizeof(OpusProjectionDecoder) and st->demixing_matrix_size_in_bytes<br>
>     since they may each have padding.  Including the alignment padding in<br>
>     demixing_matrix_size_in_bytes as in (5) avoids the second call to<br>
>     align().  Similarly in opus_projection_decoder_get_size, align()<br>
>     should just be applied to sizeof(OpusProjectionDecoder); in that case<br>
>     the other sizes may have their own alignment padding but it is already<br>
>     included in their size assuming (2) is addressed.<br>
><br>
> Done. <br>
><br>
>      5) opus_projection_decoder_init: demixing_matrix_size_in_bytes<br>
>     doesn't include the MappingMatrix struct or alignment but is used to<br>
>     determine the offset of the decoder; it should be obtained from<br>
>     mapping_matrix_get_size() and the field should be opus_int32.<br>
><br>
> This value is only meant to represent the "data" portion of the<br>
> mappingmatrix struct. Note that we include the struct size when<br>
> computing the multistream decoder pointer in get_multistream_decoder().<br>
> I will make it opus_int32 though.<br>
><br>
>      6) opus_projection_decoder_init: demixing_matrix_size is in bytes but<br>
>     is used as a parameter to ALLOC() which expects the number of<br>
>     elements, so twice as much memory is allocated.<br>
><br>
> Done.<br>
>  <br>
><br>
>      7) opus_projection_ambisonics_encoder_init: It appears that<br>
>     uninitialized values are used from the matrices if order_plus_one is 1<br>
>     or 5 to 15.<br>
><br>
> I will rename the function that validates the correct order before this<br>
> is called (It fails and hence the init() func fails if order_plus_one is<br>
> < 2 or > 4.)<br>
><br>
><br>
>      8) opus_projection_ambisonics_encoder_init:<br>
>     mixing_matrix_size_in_bytes and demixing_matrix_size_in_bytes should<br>
>     be opus_int32.<br>
><br>
> Done. <br>
><br>
><br>
>      - Mark<br>
><br>
><br>
><br>
> _______________________________________________<br>
> opus mailing list<br>
> <a href="mailto:opus@xiph.org" target="_blank">opus@xiph.org</a><br>
> <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br>
><br>
</blockquote></div>