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 > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Ulrich Windl
2017-Nov-29 08:23 UTC
[opus] Antw: Re: [PATCH] Fix memory issue in Projection API
Following the thread from outside, I think Drew should work on in-house quality assurance ;-)> 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 >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > _______________________________________________ > opus mailing list > opus at xiph.org > http://lists.xiph.org/mailman/listinfo/opus
Jean-Marc Valin
2017-Nov-29 17:04 UTC
[opus] Antw: Re: [PATCH] Fix memory issue in Projection API
On 11/29/2017 03:23 AM, Ulrich Windl wrote:> Following the thread from outside, I think Drew should work on in-house quality assurance ;-)Happens to everyone, myself (definitely) included. Jean-Marc> > >> 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 >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> >> _______________________________________________ >> opus mailing list >> opus at xiph.org >> http://lists.xiph.org/mailman/listinfo/opus > _______________________________________________ > opus mailing list > opus at xiph.org > http://lists.xiph.org/mailman/listinfo/opus >
Jean-Marc Valin
2017-Nov-30 18:38 UTC
[opus] Antw: Re: [PATCH] Fix memory issue in Projection API
Hi Drew, The float code should also be doing float multiplications. Make sure tmp is an opus_val32 and the multiplication itself casts to float rather than int32. Otherwise, the float version is likely to overflow. Jean-Marc On 11/30/2017 12:06 PM, Drew Allen wrote:> My apologies. I forgot to commit the changes before creating that last > patch. Yes, some in-house control would be good, I agree. I'll ensure my > future patches are verified internally before submitting anything new. > > Cheers, > Drew > > On Wed, Nov 29, 2017 at 9:04 AM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > On 11/29/2017 03:23 AM, Ulrich Windl wrote: > > Following the thread from outside, I think Drew should work on > in-house quality assurance ;-) > > Happens to everyone, myself (definitely) included. > > Jean-Marc > > > > > > >> 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> > >>> <mailto: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>> > >>> > <mailto: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>> > >>> > <mailto: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>>> > >>> > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto: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>>>> > >>> > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto: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>>>>> > >>> > > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>>> > >>> > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto: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>>>>>> > >>> > > > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>>> > >>> > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>>>> > >>> > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>>> > >>> > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > >>> <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > >>> > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto: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>>>>>> > >>> > <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 <mailto:opus at xiph.org>>>>>>> > >>> > > > > > > > >>> > http://lists.xiph.org/mailman/listinfo/opus > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >> _______________________________________________ > >> opus mailing list > >> opus at xiph.org <mailto:opus at xiph.org> > >> http://lists.xiph.org/mailman/listinfo/opus > > _______________________________________________ > > opus mailing list > > opus at xiph.org <mailto:opus at xiph.org> > > http://lists.xiph.org/mailman/listinfo/opus > > >