<div dir="ltr">Hi Jean-Marc,<div><br></div><div>Here's my responses:</div><div><br></div><div>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.</div><div><br></div><div>2) See above.</div><div><br></div><div>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.</div><div><br></div><div>4) See 3), short and float versions of multiply_out should function the same.</div><div><br></div><div>Let me know your thoughts.</div><div><br></div><div>Cheers,</div><div>Drew</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 21, 2017 at 7:39 PM 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">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><br>
> <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br>
><br>
</blockquote></div>