Hi Jean-Marc, Here's my responses: 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. 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. 4) See 3), short and float versions of multiply_out should function the same. Let me know your thoughts. Cheers, Drew On Tue, Nov 21, 2017 at 7:39 PM Jean-Marc Valin <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 > > http://lists.xiph.org/mailman/listinfo/opus > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171122/700f8064/attachment.html>
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>> 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> > > http://lists.xiph.org/mailman/listinfo/opus > > >
Hey Jean-Marc, Thanks again! 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). I made the other changes you suggested. Cheers, Drew On Thu, Nov 23, 2017 at 9:02 AM Jean-Marc Valin <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>> 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> > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/141db788/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Fix-memory-issue-in-Projection-API.patch Type: application/octet-stream Size: 36813 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/141db788/attachment-0001.obj>