[opus] [PATCH] Support for Channel Mapping 253.
Jean-Marc Valin
jmvalin at jmvalin.ca
Fri Nov 10 04:47:51 UTC 2017
On 11/09/2017 01:58 PM, Drew Allen wrote:
> Attached is a quick patch that addresses a bug when exporting the matrix
> from the encoder.
Actually, I don't see what your encoder change is supposed to do. Are
there cases where demixing_matrix->rows != nb_output_streams ?
Cheers,
Jean-Marc
> Cheers,
> Drew
>
> On Wed, Nov 8, 2017 at 4:44 PM Drew Allen <bitllama at google.com
> <mailto:bitllama at google.com>> wrote:
>
> Sure, ill send that asap
> On Wed, Nov 8, 2017 at 4:44 PM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
>
> Hi Drew,
>
> Your ambisonics patch is already merged. Can you send a patch that
> applies to master?
>
> Jean-Marc
>
> On 11/08/2017 07:05 PM, Drew Allen wrote:
> > Hey Jean-Marc,
> >
> > I found a bug regarding exporting the matrix that wasn't
> always grabbing
> > the correct values, causing incorrect mixing behavior. This patch
> > resolves that issue.
> >
> > Cheers,
> > Drew
> >
> > On Tue, Nov 7, 2017 at 3:04 PM Drew Allen <bitllama at google.com
> <mailto:bitllama at google.com>
> > <mailto:bitllama at google.com <mailto:bitllama at google.com>>> wrote:
> >
> > Awesome!!!! Ill think about a strategy for that soon. Glad
> it could
> > finally make it in.
> >
> > On Tue, Nov 7, 2017 at 3:01 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,
> >
> > 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>
> <mailto:bitllama at google.com <mailto:bitllama at google.com>>
> > > <mailto:bitllama at google.com
> <mailto:bitllama at google.com> <mailto: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>>
> > <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
> <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,
> > > >
> > > > 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>>>
> > > <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 <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>
> > <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 <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>>>>
> > > > <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 <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> <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 <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>>>>
> > > > <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 <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>
> <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 <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>>>
> > > <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 <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
> > > > > >
> > > > >
> > > >
> > >
> > >
> >
>
More information about the opus
mailing list