<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">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">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/<wbr>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/<wbr>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:<wbr>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:<wbr>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:<wbr>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:<wbr>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:<wbr>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_<wbr>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_<wbr>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_<wbr>size<br>
(opus_multistream_decoder.c:<wbr>47)<br>
    by 0x10006B098: opus_projection_decoder_get_<wbr>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_<wbr>impl<br>
(opus_projection_encoder.c:<wbr>125)<br>
    by 0x10006A9CA: opus_projection_ambisonics_<wbr>encoder_init<br>
(opus_projection_encoder.c:<wbr>196)<br>
    by 0x10006AC7B: opus_projection_ambisonics_<wbr>encoder_create<br>
(opus_projection_encoder.c:<wbr>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_<wbr>encoder_create<br>
(opus_projection_encoder.c:<wbr>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:<wbr>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:<wbr>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>