<div dir="ltr">Thanks again Mark for taking a look and pointing out the issues that previous patch had. I've addressed your concerns. The tests should no longer give any warnings or errors regarding usage or c89. I've ensured the memory issues have also been addressed.<div><br></div><div>Please let me know any and all other concerns as well. :)</div><div><br></div><div>Attached is the new proposed patch.</div><div><br></div><div>Cheers,</div><div>Drew</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 7, 2017 at 8:02 AM Drew Allen <<a href="mailto:bitllama@google.com">bitllama@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto">Thanks for looking over this, Mark! I'm travelling this week but when I'm back I'll address all of your concerns and send,out another patch. :)</div><div class="gmail_extra"><br><div class="gmail_quote">On Jun 7, 2017 9:56 AM, "Mark Harris" <<a href="mailto:mark.hsj@gmail.com" target="_blank">mark.hsj@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, May 30, 2017 at 3:13 PM, Drew Allen <<a href="mailto:bitllama@google.com" target="_blank">bitllama@google.com</a>> wrote:<br>
> Hello all,<br>
><br>
> Attached is the initial proposed implementation for ch.mapping 253/3, based<br>
> on the IETF proposal:<br>
> <a href="https://tools.ietf.org/html/draft-ietf-codec-ambisonics-03" rel="noreferrer" target="_blank">https://tools.ietf.org/html/draft-ietf-codec-ambisonics-03</a><br>
><br>
> A brief overview of the patch, as it is slightly lengthy:<br>
> After discussion with Jean-Marc, we determined that ch.253/3 will need the<br>
> demixing matrix as part of the encoder/decoder struct stack and thus will<br>
> require a new Encoder/Decoder structure. For this, I've proposed<br>
> OpusPEnc/OpusPDec (for "projection"), which wraps OpusMSEnc/OpusMSDec aside<br>
> from the addition of the mixing matrix structure. The functionality also<br>
> wraps the functionality of the multistream API aside from matrix transforms<br>
> during encode/decode.<br>
><br>
> We have included 3 mixing/demixing matrix pairs that are sufficient for<br>
> coding/decoding 1st/2nd/3rd order content respecitvely.<br>
><br>
> I've modified the multistream api slightly, first to include some<br>
> supplemental API extensions to allow wrapping and moved the declaration of<br>
> the MSEnc/MSDec to opus_private so that the projection API can compile.<br>
><br>
> Some basic tests to ensure matrix multiplication and encoding/decoding using<br>
> projection has been included.<br>
><br>
> I am of course happy and eager to take any and all<br>
> suggestions/comments/questions/recommendations regarding the patch,<br>
> including naming conventions/style/etc.<br>
><br>
> Looking forward to your feedback,<br>
><br>
> Cheers,<br>
> Drew<br>
<br>
<br>
Hi Drew,<br>
<br>
I ran into some issues with this new test.<br>
<br>
Building the test for floating point produces a clang compiler warning:<br>
<br>
  CC       tests/test_opus_projection.o<br>
tests/test_opus_projection.c:137:9: warning: using floating point<br>
absolute value function<br>
      'fabs' when argument is of integer type [-Wabsolute-value]<br>
    if (ABS16(a[i] - b[i]) > tolerance)<br>
        ^<br>
./celt/arch.h:194:26: note: expanded from macro 'ABS16'<br>
#define ABS16(x) ((float)fabs(x))<br>
                         ^<br>
tests/test_opus_projection.c:137:9: note: use function 'abs' instead<br>
./celt/arch.h:194:26: note: expanded from macro 'ABS16'<br>
#define ABS16(x) ((float)fabs(x))<br>
                         ^<br>
<br>
Also the test fails to compile with -std=c89:<br>
<br>
tests/test_opus_projection.c:57:1: error: C++ style comments are not<br>
allowed in ISO C90<br>
 // static const MappingMatrix test_matrix = {<br>
 ^<br>
tests/test_opus_projection.c:57:1: error: (this will be reported only<br>
once per input file)<br>
tests/test_opus_projection.c: In function 'main':<br>
tests/test_opus_projection.c:403:1: warning: control reaches end of<br>
non-void function [-Wreturn-type]<br>
 }<br>
 ^<br>
