<div dir="ltr">Thanks Jean-Marc,<div><br></div><div>Yes this makes sense: the scaling is preserved for both framelengths, and the maximum value for speech_nrg going into the square-root is now correct.</div><div><br></div><div>It may be more elegant to use</div><div> speech_nrg = silk_LSHIFT( speech_nrg, 16 );<br></div><div>instead of</div><div> speech_nrg = silk_LSHIFT_SAT32( speech_nrg, 16 );<br></div><div>as the saturation will never be used (because input speech_nrg is < 16384).</div><div><br></div><div>best,</div><div>koen.</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 16, 2018 at 3:42 PM, Jean-Marc Valin <span dir="ltr"><<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Chandrakala, Logan,<br>
<br>
Can you confirm that the attached patch fixes the overflow problem?<br>
<br>
Koen, can you confirm the fix makes sense?<br>
<br>
Cheers,<br>
<br>
Jean-Marc<br>
<br>
On 11/27/2017 12:10 PM, Logan Stromberg wrote:<br>
> Sorry, long holiday weekend in America.<br>
> I can say with pretty high certainty that there is an overflow occurring<br>
> and it is flipping smooth_coef_Q16 to be negative when it probably<br>
> shouldn't be. I had originally thought it was only an issue where it was<br>
> overflowing the 15th bit but not the 16th, which might still preserve<br>
> the intended value for operations that ignore the sign bit (in cases<br>
> where the 16-bit value is stored in a 32-bit int without sign-extension<br>
> e.g. 0x00008b3b), but that doesn't seem to be the case here.<br>
> The problem is even worse when opus_int is defined as 16-bit in the<br>
> platform - SA_Q15 overflows to negative right here with a similar effect<br>
> speech_nrg = silk_SQRT_APPROX(speech_nrg);<br>
> SA_Q15 = silk_SMULWB(32768 + speech_nrg, SA_Q15);<br>
><br>
> I can't speak for the logic where the speech energy gets doubled. It<br>
> obviously seems intentional but I don't know why. Maybe so that<br>
> smoothing is performed at a constant rate regardless of whether<br>
> framesize is 10 or 20ms?<br>
><br>
> On Sun, Nov 26, 2017 at 8:07 PM, Chandrakala Madhira<br>
> <<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a><br>
> <mailto:<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a>>> wrote:<br>
><br>
> Hi,<br>
><br>
> Can anyone let me know if this is a bug?<br>
><br>
> Thank you,<br>
> Chandrakala<br>
><br>
> ------------------------------<wbr>------------------------------<wbr>------------<br>
> *From: *"Logan Stromberg" <<a href="mailto:loganstromberg@gmail.com">loganstromberg@gmail.com</a><br>
> <mailto:<a href="mailto:loganstromberg@gmail.com">loganstromberg@gmail.<wbr>com</a>>><br>
> *To: *"Chandrakala Madhira" <<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a><br>
> <mailto:<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a>>><br>
> *Cc: *<a href="mailto:opus@xiph.org">opus@xiph.org</a> <mailto:<a href="mailto:opus@xiph.org">opus@xiph.org</a>><br>
> *Sent: *Wednesday, November 22, 2017 12:12:39 PM<br>
><br>
> *Subject: *Re: [opus] Reg an issue with smoothing factor in VAD<br>
> implementation<br>
><br>
> Yes, yes, I can reproduce it now, but only on platforms that define<br>
> a 16-bit int by default (SA_Q15 is an opus_int rather than<br>
> opus_int32). What system are you compiling this for?<br>
><br>
> On Tue, Nov 21, 2017 at 8:34 PM, Chandrakala Madhira<br>
> <<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a><br>
> <mailto:<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a>>> wrote:<br>
><br>
> Hi Logan,<br>
><br>
> Please find attached the input stream we are using testing.<br>
><br>
> Thank you,<br>
> Chandrakala<br>
><br>
> ------------------------------<wbr>------------------------------<wbr>------------<br>
> *From: *"Chandrakala Madhira"<br>
> <<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a><br>
> <mailto:<a href="mailto:chandrakala.madhira@soctronics.com">chandrakala.madhira@<wbr>soctronics.com</a>>><br>
> *To: *"Logan Stromberg" <<a href="mailto:loganstromberg@gmail.com">loganstromberg@gmail.com</a><br>
> <mailto:<a href="mailto:loganstromberg@gmail.com">loganstromberg@gmail.<wbr>com</a>>><br>
> *Cc: *<a href="mailto:opus@xiph.org">opus@xiph.org</a> <mailto:<a href="mailto:opus@xiph.org">opus@xiph.org</a>><br>
> *Sent: *Wednesday, November 22, 2017 9:53:55 AM<br>
><br>
> *Subject: *Re: [opus] Reg an issue with smoothing factor in VAD<br>
> implementation<br>
><br>
> Hi Logan,<br>
><br>
> Below is the configuration I am using. The remaining parameters<br>
> have defaults. The stream is a 16KHz stream. The bitrate<br>
> selected is 16KHz. As the thread have size limitations, I will<br>
> try to attach the stream in next mail. At 125th frame of this<br>
> stream, the probability (SA_Q15) value is 0x0000859c. This value<br>
> is treated as a negative number in step-3.<br>
><br>
> -e voip 16000 1 16000 -framesize 10 -cvbr Input/dg105_16k.wav<br>
> Output/dg105_16k_16000Fs_mono_<wbr>16000bps_10ms_vbr_voip.bit<br>
><br>
> Thank you,<br>
> Chandrakala<br>
><br>
> Thank you,<br>
> Chandrakala<br>
><br>
> ------------------------------<wbr>------------------------------<wbr>------------<br>
> *From: *"Logan Stromberg" <<a href="mailto:loganstromberg@gmail.com">loganstromberg@gmail.com</a><br>
> <mailto:<a href="mailto:loganstromberg@gmail.com">loganstromberg@gmail.<wbr>com</a>>><br>
> *To: *<a href="mailto:opus@xiph.org">opus@xiph.org</a> <mailto:<a href="mailto:opus@xiph.org">opus@xiph.org</a>><br>
> *Sent: *Tuesday, November 21, 2017 2:38:16 AM<br>
> *Subject: *Re: [opus] Reg an issue with smoothing factor in VAD<br>
> implementation<br>
><br>
> Just for fun, I tried to reproduce such an overflow. I turned on<br>
> all debug macros, assertions, and checked arithmetic and then<br>
> encoded 2 hours of mixed speech/audio with these parameters:<br>
><br>
> Sample rate = 48000<br>
> Channels = 1<br>
> Application = OPUS_APPLICATION_AUDIO<br>
> Bitrate = 24 KB/s<br>
> Force Mode = MODE_SILK_ONLY<br>
> Signal Type = OPUS_SIGNAL_AUTO<br>
> Complexity = 10<br>
> Frame size = 480 samples (10ms)<br>
><br>
> No errors came up in encoding. Chandrakala, are these the<br>
> encoding parameters that you believe should trigger the error?<br>
><br>
> - Logan<br>
><br>
> Hi,<br>
><br>
> We are looking at the VAD implementation used in opus. We<br>
> are looking at the code where speech probability is<br>
> calculated based on which SNR is estimated. Below is the<br>
> part of the code I am talking about.<br>
><br>
> /*****************************<wbr>****/<br>
> /* Speech Probability Estimation */<br>
> /*****************************<wbr>****/<br>
> SA_Q15 = silk_sigm_Q15( silk_SMULWB( VAD_SNR_FACTOR_Q16,<br>
> pSNR_dB_Q7 ) - VAD_NEGATIVE_OFFSET_Q5 ); // step1: Calculate<br>
> speech probability : comment by me<br>
><br>
> /* Power scaling */<br>
> if( speech_nrg <= 0 ) { // step2: update speech probability<br>
> based on speech energy : comment by me<br>
> SA_Q15 = silk_RSHIFT( SA_Q15, 1 );<br>
> } else if( speech_nrg < 32768 ) {<br>
> if( psEncC->frame_length == 10 * psEncC->fs_kHz ) {<br>
> speech_nrg = silk_LSHIFT_SAT32( speech_nrg, 16 ); // Energy<br>
> is doubled here : comment by me<br>
> } else {<br>
> speech_nrg = silk_LSHIFT_SAT32( speech_nrg, 15 );<br>
> }<br>
><br>
> /* square-root */<br>
> speech_nrg = silk_SQRT_APPROX( speech_nrg );<br>
> SA_Q15 = silk_SMULWB( 32768 + speech_nrg, SA_Q15 );<br>
> }<br>
><br>
> /* Smoothing coefficient */<br>
> smooth_coef_Q16 = silk_SMULWB( VAD_SNR_SMOOTH_COEF_Q18,<br>
> silk_SMULWB( (opus_int32)SA_Q15, SA_Q15 ) ); // step3:<br>
> Update the smoothing factor based on speech probability :<br>
> comment by me<br>
><br>
> if( psEncC->frame_length == 10 * psEncC->fs_kHz ) {<br>
> smooth_coef_Q16 >>= 1;<br>
> }<br>
><br>
> Here, in step1, Speech probability is calculated whose value<br>
> is expected to be within [0, 1) in Q15 format. Then based on<br>
> the speech energy levels, in Step2, the probability is<br>
> updated whose value shall also lie between [0, 1). Later in<br>
> Step3, the smooth coeff is calculated. This code do not have<br>
> any issue when the frame size is more than or equal to<br>
> 20msec. But, if the frame size is 10ms, then in step2, the<br>
> energy is doubled (this may be done because the original<br>
> Silk code is for 20ms. To convert the energy for 20ms, it<br>
> could have been doubled). When this is done the probability<br>
> which is updated in step2 becomes more than 1. When this is<br>
> used in multiplication in Step3, the value is treated as a<br>
> negative number because its a 32x16 multiplication. This is<br>
> will result in a negative smooth coefficient. Please let me<br>
> know if this is a bug.<br>
><br>
><br>
> Thank you,<br>
> Chandrakala<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> opus mailing list<br>
> <a href="mailto:opus@xiph.org">opus@xiph.org</a> <mailto:<a href="mailto:opus@xiph.org">opus@xiph.org</a>><br>
> <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/<wbr>listinfo/opus</a><br>
> <<a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/<wbr>mailman/listinfo/opus</a>><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> opus mailing list<br>
> <a href="mailto:opus@xiph.org">opus@xiph.org</a><br>
> <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/<wbr>listinfo/opus</a><br>
><br>
</blockquote></div><br></div>