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> 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>> 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>>> 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>> > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/e8c66c05/attachment.html>
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>> 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>>> 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>>>> 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>>> > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > >
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> 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>> 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>>> 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>>>> 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>>> > > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171124/719a870e/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Fix-memory-issues-in-Projection-API.patch Type: application/octet-stream Size: 39145 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171124/719a870e/attachment-0001.obj>