<br>
Running the test, an issue occurs in test_creation_arguments().<br>
opus_projection_ambisonics_encoder_create() is intentionally called<br>
with invalid arguments (initially, channels=0), which causes an error<br>
to be returned and streams and coupled_streams are left uninitialized.<br>
These uninitialized values are passed to<br>
opus_projection_decoder_create(), which does not check that the<br>
arguments are not too large before using them to attempt allocation of<br>
a ridiculous amount of memory.  Results may vary depending on the<br>
uninitialized values of streams and coupled_streams.  Valgrind reports<br>
several uses of uninitialized values such as:<br>
<br>
 Conditional jump or move depends on uninitialised value(s)<br>
    at 0x100067B72: opus_multistream_decoder_get_size<br>
(opus_multistream_decoder.c:47)<br>
    by 0x10006B098: opus_projection_decoder_get_size<br>
(opus_projection_decoder.c:54)<br>
    by 0x10006B136: opus_projection_decoder_create<br>
(opus_projection_decoder.c:68)<br>
    by 0x10000140D: test_creation_arguments (test_opus_projection.c:273)<br>
    by 0x100001D06: main (test_opus_projection.c:396)<br>
  Uninitialised value was created by a stack allocation<br>
    at 0x100001330: test_creation_arguments (test_opus_projection.c:252)<br>
<br>
Additionally, the demixing_matrix appears to be used uninitialized<br>
when order_plus_one is 1 (or greater than 4):<br>
<br>
 Conditional jump or move depends on uninitialised value(s)<br>
    at 0x10006A6DF: opus_projection_encoder_init_impl<br>
(opus_projection_encoder.c:125)<br>
    by 0x10006A9CA: opus_projection_ambisonics_encoder_init<br>
(opus_projection_encoder.c:196)<br>
    by 0x10006AC7B: opus_projection_ambisonics_encoder_create<br>
(opus_projection_encoder.c:261)<br>
    by 0x1000013D9: test_creation_arguments (test_opus_projection.c:268)<br>
    by 0x100001D06: main (test_opus_projection.c:396)<br>
  Uninitialised value was created by a heap allocation<br>
    at 0x10010FE71: malloc (vg_replace_malloc.c:302)<br>
    by 0x10006AB84: opus_alloc (os_support.h:49)<br>
    by 0x10006AC1F: opus_projection_ambisonics_encoder_create<br>
(opus_projection_encoder.c:252)<br>
    by 0x1000013D9: test_creation_arguments (test_opus_projection.c:268)<br>
    by 0x100001D06: main (test_opus_projection.c:396)<br>
<br>
Another issue occurs in mapping_matrix_dot_short().  It attempts to<br>
read beyond the end of the input buffer and may cause a segmentation<br>
fault.  It appears that the input buffer contains only 2 channels of<br>
480 samples each, but 16 channels of 960 samples each are being read<br>
from the buffer.  Valgrind reports the following:<br>
<br>
 Invalid read of size 2<br>
    at 0x10006B9D8: mapping_matrix_dot_short (mapping_matrix.c:98)<br>
    by 0x10006BABD: mapping_matrix_multiply_short (mapping_matrix.c:119)<br>
    by 0x10006AD5B: opus_projection_encode (opus_projection_encoder.c:283)<br>
    by 0x100001BCB: test_encode_decode (test_opus_projection.c:374)<br>
    by 0x100001D2D: main (test_opus_projection.c:400)<br>
  Address 0x10490b000 is not stack'd, malloc'd or (recently) free'd<br>
<br>
 Process terminating with default action of signal 11 (SIGSEGV)<br>
  Access not within mapped region at address 0x10490B000<br>
    at 0x10006B9D8: mapping_matrix_dot_short (mapping_matrix.c:98)<br>
    by 0x10006BABD: mapping_matrix_multiply_short (mapping_matrix.c:119)<br>
    by 0x10006AD5B: opus_projection_encode (opus_projection_encoder.c:283)<br>
    by 0x100001BCB: test_encode_decode (test_opus_projection.c:374)<br>
    by 0x100001D2D: main (test_opus_projection.c:400)<br>
<br>
 - Mark<br>
</blockquote></div></div>
</blockquote></div></div>