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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20170530/a0a9ef46/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Support-for-Ch.253-Using-OpusPDec-OpusPEnc-structs.patch Type: text/x-patch Size: 59771 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20170530/a0a9ef46/attachment-0001.bin>
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, > DrewHi 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
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/20170607/7ed3e826/attachment.html>