thr3ads.net - opus - [opus] Initial implementation of ch.mapping 253/3 [Jun 2017]

If this information is useful, please help other people find it:
Share via:

Drew Allen

2017-May-30 22:13 UTC

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

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>

Mark Harris

2017-Jun-07 08:56 UTC

head link

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

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

Drew Allen

2017-Jun-07 15:02 UTC

head link

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

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>

Possibly Parallel Threads

Search for more possibly parallel threads

opus - Jun 2017 - Initial implementation of ch.mapping 253/3

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

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

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

Possibly Parallel Threads