Hi Drew,
Thanks for addressing these issues. A few remaining things:
1) In this line:
buf[i] = (opus_int16)demixing_matrix[2*i] +
((opus_int16)demixing_matrix[2*i+1] << 8);
you might want to test it with undefined-behaviour sanitizer, but I
believe the second term will be undefined for negative values (the value
would be positive after the cast and only be negative because of the
shift).
I think you'll want something like:
(opus_int16)((unsigned)demixing_matrix[2*i+1] << 8)
(though you might want to check it too)
2) In mapping_matrix_multiply_short(), I would recommend something along
these lines (untested) to improve accuracy:
opus_int32 tmp = 0;
for (col = 0; col < input_rows; col++)
{
tmp + (matrix_data[MATRIX_INDEX(matrix->rows, row, col)] *
input[MATRIX_INDEX(input_rows, col, i)]) >> 8;
}
output[MATRIX_INDEX(output_rows, row, i)] = (tmp+64)>>7;
3) Looking at your get_*() function to compute offsets, I think you're
missing some align() calls in the terms. I think you could end up with
misaligned pointers, which is usually find on x86, but not on other
architectures (e.g. ARM).
Cheers,
Jean-Marc
On 10/10/17 07:09 PM, Drew Allen wrote:> 1. Fixed the va_end() thing. Thanks for the clarification.
> 2. I've redone the mapping matrix so it only either handles floats or
> int16 explicitly. please let me know if you'd like another
> functionality, but my thoughts on this were to keep the val16 handled
> entirely inside multistream API.
> 3. endianness fixed. thank you!
> 4. Removed those pointers, accidentally left them in but they were
> functionally useless.
>
> Attached a new patch with those changes.
>
> On Tue, Oct 10, 2017 at 12:19 PM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
>
> Hi Drew,
>
> On 10/10/17 02:29 PM, Drew Allen wrote:
> > 2) Someone on this list might know better than I do on that one,
> but for
> > the new _ctl_va_list() calls, I believe the convention is for
> va_start()
> > and va_end() to appear in the caller rather than in in the
va_list()
> > function itself.
> >
> > *My understanding is that it's impossible to pass ellipsis to
another
> > function.*
>
> Basically, what I mean is that I *think* you should remove the calls to
> va_end() from _ctl_va_list() calls and instead have the regular _ctl()
> functions call va_end(). That appears to be how functions like
vprintf()
> are called.
>
> > 5) Do you need to expose the "generic"
opus_projection_encode() and
> > opus_projection_encode_float() in the API?
> >
> > *I'm confused... How else would people be able to encode a
packet of
> > audio using the projection encoder? *
>
> Sorry, I got confused when reviewing. Forget about that point!
>
> > 6) In mapping_matrix_multiply_float(), for the !FIXED_POINT case
> you're
> > multiplying by (1.f / 32768.f), but I think in the FIXED_POINT
> case, you
> > should instead >>15
> >
> > *My mistake. Is there a good resource I can read about fixed point
> float
> > operations?*
>
> I'm not aware of any unfortunately.
>
> > 8) For both matrix multiply functions, I'm a bit concerned
with the
> > fixed-point accuracy. Maybe there's a way to keep the
intermediate
> > results in 32 bits and rounding at the end of the sum. For
example,
> > shifting only by 8 in the loop and doing a final shift by 7
> outside the
> > loop.
> >
> > *Done. I've shifted >>4 to matrix_data and >>4 to
input inside the
> loop
> > and >>= 7 to the output outside the loop. Let me know your
thoughts.*
>
> I'm not sure I understand what you mean here (and I can't see
where that
> change is in the code).
>
> > 9) For endianness conversion, you shouldn't have to even
detect it. I
> > think something like this (untested) should work for both
> little-endian
> > and big endian:
> > for (i=0;i<...;i++)
> > short_ptr[i] = char_ptr[2*i] + (char_ptr[2*i] << 8);
> >
> > *Can you show how to go from short -> char as well? I'm
alittle
> confused
> > on how this can be done without knowing the endianness first.*
>
> Well, the endianness of a machine is only visible when you attempt to
> cast multi-byte values to char pointers (input/output them). Unless you
> do that, you have no way to know how your "int" value is
represented. In
> this case, we've decided that the byte ordering
> "on the wire" is little endian, so all you need is write out
the least
> significant byte,then the most significant byte. If you cast to char,
> then you need the endianness, but if you do (my_short>>8), you
always
> get the most significant byte. Similarly, (my_short&0xff) is always
the
> least significant byte. No need for an endianness check.
>
> > 10) Please don't store pointers (MappingMatrix, OpusMSEncoder,
...) in
> > the OpusPEncoder/OpusPDecoder structs. All the data should be in a
> > single struct with no pointers (except to constant data) so that
they
> > can be shallow-copied.
> >
> > *Done.*
>
> Well, I still see pointers in your OpusProjectionDecoder.
>
> Cheers,
>
> Jean-Marc
>
> > On Wed, Oct 4, 2017 at 10:30 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,
> >
> > Here's some comments on your patch (in no particular
order):
> >
> > 1) I see that it's adding an #include of stdarg.h to
> opus_multistream.h
> > Is that left over from the previous version?
> >
> > 2) Someone on this list might know better than I do on that
> one, but for
> > the new _ctl_va_list() calls, I believe the convention is for
> va_start()
> > and va_end() to appear in the caller rather than in in the
> va_list()
> > function itself.
> >
> > 3) I'm not sure I understand why the
opus_copy_channel_out*()
> functions
> > had to be moved.
> >
> > 4) OpusPEncoder: I'd prefer a more explicit name:
> OpusProjectionEncoder
> >
> > 5) Do you need to expose the "generic"
> opus_projection_encode() and
> > opus_projection_encode_float() in the API?
> >
> > 6) In mapping_matrix_multiply_float(), for the !FIXED_POINT
> case you're
> > multiplying by (1.f / 32768.f), but I think in the FIXED_POINT
> case, you
> > should instead >>15
> >
> > 7) In mapping_matrix_multiply_short(), I would prefer
>>15
> instead of
> > /32768 even though most compilers are smart enough by now.
> >
> > 8) For both matrix multiply functions, I'm a bit concerned
> with the
> > fixed-point accuracy. Maybe there's a way to keep the
intermediate
> > results in 32 bits and rounding at the end of the sum. For
> example,
> > shifting only by 8 in the loop and doing a final shift by 7
> outside the
> > loop.
> >
> > 9) For endianness conversion, you shouldn't have to even
> detect it. I
> > think something like this (untested) should work for both
> little-endian
> > and big endian:
> > for (i=0;i<...;i++)
> > short_ptr[i] = char_ptr[2*i] + (char_ptr[2*i] << 8);
> >
> > 10) Please don't store pointers (MappingMatrix,
OpusMSEncoder,
> ...) in
> > the OpusPEncoder/OpusPDecoder structs. All the data should be
in a
> > single struct with no pointers (except to constant data) so
> that they
> > can be shallow-copied.
> >
> > Cheers,
> >
> > Jean-Marc
> >
> > On 27/09/17 03:19 PM, Drew Allen wrote:
> > > Hello all,
> > >
> > > Here is an updated patch for channel mapping 253 support
> using the
> > > proposed "Projection" API. Matrix operations
support and
> tests are
> > > included. Looking forward to your feedback.
> > >
> > > Cheers,
> > > Drew
> > >
> > > On Tue, Sep 19, 2017 at 9:04 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:
> > >
> > > Hello all,
> > >
> > > Attached is an up-to-date patch for supporting
channel
> mapping
> > 253.
> > > Please advise and Thank you for your time.
> > >
> > > Cheers,
> > > Drew
> > >
> > >
> > >
> > >
> > > _______________________________________________
> > > 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
> > >
> >
>
Timothy B. Terriberry
2017-Oct-11 21:12 UTC
[opus] [PATCH] Support for Channel Mapping 253.
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.
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> 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 > http://lists.xiph.org/mailman/listinfo/opus >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171012/3185d218/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Support-for-Channel-Mapping-253.patch Type: text/x-patch Size: 87427 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171012/3185d218/attachment-0001.bin>