[opus] Initial implementation of ch.mapping 253/3

Wed Jun 28 18:16:57 UTC 2017

Just a friendly ping about this patch. Please let me know what I can do to
assist in the review process. Cheers!

On Mon, Jun 12, 2017 at 9:34 AM Drew Allen <bitllama at google.com> wrote:

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