<div dir="ltr">Hi Mark, <div><br></div><div>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.</div><div><br></div><div>Cheers,</div><div>Drew</div><div><br><div class="gmail_quote"><div dir="ltr">On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <<a href="mailto:mark.hsj@gmail.com">mark.hsj@gmail.com</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>
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></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 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></blockquote><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 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></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 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></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 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></blockquote><div>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().</div><div>I will make it opus_int32 though.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 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></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 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></blockquote><div>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.)</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
 8) opus_projection_ambisonics_encoder_init:<br>
mixing_matrix_size_in_bytes and demixing_matrix_size_in_bytes should<br>
be opus_int32.</blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
 - Mark<br>
</blockquote></div></div></div>