[opus] [PATCH] Support for Channel Mapping 253.
Jean-Marc Valin
jmvalin at jmvalin.ca
Mon Nov 6 18:04:54 UTC 2017
Hi Drew,
I've been thinking about your "make_iso_compilers_happy" typedef, which
I assume is meant to avoid "empty" C files. Instead of adding those,
have you tried moving the #ifdef ENABLE_EXPERIMENTAL_AMBISONICS after
the #include directives? Is seems like it would achieve the same thing,
right? Can you check and see if that works. If not, we can just go with
the "make_iso_compilers_happy".
Also, you left two #define OPUS_*_C macros at the beginnign of C files.
Cheers,
Jean-Marc
On 11/03/2017 02:51 PM, Drew Allen wrote:
> Here's another one.
>
> On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
>
> Hi Drew,
>
> We're getting there... Some minor comments:
>
> 1) The public header file should not have an
> #ifdef ENABLE_EXPERIMENTAL_AMBISONICS
> since that would require the user code to define it.
>
> Done
>
> 2) Why do you have #define MAPPING_MATRIX_C ?
>
> No idea. Fixed.
>
>
> 3) Looks like MAPPING_MATRIX_MAX_SIZE is not longer useful, right?
>
> Yup. Removed.
>
>
> 4) Even though it's not strictly necessary here, please add parentheses
> to the definition of MATRIX_INDEX() to avoid nasty surprises in the
> future (e.g. MATRIX_INDEX(...)*sizeof(foo) would be really bad).
>
> Done.
>
>
> Cheers,
>
> Jean-Marc
>
>
> On 10/31/2017 04:10 PM, Drew Allen wrote:
> > Hi Jean-Marc,
> >
> > Thanks so much for your review. Attached are my comments and an
> updated
> > patch.
> >
> > On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc Valin
> <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>
> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> wrote:
> >
> > Hi Drew,
> >
> > I've had some time to dig more deeply into your patch. Here's
> some more
> > in-depth comments:
> >
> > 1) I note that your OpusMSEncoder struct in private.h adds a
> > subframe_mem[] that's not in opus_multistream_encoder.c. I
> assume it's
> > due to a merge problem (that field was removed some time ago),
> but can
> > you confirm/fix the issue?
> >
> > Done. Yes, this is a merge conflict.
> >
> >
> > 2) I noticed your patch adds many OPUS_EXPORT declarations.
> OPUS_EXPORT
> > is only meant for functions exposed to the API (and in the
> include/
> > directory), but I see several of these in mapping_matrix.h,
> which I
> > think is incorrect.
> >
> > Done.
> >
> >
> > 3) opus_projection_ambisonics_encoder_init() (which should be
> > OPUS_EXPORT) is missing from opus_projection.h
> >
> > Done.
> >
> >
> > 4) I believe mapping_matrix_get_size() should be returning
> > align(sizeof(MappingMatrix)) + rows * cols * sizeof(opus_int16)
> > rather than:
> > align(sizeof(MappingMatrix) + rows * cols * sizeof(opus_int16))
> > to match what mapping_matrix_get_data() does
> >
> > Done.
> >
> >
> > 5) I'm not sure I understand the purpose of
> mapping_matrix_validate().
> > Can the user really cause the matrix not to be valid? If yes,
> then can't
> > this cause problems even earlier in the code? If not, then
> maybe an
> > assertion would be better. Also, is the size of the matrix the
> only
> > thing you can validate?
> >
> > Removed it. You're right, it's functionally impossible for the user to
> > create an invalid matrix with the current API.
> >
> >
> > 6) mapping_matrix_init() returns an error code, but the code
> that's
> > calling it never checks it. Considering that it's an internal
> call, I
> > would say it's probably better not to return anything, but
> instead to
> > assert in the function.
> >
> > Done.
> >
> >
> > 7) Same for mapping_matrix_multiply_short() and
> > mapping_matrix_multiply_float(), the return value isn't
> checked. So it
> > should either be replaced by an asssert or (if the user can
> cause a
> > failure) actually checked.
> >
> > Done.
> >
> > 8) I see there's a "gain" field in the matrix, but I can't see
> it used
> > anywhere. Did I miss something?
> >
> > Gain should be pulled out by the user via
> > a OPUS_PROJECTION_GET_DEMIXING_MATRIX_GAIN call and add it to the
> > overall output gain. We assume the mixing matrix gain is always
> zero and
> > that this only matters for the output gain from the demixing matrix.
> >
> >
> > 9) In get_streams_from_channels(), I think it'd be simpler to just
> > replace:
> > *streams = channels / 2 + (channels % 2 == 1);
> > with:
> > *streams = (channels+1) / 2;
> >
> > Done.
> >
> >
> > 10) In opus_projection_ambisonics_encoder_init(), you to see
> if streams
> > and coupled_streams are NULL, but unless I missed something I
> don't
> > think there's any valid case where you wouldn't want to get those
> > values. If you don't have them, then you have no way of
> knowing what
> > mapping the encoder used, so no way of decoding. Instead, I
> would just
> > return OPUS_BAD_ARG if they're NULL.
> >
> > Done.
> >
> >
> > 11) So one issue I just noticed is that
> opus_projection_encode() and
> > opus_projection_encode_float() (same for the decoder) use
> arbitrarily
> > large amounts of stack memory for "buf". In
> opus_multistream_encode()
> > that's avoided by converting just two channels at a time, but
> here it's
> > not quite clear how to do that without duplicating a lot of the
> > multistream code. If we can't address the issue with this
> patch, the
> > least would be to abort when trying to use these calls with
> > NONTHREADSAFE_PSEUDOSTACK
> >
> > Done. Let me know if we should try designing something around this.
> >
> >
> > 12) I think opus_projection_decoder_ctl() is missing a
> va_end() call.
> >
> >
> > Done.
> >
> >
> >
> > Cheers,
> >
> > Jean-Marc
> >
> >
> >
> > On 10/12/2017 05:44 PM, Drew Allen wrote:
> > > thanks for all your feedback. here's the revised patch:
> > >
> > > On Wed, Oct 11, 2017 at 2:20 PM Timothy B. Terriberry
> > <tterribe at xiph.org <mailto:tterribe at xiph.org>
> <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>
> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>
> <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>> wrote:
> > >
> > > Jean-Marc Valin wrote:
> > > > I think you'll want something like:
> > > > (opus_int16)((unsigned)demixing_matrix[2*i+1] << 8)
> > > > (though you might want to check it too)
> > >
> > > FWIW, we use the construct
> > > int s = buf[2*i + 1] << 8 | buf[2*i];
> > > s = ((s & 0xFFFF) ^ 0x8000) - 0x8000;
> > >
> > > to manually sign-extend the result in opus_compare (and also
> > opus_demo).
> > > If you ultimately cast s to (opus_int16), then I'm
> pretty sure
> > most
> > > compilers will turn the second line into a no-op and
> optimize
> > it away.
> > > _______________________________________________
> > > opus mailing list
> > > opus at xiph.org <mailto:opus at xiph.org>
> <mailto:opus at xiph.org <mailto:opus at xiph.org>> <mailto:opus at xiph.org
> <mailto:opus at xiph.org>
> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>
> > > http://lists.xiph.org/mailman/listinfo/opus
> > >
> > >
> > >
> > > _______________________________________________
> > > opus mailing list
> > > opus at xiph.org <mailto:opus at xiph.org> <mailto:opus at xiph.org
> <mailto:opus at xiph.org>>
> > > http://lists.xiph.org/mailman/listinfo/opus
> > >
> >
>
More information about the opus
mailing list