[opus] [PATCH] Support for Channel Mapping 253.
Jean-Marc Valin
jmvalin at jmvalin.ca
Tue Nov 7 23:00:59 UTC 2017
Hi Drew,
Thanks for the update. Your patch is now in master. Now, it would be
good if you could think of a way to reduce the stack usage as we discussed.
Cheers,
Jean-Marc
On 11/07/2017 04:28 PM, Drew Allen wrote:
> Here's another patch. Cheers!
>
> On Mon, Nov 6, 2017 at 10:08 AM Drew Allen <bitllama at google.com
> <mailto:bitllama at google.com>> wrote:
>
> Ok great. I'll make those changes you suggested and get back to you
> this morning.
>
> Cheers,
> Drew
>
>
>
> 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>
> > <mailto: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>>
> > > <mailto: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>>>
> > > > <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 <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>>>
> <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 <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>>
> <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
> > > >
> > >
> >
>
>
More information about the opus
mailing list