Fix for left shift of negative value in NetEq.

Fix for left shift of negative value in NetEq.

BUG=chromium:666612
Committed: https://crrev.com/7b2516620cebe50cf0cf255219794e46b82f8dbd
Cr-Commit-Position: refs/heads/master@{#15596}

[ivoc, 2016-12-13 10:21:56]

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

[ivoc, 2016-12-13 10:21:56]

Hi Henrik, PTAL.

[hlundin-webrtc, 2016-12-14 08:24:05]

LGTM with questions and nit.

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
File webrtc/common_audio/signal_processing/levinson_durbin.c (right):

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
webrtc/common_audio/signal_processing/levinson_durbin.c:49: R_low[i] =
(int16_t)((temp1W32 - ((int32_t)R_hi[i] * 65536)) >> 1);
I don't understand this code...

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
webrtc/common_audio/signal_processing/levinson_durbin.c:54: temp2W32 = (R[1] *
(1 << norm)); // R[1] in Q31
You can drop the outer parentheses.

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
webrtc/common_audio/signal_processing/levinson_durbin.c:54: temp2W32 = (R[1] *
(1 << norm)); // R[1] in Q31
Since the code at line 49 is quite obscure, did you verify that you get the same
value for temp2W32 before and after your change?

[ivoc, 2016-12-14 09:10:44]

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
File webrtc/common_audio/signal_processing/levinson_durbin.c (left):

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
webrtc/common_audio/signal_processing/levinson_durbin.c:55: +
WEBRTC_SPL_LSHIFT_W32((int32_t)R_low[1],1); // R[1] in Q31
This code is putting the upper and lower bits together again into one number,
but only for R[1]. It seems a bit pointless to me. Maybe the author thought
shifting and adding is cheaper than multiplying?

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
File webrtc/common_audio/signal_processing/levinson_durbin.c (right):

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
webrtc/common_audio/signal_processing/levinson_durbin.c:49: R_low[i] =
(int16_t)((temp1W32 - ((int32_t)R_hi[i] * 65536)) >> 1);
On 2016/12/14 08:24:05, hlundin-webrtc wrote:
> I don't understand this code...

It's pretty hard to read, but I think it's just splitting temp1W32 into the
upper 16 and lower 16 bits.

https://codereview.webrtc.org/2569193002/diff/1/webrtc/common_audio/signal_pr...
webrtc/common_audio/signal_processing/levinson_durbin.c:54: temp2W32 = (R[1] *
(1 << norm)); // R[1] in Q31
On 2016/12/14 08:24:05, hlundin-webrtc wrote:
> Since the code at line 49 is quite obscure, did you verify that you get the
same
> value for temp2W32 before and after your change?

I verified that it gives exactly the same values. It also still passes the
bitexactness tests.

[ivoc, 2016-12-14 09:36:18]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-12-14 09:36:18]

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/2569193002/#ps20001 (title:
"Removed parenthesis")

[commit-bot: I haz the power, 2016-12-14 09:36:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-14 09:59:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481708178408800,
"parent_rev": "e2c16c30719bad69f5d01cc26cbfa5815d58f42e", "commit_rev":
"89a517032a06df6be85be1fd1ef51453b8391555"}

[commit-bot: I haz the power, 2016-12-14 10:00:01]

Description was changed from

==========
Fix for left shift of negative value in NetEq.

BUG=chromium:666612
==========

to

==========
Fix for left shift of negative value in NetEq.

BUG=chromium:666612

Review-Url: https://codereview.webrtc.org/2569193002
==========

[commit-bot: I haz the power, 2016-12-14 10:00:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-14 10:00:08]

Description was changed from

==========
Fix for left shift of negative value in NetEq.

BUG=chromium:666612

Review-Url: https://codereview.webrtc.org/2569193002
==========

to

==========
Fix for left shift of negative value in NetEq.

BUG=chromium:666612

Committed: https://crrev.com/7b2516620cebe50cf0cf255219794e46b82f8dbd
Cr-Commit-Position: refs/heads/master@{#15596}
==========

[commit-bot: I haz the power, 2016-12-14 10:00:09]

Patchset 2 (id:??) landed as
https://crrev.com/7b2516620cebe50cf0cf255219794e46b82f8dbd
Cr-Commit-Position: refs/heads/master@{#15596}