[opus] [PATCH] Support for Channel Mapping 253.
Drew Allen
bitllama at google.com
Thu Nov 9 18:58:38 UTC 2017
Hi all,
Attached is a quick patch that addresses a bug when exporting the matrix
from the encoder.
Cheers,
Drew
On Wed, Nov 8, 2017 at 4:44 PM Drew Allen <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> 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>> 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>> 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>>>
>> 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>>>> 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>>>>>
>> 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>>>>>>
>> 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>>>>>
>> > > > > >
>> > 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>>>>
>> > > > > >
>> http://lists.xiph.org/mailman/listinfo/opus
>> > > > > >
>> > > > >
>> > > >
>> > >
>> > >
>> >
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171109/7bb62065/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-matrix-export-via-CTL-func.patch
Type: text/x-patch
Size: 2308 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171109/7bb62065/attachment-0001.bin>
More information about the opus
mailing list