<div dir="ltr">Hey Jean-Marc,<div><br></div><div>Thanks again!</div><div><br></div><div>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).</div><div><br></div><div>I made the other changes you suggested.</div><div><br></div><div>Cheers,</div><div>Drew</div><div><br></div><div><br></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Nov 23, 2017 at 9:02 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">On 11/22/2017 06:23 PM, Drew Allen wrote:<br>
> 1) I didn't change how multistream_decode_float works in the argument<br>
> list... I noticed it changes it's arguments depending on whether<br>
> FIXED_POINT is used. I copied this style for the projection API as well.<br>
> If this isn't desired, we should make those changes separately.<br>
><br>
> 2) See above.<br>
<br>
Just noticed the code will actually work because opus_val16 is defined<br>
to the right thing, but I still think it's a bad idea. For example, if<br>
you look at the public header, opus_projection_decode() has a pcm<br>
argument of type opus_int16*, so it's a bit confusing for the C file to<br>
define pcm as opus_val16*, even if the two map to the same.<br>
<br>
> 3) I only zero out initially. For the matrix_multiply_out functions, we<br>
> need to be able to take a single decoded stream and add weighted<br>
> versions of it to each of the final output channels. This zero-ing out<br>
> on stream 0 ensures the block is zero-set so we can incrementally add<br>
> all the decoded streams to the output channels.<br>
<br>
OK, I see. In that case, you should replace the two for() loops with<br>
just a single call to OPUS_COPY(). That should be both faster and simpler.<br>
<br>
> 4) See 3), short and float versions of multiply_out should function the<br>
> same.<br>
<br>
Again, I now see what that code is doing. Unfortunately, it means<br>
increased noise for fixed-point since we have to round multiple times. I<br>
don't have a proposed fix for that, so I guess we'll have to deal with<br>
it. However, instead of rounding tmp twice (>>8 followed by +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 <<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>
> Had a look at your patch. I think it's on the right track -- there's no<br>
> fundamental issue with it as far as I can tell. There's a few<br>
> implementation issues though.<br>
><br>
> 1) In opus_multi_stream_decode_float(), you changed the pcm argument<br>
> from float* to opus_val16*. That's a mistake and will fail for<br>
> fixed-point. This should have shown up as an error on a fixed-point<br>
> build. Make sure you test that.<br>
><br>
> 2) In opus_projection_decode(), you also changed the pcm argument to an<br>
> opus_val16*, which again will cause problems for the same reasons as 1).<br>
> Please check you haven't made that same mistake elsewhere.<br>
><br>
> 3) In opus_projection_copy_channel_out_float() and<br>
> opus_projection_copy_channel_out_short(), you're zeroing the 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 overwriting all channels instead of<br>
> just the one you're copying. That should have come up in testing, no?<br>
><br>
> 4) You might want to have a look at<br>
> mapping_matrix_multiply_channel_out_short(). I can't quite follow what<br>
> it's doing, but it seems wrong since the whole point of adding the "tmp"<br>
> is to avoid rounding multiple times.<br>
><br>
> I haven't had time to thoroughly review the changes to 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 memory issue using the Projection API<br>
> > when compiling using a psuedo-stack / limited memory.<br>
> ><br>
> > Please let me any ?s you might have.<br>
> ><br>
> > Cheers,<br>
> > Drew<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>
> > <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br>
> ><br>
><br>
</blockquote></div></div>