[opus] [PATCH] Support for Channel Mapping 253.

[Drew Allen]

Hey Mark,

My apologies...Attached is a revised patch that corrects those issues.

Cheers,
Drew

On Mon, Nov 20, 2017 at 9:17 PM Mark Harris <mark.hsj at gmail.com> wrote:

> Hi Drew,
>
> Thanks for the fixes.  See below for a couple of remaining issues.
>
>
> On Mon, Nov 20, 2017 at 5:57 PM, Drew Allen <bitllama at google.com> wrote:
> > On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <mark.hsj at gmail.com> wrote:
> >>
> >>  5) opus_projection_decoder_init: demixing_matrix_size_in_bytes
> >> doesn't include the MappingMatrix struct or alignment but is used to
> >> determine the offset of the decoder; it should be obtained from
> >> mapping_matrix_get_size() and the field should be opus_int32.
> >>
> > This value is only meant to represent the "data" portion of the
> > mappingmatrix struct. Note that we include the struct size when computing
> > the multistream decoder pointer in get_multistream_decoder().
> > I will make it opus_int32 though.
>
> Where in get_multistream_decoder() does it include the size of struct
> MappingMatrix?  It only includes sizeof(OpusProjectionDecoder) and
> st->demixing_matrix_size_in_bytes, neither of which includes the size
> of struct MappingMatrix.  And if you're not going to use
> mapping_matrix_get_size() then you'll also need to include the
> alignment padding after the matrix data separately from alignment
> padding on each of the two structs.
>
> >
> >>
> >>  7) opus_projection_ambisonics_encoder_init: It appears that
> >> uninitialized values are used from the matrices if order_plus_one is 1
> >> or 5 to 15.
> >
> > I will rename the function that validates the correct order before this
> is
> > called (It fails and hence the init() func fails if order_plus_one is <
> 2 or
> >> 4.)
>
> Where does it check for order_plus_one < 2 or > 4?  The draft allows
> order 0 and also higher orders.  The current check in
> src/opus_projection_encoder.c line 65 checks for "order_plus_one < 1
> || order_plus_one > 15".  I don't see a more restrictive check, and if
> there is one somewhere it doesn't prevent
> opus_projection_ambisonics_encoder_init() from accessing uninitialized
> memory.  This can be seen by running test_opus_projection under
> valgrind:
>
> ==22926== Use of uninitialised value of size 8
> ==22926==    at 0x1000E61D0: opus_projection_ambisonics_encoder_init
> (opus_projection_encoder.c:197)
> ==22926==    by 0x1000E642C: opus_projection_ambisonics_encoder_create
> (opus_projection_encoder.c:247)
> ==22926==    by 0x1000014C1: test_creation_arguments
> (test_opus_projection.c:235)
> ==22926==    by 0x100001AEE: main (test_opus_projection.c:411)
> ==22926==  Uninitialised value was created by a heap allocation
> ==22926==    at 0x10009B616: malloc (vg_replace_malloc.c:302)
> ==22926==    by 0x1000E6407: opus_projection_ambisonics_encoder_create
> (os_support.h:49)
> ==22926==    by 0x1000014C1: test_creation_arguments
> (test_opus_projection.c:235)
> ==22926==    by 0x100001AEE: main (test_opus_projection.c:411)
> ==22926==
> ==22926== Conditional jump or move depends on uninitialised value(s)
> ==22926==    at 0x1000E61E5: opus_projection_ambisonics_encoder_init
> (opus_projection_encoder.c:204)
> ==22926==    by 0x1000E642C: opus_projection_ambisonics_encoder_create
> (opus_projection_encoder.c:247)
> ==22926==    by 0x1000014C1: test_creation_arguments
> (test_opus_projection.c:235)
> ==22926==    by 0x100001AEE: main (test_opus_projection.c:411)
> ==22926==  Uninitialised value was created by a heap allocation
> ==22926==    at 0x10009B616: malloc (vg_replace_malloc.c:302)
> ==22926==    by 0x1000E6407: opus_projection_ambisonics_encoder_create
> (os_support.h:49)
> ==22926==    by 0x1000014C1: test_creation_arguments
> (test_opus_projection.c:235)
> ==22926==    by 0x100001AEE: main (test_opus_projection.c:411)
>
>  - Mark
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/36e281b3/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Resolve-align-issues-in-Projection-API.patch
Type: application/octet-stream
Size: 10732 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/36e281b3/attachment-0001.obj>