Avoiding overflow in cross correlation in NetEq.

Avoiding overflow in cross correlation in NetEq.

BUG=

Committed: https://crrev.com/3d09dfdbba5319c735b73b471ff69976292a7583
Cr-Commit-Position: refs/heads/master@{#12538}

[minyue-webrtc, 2016-04-20 17:55:25]

Patchset #1 (id:1) has been deleted

[minyue-webrtc, 2016-04-20 17:57:31]

Description was changed from

==========
aaaa


revise


fixing assert


a fix

BUG=
==========

to

==========
Avoiding overflow in cross correlation in NetEq.

BUG=
==========

[minyue-webrtc, 2016-04-20 17:57:31]

minyue@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[minyue-webrtc, 2016-04-20 17:58:59]

Hi Henrik,

Please take a look before I fix the bit exactness test.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
File webrtc/common_audio/signal_processing/cross_correlation.c (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/cross_correlation.c:31: assert((add >= 0
&& corr >= old) || (add < 0 && corr < old));
test code, will remove

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:57: assert((double)max_1 * max_2 *
sequence_1_length / (1 << scaling) <=
test code, will remove

[hlundin-webrtc, 2016-04-21 08:52:01]

Nice. Please, see my comments.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
File webrtc/common_audio/signal_processing/cross_correlation.c (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/cross_correlation.c:31: assert((add >= 0
&& corr >= old) || (add < 0 && corr < old));
On 2016/04/20 17:58:59, minyue-webrtc wrote:
> test code, will remove

You mean that all changes in this file will be reverted?

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:31: // This function decides the
overflow-protecting scaling and call
call -> calls

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:32: // WebRtcSpl_CrossCorrelation.s
Remove the 's' at the end of the line.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:39: int cross_correlation_step) {
You are always calling this function with cross_correlation_step == -1. You can
simplify the code below if you remove the cross_correlation_step parameter and
simply assume -1.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:53: const int32_t factor =
WEBRTC_SPL_MUL(max_1, max_2) /
I think you can omit WEBRTC_SPL_MUL and simply do max_1 * max_2.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:54: (WEBRTC_SPL_WORD32_MAX /
(int32_t)sequence_1_length);
Use c++-style cast.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:54: (WEBRTC_SPL_WORD32_MAX /
(int32_t)sequence_1_length);
Use std::numeric_limits<int32_t>::max() instead of WEBRTC_SPL_WORD32_MAX.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:57: assert((double)max_1 * max_2 *
sequence_1_length / (1 << scaling) <=
On 2016/04/20 17:58:59, minyue-webrtc wrote:
> test code, will remove

Acknowledged.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:66: if (right_shifts)
You are not calling this function without a valid pointer to |right_shifts|. I
suggest you simply DCHECK that |right_shifts| is not nullptr, skip the internal
variable |scaling|, and use |*right_shifts| instead of |scaling|.

[minyue-webrtc, 2016-04-21 11:06:50]

Hi Henrik,

Thanks for the comments! I have addressed them. One thing to discuss with you is
whether we'd make this a method for other calls to WebRtcSpl_CrossCorrelation,
which may also be vulnerable.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
File webrtc/common_audio/signal_processing/cross_correlation.c (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/cross_correlation.c:31: assert((add >= 0
&& corr >= old) || (add < 0 && corr < old));
On 2016/04/21 08:52:00, hlundin-webrtc wrote:
> On 2016/04/20 17:58:59, minyue-webrtc wrote:
> > test code, will remove
> 
> You mean that all changes in this file will be reverted?

yes. but of course that means there will no sanity check in this method.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:32: // WebRtcSpl_CrossCorrelation.s
On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> Remove the 's' at the end of the line.

oh, sorry, typo

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:39: int cross_correlation_step) {
On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> You are always calling this function with cross_correlation_step == -1. You
can
> simplify the code below if you remove the cross_correlation_step parameter and
> simply assume -1.

I am about to discuss with you whether we'd put this method to
signal_processing_library, since other calls to WebRtcSpl_CrossCorrelation are
prone to overflow as well.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:53: const int32_t factor =
WEBRTC_SPL_MUL(max_1, max_2) /
On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> I think you can omit WEBRTC_SPL_MUL and simply do max_1 * max_2.

Yes, I should.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:54: (WEBRTC_SPL_WORD32_MAX /
(int32_t)sequence_1_length);
On 2016/04/21 08:52:00, hlundin-webrtc wrote:
> Use c++-style cast.

Done.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:54: (WEBRTC_SPL_WORD32_MAX /
(int32_t)sequence_1_length);
On 2016/04/21 08:52:00, hlundin-webrtc wrote:
> Use c++-style cast.

Done.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:66: if (right_shifts)
On 2016/04/21 08:52:00, hlundin-webrtc wrote:
> You are not calling this function without a valid pointer to |right_shifts|. I
> suggest you simply DCHECK that |right_shifts| is not nullptr, skip the
internal
> variable |scaling|, and use |*right_shifts| instead of |scaling|.

ok, I was thinking of reducing some uses of the returned scale. This is true and
see my new patch. If you think the additional change is not needed, I can come
back.

[hlundin-webrtc, 2016-04-22 06:48:57]

A few more comments.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
File webrtc/common_audio/signal_processing/cross_correlation.c (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/common_audio/signa...
webrtc/common_audio/signal_processing/cross_correlation.c:31: assert((add >= 0
&& corr >= old) || (add < 0 && corr < old));
On 2016/04/21 11:06:49, minyue-webrtc wrote:
> On 2016/04/21 08:52:00, hlundin-webrtc wrote:
> > On 2016/04/20 17:58:59, minyue-webrtc wrote:
> > > test code, will remove
> > 
> > You mean that all changes in this file will be reverted?
> 
> yes. but of course that means there will no sanity check in this method.

Acknowledged.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:31: // This function decides the
overflow-protecting scaling and call
On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> call -> calls

Still, call -> calls

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:39: int cross_correlation_step) {
On 2016/04/21 11:06:49, minyue-webrtc wrote:
> On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> > You are always calling this function with cross_correlation_step == -1. You
> can
> > simplify the code below if you remove the cross_correlation_step parameter
and
> > simply assume -1.
> 
> I am about to discuss with you whether we'd put this method to
> signal_processing_library, since other calls to WebRtcSpl_CrossCorrelation are
> prone to overflow as well.

OK. Keep it like this.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:66: if (right_shifts)
On 2016/04/21 11:06:50, minyue-webrtc wrote:
> On 2016/04/21 08:52:00, hlundin-webrtc wrote:
> > You are not calling this function without a valid pointer to |right_shifts|.
I
> > suggest you simply DCHECK that |right_shifts| is not nullptr, skip the
> internal
> > variable |scaling|, and use |*right_shifts| instead of |scaling|.
> 
> ok, I was thinking of reducing some uses of the returned scale. This is true
and
> see my new patch. If you think the additional change is not needed, I can come
> back.

I see. But then I suggest you let the scaling factor be a return value of the
method instead. Callers that do not care can simply not use the return value.
WDYT?

https://codereview.webrtc.org/1908623002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:62: const int32_t factor = max_1 *
max_2 / (std::numeric_limits<int32_t>::max() /
Even though it is true that A*B/C will be evaluated as (A*B)/C, I think you can
help the reader by explicitly adding the parentheses around max_1*max_2.

[minyue-webrtc, 2016-04-22 07:39:49]

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:31: // This function decides the
overflow-protecting scaling and call
On 2016/04/22 06:48:56, hlundin-webrtc wrote:
> On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> > call -> calls
> 
> Still, call -> calls

Yes, I realized it when I uploaded the patch.

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:39: int cross_correlation_step) {
On 2016/04/22 06:48:56, hlundin-webrtc wrote:
> On 2016/04/21 11:06:49, minyue-webrtc wrote:
> > On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> > > You are always calling this function with cross_correlation_step == -1.
You
> > can
> > > simplify the code below if you remove the cross_correlation_step parameter
> and
> > > simply assume -1.
> > 
> > I am about to discuss with you whether we'd put this method to
> > signal_processing_library, since other calls to WebRtcSpl_CrossCorrelation
are
> > prone to overflow as well.
> 
> OK. Keep it like this.

Do you think it is good that we try to replace WebRtcSpl_CrossCorrelation with
CrossCorrelation now (at least for all uses in neteq), to avoid changing
bitexactness test twice?

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:66: if (right_shifts)
On 2016/04/22 06:48:56, hlundin-webrtc wrote:
> On 2016/04/21 11:06:50, minyue-webrtc wrote:
> > On 2016/04/21 08:52:00, hlundin-webrtc wrote:
> > > You are not calling this function without a valid pointer to
|right_shifts|.
> I
> > > suggest you simply DCHECK that |right_shifts| is not nullptr, skip the
> > internal
> > > variable |scaling|, and use |*right_shifts| instead of |scaling|.
> > 
> > ok, I was thinking of reducing some uses of the returned scale. This is true
> and
> > see my new patch. If you think the additional change is not needed, I can
come
> > back.
> 
> I see. But then I suggest you let the scaling factor be a return value of the
> method instead. Callers that do not care can simply not use the return value.
> WDYT?

Good idea.

[hlundin-webrtc, 2016-04-22 07:48:20]

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:39: int cross_correlation_step) {
On 2016/04/22 07:39:49, minyue-webrtc wrote:
> On 2016/04/22 06:48:56, hlundin-webrtc wrote:
> > On 2016/04/21 11:06:49, minyue-webrtc wrote:
> > > On 2016/04/21 08:52:01, hlundin-webrtc wrote:
> > > > You are always calling this function with cross_correlation_step == -1.
> You
> > > can
> > > > simplify the code below if you remove the cross_correlation_step
parameter
> > and
> > > > simply assume -1.
> > > 
> > > I am about to discuss with you whether we'd put this method to
> > > signal_processing_library, since other calls to WebRtcSpl_CrossCorrelation
> are
> > > prone to overflow as well.
> > 
> > OK. Keep it like this.
> 
> Do you think it is good that we try to replace WebRtcSpl_CrossCorrelation with
> CrossCorrelation now (at least for all uses in neteq), to avoid changing
> bitexactness test twice?

Replace all in NetEq now. Leave iLBC as it is.

[minyue-webrtc, 2016-04-22 13:15:30]

Patchset #3 (id:60001) has been deleted

[minyue-webrtc, 2016-04-22 13:58:30]

Hi Henrik,

I made the new method a general tool and used it in neteq to place old cross
correlation calculation everywhere.

Please take a look. 

From try jobs, it is still only bit-exactness tests being affected.

https://codereview.webrtc.org/1908623002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1908623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:62: const int32_t factor = max_1 *
max_2 / (std::numeric_limits<int32_t>::max() /
On 2016/04/22 06:48:56, hlundin-webrtc wrote:
> Even though it is true that A*B/C will be evaluated as (A*B)/C, I think you
can
> help the reader by explicitly adding the parentheses around max_1*max_2.

Done.

[hlundin-webrtc, 2016-04-25 07:53:58]

Very nice! Just a few small comments.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/cross_correlation.cc (right):

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:13: #include <assert.h>
Use DCHECK instead of assert, and include base/webrtc/checks.h instead of
assert.h.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:14: #include <limits>
Order of includes is wrong.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:54: assert((double)max_1
* max_2 * sequence_1_length / (1 << scaling) <=
RTC_DCHECK_LE

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:56: assert(scaling == 0
||
RTC_DCHECK

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/cross_correlation.h (right):

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.h:14: #include
"webrtc/common_audio/signal_processing/include/signal_processing_library.h"
You don't need to include SPL here, but you do need something that defines
int32_t.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.h:39: //      Number of
right bit shifts in cross_correlation.
Delete "bit".

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.h:41: int
CrossCorrelationWithAutoShift(int32_t* cross_correlation,
Output parameter cross_correlation should be after all input parameters.

[hlundin-webrtc, 2016-04-25 07:55:53]

On 2016/04/25 07:53:58, hlundin-webrtc wrote:
> Very nice! Just a few small comments.
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/neteq/cross_correlation.cc (right):
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/cross_correlation.cc:13: #include <assert.h>
> Use DCHECK instead of assert, and include base/webrtc/checks.h instead of
> assert.h.
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/cross_correlation.cc:14: #include <limits>
> Order of includes is wrong.
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/cross_correlation.cc:54:
assert((double)max_1
> * max_2 * sequence_1_length / (1 << scaling) <=
> RTC_DCHECK_LE
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/cross_correlation.cc:56: assert(scaling == 0
> ||
> RTC_DCHECK
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/neteq/cross_correlation.h (right):
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/cross_correlation.h:14: #include
> "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
> You don't need to include SPL here, but you do need something that defines
> int32_t.
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/cross_correlation.h:39: //      Number of
> right bit shifts in cross_correlation.
> Delete "bit".
> 
>
https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/cross_correlation.h:41: int
> CrossCorrelationWithAutoShift(int32_t* cross_correlation,
> Output parameter cross_correlation should be after all input parameters.

Also, will CL make it possible to re-enable any of the tests that were disabled
in https://codereview.webrtc.org/1915813002/? If so, please do so in this CL or
in a follow-up.

[minyue-webrtc, 2016-04-27 09:57:19]

Patchset #4 (id:100001) has been deleted

[minyue-webrtc, 2016-04-27 10:33:33]

Description was changed from

==========
Avoiding overflow in cross correlation in NetEq.

BUG=
==========

to

==========
Avoiding overflow in cross correlation in NetEq.

BUG=webrtc:5820
==========

[minyue-webrtc, 2016-04-27 10:34:56]

Patchset #5 (id:140001) has been deleted

[minyue-webrtc, 2016-04-27 10:38:04]

Hi Henrik,

Patch set 4 addressed your comment and fixed bit exactness tests. 

Patch set 5 removed test code.

As you see from try job on Patch set 4, TestBitExactness can pass UBSAN but
TestOpusBitExactness cannot. We only open the former for now.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/cross_correlation.cc (right):

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:13: #include <assert.h>
On 2016/04/25 07:53:57, hlundin-webrtc wrote:
> Use DCHECK instead of assert, and include base/webrtc/checks.h instead of
> assert.h.

will remove

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:14: #include <limits>
On 2016/04/25 07:53:57, hlundin-webrtc wrote:
> Order of includes is wrong.

Done.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:54: assert((double)max_1
* max_2 * sequence_1_length / (1 << scaling) <=
On 2016/04/25 07:53:57, hlundin-webrtc wrote:
> RTC_DCHECK_LE

I plan to remove these two lines. I've done an exhaustive test on this two
lines.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.cc:56: assert(scaling == 0
||
On 2016/04/25 07:53:57, hlundin-webrtc wrote:
> RTC_DCHECK

also plan to remove

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/cross_correlation.h (right):

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.h:14: #include
"webrtc/common_audio/signal_processing/include/signal_processing_library.h"
On 2016/04/25 07:53:57, hlundin-webrtc wrote:
> You don't need to include SPL here, but you do need something that defines
> int32_t.

Done.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.h:39: //      Number of
right bit shifts in cross_correlation.
On 2016/04/25 07:53:57, hlundin-webrtc wrote:
> Delete "bit".

Done.

https://codereview.webrtc.org/1908623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/cross_correlation.h:41: int
CrossCorrelationWithAutoShift(int32_t* cross_correlation,
On 2016/04/25 07:53:57, hlundin-webrtc wrote:
> Output parameter cross_correlation should be after all input parameters.

Done.

[hlundin-webrtc, 2016-04-27 14:14:32]

lgtm

[minyue-webrtc, 2016-04-27 14:27:22]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-04-27 14:27:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1908623002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1908623002/160001

[commit-bot: I haz the power, 2016-04-27 14:41:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 14:41:04]

Try jobs failed on following builders:
  linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[minyue-webrtc, 2016-04-27 15:27:16]

It turns out that I cannot turn ubsan_vptr off. It seems to have to do with the
protobuf.

[minyue-webrtc, 2016-04-27 15:28:13]

On 2016/04/27 15:27:16, minyue-webrtc wrote:

I mean I cannot turn ubsan_vptr on.

> It turns out that I cannot turn ubsan_vptr off. It seems to have to do with
the
> protobuf.

[minyue-webrtc, 2016-04-27 15:39:26]

Description was changed from

==========
Avoiding overflow in cross correlation in NetEq.

BUG=webrtc:5820
==========

to

==========
Avoiding overflow in cross correlation in NetEq.

BUG=
==========

[minyue-webrtc, 2016-04-27 19:35:27]

On 2016/04/27 15:28:13, minyue-webrtc wrote:
> On 2016/04/27 15:27:16, minyue-webrtc wrote:
> 
> I mean I cannot turn ubsan_vptr on.
> 
> > It turns out that I cannot turn ubsan_vptr off. It seems to have to do with
> the
> > protobuf.

Hi Henrik,

I've removed the BUG field. This cannot fix it, unfortunately. If you still
think this CL is valid, can I go ahead submit it?

[hlundin-webrtc, 2016-04-27 21:01:14]

On 2016/04/27 19:35:27, minyue-webrtc wrote:
> On 2016/04/27 15:28:13, minyue-webrtc wrote:
> > On 2016/04/27 15:27:16, minyue-webrtc wrote:
> > 
> > I mean I cannot turn ubsan_vptr on.
> > 
> > > It turns out that I cannot turn ubsan_vptr off. It seems to have to do
with
> > the
> > > protobuf.
> 
> Hi Henrik,
> 
> I've removed the BUG field. This cannot fix it, unfortunately. If you still
> think this CL is valid, can I go ahead submit it?

Go ahead and land it.

[minyue-webrtc, 2016-04-27 21:07:26]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-04-27 21:07:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1908623002/#ps180001 (title:
"turn off ubsan as it was")

[commit-bot: I haz the power, 2016-04-27 21:07:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1908623002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1908623002/180001

[commit-bot: I haz the power, 2016-04-27 22:06:14]

Description was changed from

==========
Avoiding overflow in cross correlation in NetEq.

BUG=
==========

to

==========
Avoiding overflow in cross correlation in NetEq.

BUG=
==========

[commit-bot: I haz the power, 2016-04-27 22:06:15]

Committed patchset #6 (id:180001)

[minyue-webrtc, 2016-04-28 09:16:33]

A revert of this CL (patchset #6 id:180001) has been created in
https://codereview.webrtc.org/1925053002/ by minyue@webrtc.org.

The reason for reverting is: There seems an error made in this patch.

Hi Henrik,

I think the bit shift returned by CrossCorrelationWithAutoShift may be wrongly
used by DotProduct.

We'd better revert this CL.

Doing another fix (and future fixes) will be paintful. I will work on a
easy-to-modify bitexactness test first..

[commit-bot: I haz the power, 2016-05-01 22:01:01]

Description was changed from

==========
Avoiding overflow in cross correlation in NetEq.

BUG=
==========

to

==========
Avoiding overflow in cross correlation in NetEq.

BUG=

Committed: https://crrev.com/3d09dfdbba5319c735b73b471ff69976292a7583
Cr-Commit-Position: refs/heads/master@{#12538}
==========

[commit-bot: I haz the power, 2016-05-01 22:01:03]

Patchset 6 (id:??) landed as
https://crrev.com/3d09dfdbba5319c735b73b471ff69976292a7583
Cr-Commit-Position: refs/heads/master@{#12538}