I only had a quick look, but your patch looks good except for the: output[output_rows * i] = (1/(32768.f*128.f))*tmp; For floating point, you shouldn't do the >>7 either. Just remove the >>8 from the floating-point calculation of tmp so that all the scaling is done in float. Cheers, Jean-Marc On 11/27/2017 04:01 PM, Drew Allen wrote:> Hi Jean-Marc, > > Attached is an updated patch with your suggestions + additional > corrections I caught over the weekend. > > Cheers, > Drew > > On Fri, Nov 24, 2017 at 10:08 AM Drew Allen <bitllama at google.com > <mailto:bitllama at google.com>> wrote: > > Aha good point! Im travelling this weekend but will submit another > patch Monday morning. > > Cheers, > Drew > On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > Hi Drew, > > I noticed you reverted the > output[output_rows * i] = (tmp + 16384) >> 15; > from the previous patch. That's still good. What should have been > changed is the float version: > output[output_rows * i] = (1/32768.f) * ((tmp + 16384) >> 15); > which should just be: > output[output_rows * i] = (1/(32768.f*32768.f)) * tmp; > since there's no point in doing integer rounding when you have float > available. > > Cheers, > > Jean-Marc > > On 11/23/2017 10:35 PM, Drew Allen wrote: > > Hi Jean-Marc, > > > > Attached is an updated patch. I had to include some of Mark's > > suggestions in order to get the tests to work correctly. I > will still > > submit a separate patch for him for a few other concerns he > had after > > this one clears. > > > > Cheers, > > Drew > > > > On Thu, Nov 23, 2017 at 10:42 AM Jean-Marc Valin > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> wrote: > > > > Actually, there's also something wrong with the in_short() > function. For > > floating point (#else case), you shouldn't need shifting > since you're > > already doing the scaling through a float multiply. > > > > Jean-Marc > > > > On 11/23/2017 01:39 PM, Drew Allen wrote: > > > got it. actually that patch i sent you has something > wrong with the > > > mapping_matrix_multiply_short_out... let me fix that and > will send you > > > another patch soon. > > > > > > Cheers, > > > Drew > > > > > > On Thu, Nov 23, 2017 at 10:34 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: > > > > > > On 11/23/2017 01:28 PM, Drew Allen wrote: > > > > To your first point, I was only trying to copy how > > _multistream_'s c > > > > files function in this way, possibly that's worth > > refactoring as well > > > > (as a separate patch). > > > > > > Well, the opus_multistream_decode_* calls were > correct before your > > > patch. It's only the encode ones that have the > issues (should be > > > addressed separately). > > > > > > Jean-Marc > > > > > > > On Thu, Nov 23, 2017 at 9:02 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: > > > > > > > > On 11/22/2017 06:23 PM, Drew Allen wrote: > > > > > 1) I didn't change how > multistream_decode_float works > > in the > > > argument > > > > > list... I noticed it changes it's arguments > depending > > on whether > > > > > FIXED_POINT is used. I copied this style for the > > projection > > > API as > > > > well. > > > > > If this isn't desired, we should make those > changes > > separately. > > > > > > > > > > 2) See above. > > > > > > > > Just noticed the code will actually work because > > opus_val16 is > > > defined > > > > to the right thing, but I still think it's a > bad idea. For > > > example, if > > > > you look at the public header, > opus_projection_decode() > > has a pcm > > > > argument of type opus_int16*, so it's a bit > confusing > > for the > > > C file to > > > > define pcm as opus_val16*, even if the two map > to the same. > > > > > > > > > 3) I only zero out initially. For the > matrix_multiply_out > > > > functions, we > > > > > need to be able to take a single decoded > stream and > > add weighted > > > > > versions of it to each of the final output > channels. This > > > zero-ing out > > > > > on stream 0 ensures the block is zero-set so > we can > > > incrementally add > > > > > all the decoded streams to the output channels. > > > > > > > > OK, I see. In that case, you should replace > the two for() > > > loops with > > > > just a single call to OPUS_COPY(). That should > be both > > faster and > > > > simpler. > > > > > > > > > 4) See 3), short and float versions of > multiply_out should > > > > function the > > > > > same. > > > > > > > > Again, I now see what that code is doing. > Unfortunately, > > it means > > > > increased noise for fixed-point since we have > to round > > > multiple times. I > > > > don't have a proposed fix for that, so I guess > we'll have to > > > deal with > > > > it. However, instead of rounding tmp twice > (>>8 followed by > > > +64>>7), you > > > > should only round once. That means (tmp+16384)>>15 > > > > > > > > Cheers, > > > > > > > > Jean-Marc > > > > > > > > > Let me know your thoughts. > > > > > > > > > > Cheers, > > > > > Drew > > > > > > > > > > On Tue, Nov 21, 2017 at 7:39 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, > > > > > > > > > > Had a look at your patch. I think it's > on the > > right track -- > > > > there's no > > > > > fundamental issue with it as far as I > can tell. > > There's > > > a few > > > > > implementation issues though. > > > > > > > > > > 1) In opus_multi_stream_decode_float(), you > > changed the pcm > > > > argument > > > > > from float* to opus_val16*. That's a > mistake and will > > > fail for > > > > > fixed-point. This should have shown up > as an error > > on a > > > > fixed-point > > > > > build. Make sure you test that. > > > > > > > > > > 2) In opus_projection_decode(), you also > changed > > the pcm > > > > argument to an > > > > > opus_val16*, which again will cause > problems for > > the same > > > > reasons as 1). > > > > > Please check you haven't made that same > mistake > > elsewhere. > > > > > > > > > > 3) In > opus_projection_copy_channel_out_float() and > > > > > > opus_projection_copy_channel_out_short(), you're > > zeroing the > > > > values with > > > > > this loop: > > > > > for (i=0;i<frame_size;i++) > > > > > { > > > > > for (j=0;j<dst_stride;j++) > > > > > { > > > > > float_dst[i*dst_stride+j] = 0; > > > > > } > > > > > } > > > > > That looks wrong, since you'll be > overwriting all > > channels > > > > instead of > > > > > just the one you're copying. That should > have come > > up in > > > > testing, no? > > > > > > > > > > 4) You might want to have a look at > > > > > > mapping_matrix_multiply_channel_out_short(). I > > can't quite > > > > follow what > > > > > it's doing, but it seems wrong since the > whole > > point of > > > adding > > > > the "tmp" > > > > > is to avoid rounding multiple times. > > > > > > > > > > I haven't had time to thoroughly review > the changes to > > > > mapping_matrix.c, > > > > > but I'll do that on your revised version. > > > > > > > > > > Cheers, > > > > > > > > > > Jean-Marc > > > > > > > > > > > > > > > > > > > > > > > > > On 11/20/2017 05:15 PM, Drew Allen wrote: > > > > > > Hello, > > > > > > > > > > > > Attached is a patch to resolve a > memory issue > > using the > > > > Projection API > > > > > > when compiling using a psuedo-stack / > limited > > memory. > > > > > > > > > > > > Please let me any ?s you might have. > > > > > > > > > > > > Cheers, > > > > > > Drew > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > 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 > > > > > > > > > > > > > > > > > > > > >
Done! On Tue, Nov 28, 2017 at 12:12 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> I only had a quick look, but your patch looks good except for the: > output[output_rows * i] = (1/(32768.f*128.f))*tmp; > > For floating point, you shouldn't do the >>7 either. Just remove the >>8 > from the floating-point calculation of tmp so that all the scaling is > done in float. > > Cheers, > > Jean-Marc > > On 11/27/2017 04:01 PM, Drew Allen wrote: > > Hi Jean-Marc, > > > > Attached is an updated patch with your suggestions + additional > > corrections I caught over the weekend. > > > > Cheers, > > Drew > > > > On Fri, Nov 24, 2017 at 10:08 AM Drew Allen <bitllama at google.com > > <mailto:bitllama at google.com>> wrote: > > > > Aha good point! Im travelling this weekend but will submit another > > patch Monday morning. > > > > Cheers, > > Drew > > On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin <jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca>> wrote: > > > > Hi Drew, > > > > I noticed you reverted the > > output[output_rows * i] = (tmp + 16384) >> 15; > > from the previous patch. That's still good. What should have been > > changed is the float version: > > output[output_rows * i] = (1/32768.f) * ((tmp + 16384) >> 15); > > which should just be: > > output[output_rows * i] = (1/(32768.f*32768.f)) * tmp; > > since there's no point in doing integer rounding when you have > float > > available. > > > > Cheers, > > > > Jean-Marc > > > > On 11/23/2017 10:35 PM, Drew Allen wrote: > > > Hi Jean-Marc, > > > > > > Attached is an updated patch. I had to include some of Mark's > > > suggestions in order to get the tests to work correctly. I > > will still > > > submit a separate patch for him for a few other concerns he > > had after > > > this one clears. > > > > > > Cheers, > > > Drew > > > > > > On Thu, Nov 23, 2017 at 10:42 AM Jean-Marc Valin > > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > wrote: > > > > > > Actually, there's also something wrong with the in_short() > > function. For > > > floating point (#else case), you shouldn't need shifting > > since you're > > > already doing the scaling through a float multiply. > > > > > > Jean-Marc > > > > > > On 11/23/2017 01:39 PM, Drew Allen wrote: > > > > got it. actually that patch i sent you has something > > wrong with the > > > > mapping_matrix_multiply_short_out... let me fix that and > > will send you > > > > another patch soon. > > > > > > > > Cheers, > > > > Drew > > > > > > > > On Thu, Nov 23, 2017 at 10:34 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: > > > > > > > > On 11/23/2017 01:28 PM, Drew Allen wrote: > > > > > To your first point, I was only trying to copy how > > > _multistream_'s c > > > > > files function in this way, possibly that's worth > > > refactoring as well > > > > > (as a separate patch). > > > > > > > > Well, the opus_multistream_decode_* calls were > > correct before your > > > > patch. It's only the encode ones that have the > > issues (should be > > > > addressed separately). > > > > > > > > Jean-Marc > > > > > > > > > On Thu, Nov 23, 2017 at 9:02 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: > > > > > > > > > > On 11/22/2017 06:23 PM, Drew Allen wrote: > > > > > > 1) I didn't change how > > multistream_decode_float works > > > in the > > > > argument > > > > > > list... I noticed it changes it's arguments > > depending > > > on whether > > > > > > FIXED_POINT is used. I copied this style for > the > > > projection > > > > API as > > > > > well. > > > > > > If this isn't desired, we should make those > > changes > > > separately. > > > > > > > > > > > > 2) See above. > > > > > > > > > > Just noticed the code will actually work > because > > > opus_val16 is > > > > defined > > > > > to the right thing, but I still think it's a > > bad idea. For > > > > example, if > > > > > you look at the public header, > > opus_projection_decode() > > > has a pcm > > > > > argument of type opus_int16*, so it's a bit > > confusing > > > for the > > > > C file to > > > > > define pcm as opus_val16*, even if the two map > > to the same. > > > > > > > > > > > 3) I only zero out initially. For the > > matrix_multiply_out > > > > > functions, we > > > > > > need to be able to take a single decoded > > stream and > > > add weighted > > > > > > versions of it to each of the final output > > channels. This > > > > zero-ing out > > > > > > on stream 0 ensures the block is zero-set so > > we can > > > > incrementally add > > > > > > all the decoded streams to the output > channels. > > > > > > > > > > OK, I see. In that case, you should replace > > the two for() > > > > loops with > > > > > just a single call to OPUS_COPY(). That should > > be both > > > faster and > > > > > simpler. > > > > > > > > > > > 4) See 3), short and float versions of > > multiply_out should > > > > > function the > > > > > > same. > > > > > > > > > > Again, I now see what that code is doing. > > Unfortunately, > > > it means > > > > > increased noise for fixed-point since we have > > to round > > > > multiple times. I > > > > > don't have a proposed fix for that, so I guess > > we'll have to > > > > deal with > > > > > it. However, instead of rounding tmp twice > > (>>8 followed by > > > > +64>>7), you > > > > > should only round once. That means > (tmp+16384)>>15 > > > > > > > > > > Cheers, > > > > > > > > > > Jean-Marc > > > > > > > > > > > Let me know your thoughts. > > > > > > > > > > > > Cheers, > > > > > > Drew > > > > > > > > > > > > On Tue, Nov 21, 2017 at 7:39 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, > > > > > > > > > > > > Had a look at your patch. I think it's > > on the > > > right track -- > > > > > there's no > > > > > > fundamental issue with it as far as I > > can tell. > > > There's > > > > a few > > > > > > implementation issues though. > > > > > > > > > > > > 1) In opus_multi_stream_decode_float(), > you > > > changed the pcm > > > > > argument > > > > > > from float* to opus_val16*. That's a > > mistake and will > > > > fail for > > > > > > fixed-point. This should have shown up > > as an error > > > on a > > > > > fixed-point > > > > > > build. Make sure you test that. > > > > > > > > > > > > 2) In opus_projection_decode(), you also > > changed > > > the pcm > > > > > argument to an > > > > > > opus_val16*, which again will cause > > problems for > > > the same > > > > > reasons as 1). > > > > > > Please check you haven't made that same > > mistake > > > elsewhere. > > > > > > > > > > > > 3) In > > opus_projection_copy_channel_out_float() and > > > > > > > > opus_projection_copy_channel_out_short(), you're > > > zeroing the > > > > > values with > > > > > > this loop: > > > > > > for (i=0;i<frame_size;i++) > > > > > > { > > > > > > for (j=0;j<dst_stride;j++) > > > > > > { > > > > > > float_dst[i*dst_stride+j] = 0; > > > > > > } > > > > > > } > > > > > > That looks wrong, since you'll be > > overwriting all > > > channels > > > > > instead of > > > > > > just the one you're copying. That should > > have come > > > up in > > > > > testing, no? > > > > > > > > > > > > 4) You might want to have a look at > > > > > > > > mapping_matrix_multiply_channel_out_short(). I > > > can't quite > > > > > follow what > > > > > > it's doing, but it seems wrong since the > > whole > > > point of > > > > adding > > > > > the "tmp" > > > > > > is to avoid rounding multiple times. > > > > > > > > > > > > I haven't had time to thoroughly review > > the changes to > > > > > mapping_matrix.c, > > > > > > but I'll do that on your revised version. > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Jean-Marc > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 11/20/2017 05:15 PM, Drew Allen wrote: > > > > > > > Hello, > > > > > > > > > > > > > > Attached is a patch to resolve a > > memory issue > > > using the > > > > > Projection API > > > > > > > when compiling using a psuedo-stack / > > limited > > > memory. > > > > > > > > > > > > > > Please let me any ?s you might have. > > > > > > > > > > > > > > Cheers, > > > > > > > Drew > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171128/801e9b50/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Fix-memory-issues-in-Projection-API.patch Type: text/x-patch Size: 41342 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171128/801e9b50/attachment-0001.bin>
I think you just attached the wrong (previous) version of the patch. Jean-Marc On 11/28/2017 12:24 PM, Drew Allen wrote:> Done! > > On Tue, Nov 28, 2017 at 12:12 AM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > I only had a quick look, but your patch looks good except for the: > output[output_rows * i] = (1/(32768.f*128.f))*tmp; > > For floating point, you shouldn't do the >>7 either. Just remove the >>8 > from the floating-point calculation of tmp so that all the scaling is > done in float. > > Cheers, > > Jean-Marc > > On 11/27/2017 04:01 PM, Drew Allen wrote: > > Hi Jean-Marc, > > > > Attached is an updated patch with your suggestions + additional > > corrections I caught over the weekend. > > > > Cheers, > > Drew > > > > On Fri, Nov 24, 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: > > > > Aha good point! Im travelling this weekend but will submit another > > patch Monday morning. > > > > Cheers, > > Drew > > On Fri, Nov 24, 2017 at 9:15 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, > > > > I noticed you reverted the > > output[output_rows * i] = (tmp + 16384) >> 15; > > from the previous patch. That's still good. What should > have been > > changed is the float version: > > output[output_rows * i] = (1/32768.f) * ((tmp + 16384) >> 15); > > which should just be: > > output[output_rows * i] = (1/(32768.f*32768.f)) * tmp; > > since there's no point in doing integer rounding when you > have float > > available. > > > > Cheers, > > > > Jean-Marc > > > > On 11/23/2017 10:35 PM, Drew Allen wrote: > > > Hi Jean-Marc, > > > > > > Attached is an updated patch. I had to include some of > Mark's > > > suggestions in order to get the tests to work correctly. I > > will still > > > submit a separate patch for him for a few other concerns he > > had after > > > this one clears. > > > > > > Cheers, > > > Drew > > > > > > On Thu, Nov 23, 2017 at 10:42 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: > > > > > > Actually, there's also something wrong with the > in_short() > > function. For > > > floating point (#else case), you shouldn't need shifting > > since you're > > > already doing the scaling through a float multiply. > > > > > > Jean-Marc > > > > > > On 11/23/2017 01:39 PM, Drew Allen wrote: > > > > got it. actually that patch i sent you has something > > wrong with the > > > > mapping_matrix_multiply_short_out... let me fix > that and > > will send you > > > > another patch soon. > > > > > > > > Cheers, > > > > Drew > > > > > > > > On Thu, Nov 23, 2017 at 10:34 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: > > > > > > > > On 11/23/2017 01:28 PM, Drew Allen wrote: > > > > > To your first point, I was only trying to > copy how > > > _multistream_'s c > > > > > files function in this way, possibly that's > worth > > > refactoring as well > > > > > (as a separate patch). > > > > > > > > Well, the opus_multistream_decode_* calls were > > correct before your > > > > patch. It's only the encode ones that have the > > issues (should be > > > > addressed separately). > > > > > > > > Jean-Marc > > > > > > > > > On Thu, Nov 23, 2017 at 9:02 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>>>> > > > > > <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: > > > > > > > > > > On 11/22/2017 06:23 PM, Drew Allen wrote: > > > > > > 1) I didn't change how > > multistream_decode_float works > > > in the > > > > argument > > > > > > list... I noticed it changes it's > arguments > > depending > > > on whether > > > > > > FIXED_POINT is used. I copied this > style for the > > > projection > > > > API as > > > > > well. > > > > > > If this isn't desired, we should make > those > > changes > > > separately. > > > > > > > > > > > > 2) See above. > > > > > > > > > > Just noticed the code will actually work > because > > > opus_val16 is > > > > defined > > > > > to the right thing, but I still think it's a > > bad idea. For > > > > example, if > > > > > you look at the public header, > > opus_projection_decode() > > > has a pcm > > > > > argument of type opus_int16*, so it's a bit > > confusing > > > for the > > > > C file to > > > > > define pcm as opus_val16*, even if the > two map > > to the same. > > > > > > > > > > > 3) I only zero out initially. For the > > matrix_multiply_out > > > > > functions, we > > > > > > need to be able to take a single decoded > > stream and > > > add weighted > > > > > > versions of it to each of the final output > > channels. This > > > > zero-ing out > > > > > > on stream 0 ensures the block is > zero-set so > > we can > > > > incrementally add > > > > > > all the decoded streams to the output > channels. > > > > > > > > > > OK, I see. In that case, you should replace > > the two for() > > > > loops with > > > > > just a single call to OPUS_COPY(). That > should > > be both > > > faster and > > > > > simpler. > > > > > > > > > > > 4) See 3), short and float versions of > > multiply_out should > > > > > function the > > > > > > same. > > > > > > > > > > Again, I now see what that code is doing. > > Unfortunately, > > > it means > > > > > increased noise for fixed-point since we > have > > to round > > > > multiple times. I > > > > > don't have a proposed fix for that, so I > guess > > we'll have to > > > > deal with > > > > > it. However, instead of rounding tmp twice > > (>>8 followed by > > > > +64>>7), you > > > > > should only round once. That means > (tmp+16384)>>15 > > > > > > > > > > Cheers, > > > > > > > > > > Jean-Marc > > > > > > > > > > > Let me know your thoughts. > > > > > > > > > > > > Cheers, > > > > > > Drew > > > > > > > > > > > > On Tue, Nov 21, 2017 at 7:39 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>>>>> > > > > > > <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 <mailto:jmvalin at jmvalin.ca>>>>>>> wrote: > > > > > > > > > > > > Hi Drew, > > > > > > > > > > > > Had a look at your patch. I think it's > > on the > > > right track -- > > > > > there's no > > > > > > fundamental issue with it as far as I > > can tell. > > > There's > > > > a few > > > > > > implementation issues though. > > > > > > > > > > > > 1) In > opus_multi_stream_decode_float(), you > > > changed the pcm > > > > > argument > > > > > > from float* to opus_val16*. That's a > > mistake and will > > > > fail for > > > > > > fixed-point. This should have shown up > > as an error > > > on a > > > > > fixed-point > > > > > > build. Make sure you test that. > > > > > > > > > > > > 2) In opus_projection_decode(), > you also > > changed > > > the pcm > > > > > argument to an > > > > > > opus_val16*, which again will cause > > problems for > > > the same > > > > > reasons as 1). > > > > > > Please check you haven't made that > same > > mistake > > > elsewhere. > > > > > > > > > > > > 3) In > > opus_projection_copy_channel_out_float() and > > > > > > > > opus_projection_copy_channel_out_short(), you're > > > zeroing the > > > > > values with > > > > > > this loop: > > > > > > for (i=0;i<frame_size;i++) > > > > > > { > > > > > > for (j=0;j<dst_stride;j++) > > > > > > { > > > > > > float_dst[i*dst_stride+j] = 0; > > > > > > } > > > > > > } > > > > > > That looks wrong, since you'll be > > overwriting all > > > channels > > > > > instead of > > > > > > just the one you're copying. That > should > > have come > > > up in > > > > > testing, no? > > > > > > > > > > > > 4) You might want to have a look at > > > > > > > > mapping_matrix_multiply_channel_out_short(). I > > > can't quite > > > > > follow what > > > > > > it's doing, but it seems wrong > since the > > whole > > > point of > > > > adding > > > > > the "tmp" > > > > > > is to avoid rounding multiple times. > > > > > > > > > > > > I haven't had time to thoroughly > review > > the changes to > > > > > mapping_matrix.c, > > > > > > but I'll do that on your revised > version. > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Jean-Marc > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 11/20/2017 05:15 PM, Drew Allen > wrote: > > > > > > > Hello, > > > > > > > > > > > > > > Attached is a patch to resolve a > > memory issue > > > using the > > > > > Projection API > > > > > > > when compiling using a > psuedo-stack / > > limited > > > memory. > > > > > > > > > > > > > > Please let me any ?s you might have. > > > > > > > > > > > > > > Cheers, > > > > > > > Drew > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > >