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