I've solely addressed this concern here. Cheers, Drew On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin <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>> 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>>> 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>>>> 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>>>>> 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>>>> > > > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > > > > > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171204/ba4e4553/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: 41519 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171204/ba4e4553/attachment-0001.bin>
FYI Mark Harris' concerns have all be addressed in this patch as well (they were needed in order to get all the tests to pass). On Mon, Dec 4, 2017 at 3:34 PM Drew Allen <bitllama at google.com> wrote:> I've solely addressed this concern here. > > Cheers, > Drew > > > On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin <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>> 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>>> 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>>>> 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>>>>> 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>>>> >> > > > > > http://lists.xiph.org/mailman/listinfo/opus >> > > > > > >> > > > > >> > > > >> > > >> > >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171205/fbfb7083/attachment-0001.html>
Made a few minor tweaks to your patch (attached). Can you confirm you're OK with those and I haven't missed anything? Cheers, Jean-Marc On 12/04/2017 06:34 PM, Drew Allen wrote:> I've solely addressed this concern here. > > 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 -------------- A non-text attachment was scrubbed... Name: 0001-Fix-memory-issues-in-Projection-API.patch Type: text/x-patch Size: 41537 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171207/533f4ab4/attachment-0001.bin>
Those changes look good to me! Thanks :) On Thu, Dec 7, 2017 at 10:06 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Made a few minor tweaks to your patch (attached). Can you confirm you're > OK with those and I haven't missed anything? > > Cheers, > > Jean-Marc > > On 12/04/2017 06:34 PM, Drew Allen wrote: > > I've solely addressed this concern here. > > > > 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/20171207/6b6ebacb/attachment-0001.html>