Fixed the overflow in the AGC

Fixed the overflow in the AGC

BUG=webrtc:8236
Review-Url: https://codereview.webrtc.org/3009373002
Cr-Commit-Position: refs/heads/master@{#19818}
Committed: https://chromium.googlesource.com/external/webrtc/+/fb2fa3f54e97c4465a6fd6fd992903a52397d739

[peah-webrtc, 2017-09-12 13:39:40]

Patchset #1 (id:1) has been deleted

[peah-webrtc, 2017-09-12 13:45:10]

Patchset #1 (id:20001) has been deleted

[peah-webrtc, 2017-09-12 13:48:09]

peah@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[peah-webrtc, 2017-09-12 13:48:10]

Could you please take a look at this CL?

[peah-webrtc, 2017-09-12 13:48:21]

The CQ bit was checked by peah@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-12 13:48:34]

Dry run: 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, 2017-09-12 14:43:02]

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

[commit-bot: I haz the power, 2017-09-12 14:43:03]

Dry run: This issue passed the CQ dry run.

[the sun, 2017-09-12 17:06:59]

lgtm %...

1. Are there any performance implications on e.g. low end devices? 
2. Should we cap "gain32" instead, to avoid the long mult?
3. How often is the gain computed?

[peah-webrtc, 2017-09-13 07:19:49]

peah@webrtc.org changed reviewers:
+ aleloi@webrtc.org, alessiob@webrtc.org

[peah-webrtc, 2017-09-13 07:19:50]

alessiob@ and aleloi@, could you take a look at this as well for whether you
think the complexity increase is acceptable.

[the sun, 2017-09-13 07:30:58]

On 2017/09/13 07:19:50, peah-webrtc wrote:
> alessiob@ and aleloi@, could you take a look at this as well for whether you
> think the complexity increase is acceptable.

Looking at the code again, we should cap the gain before iterating over samples.

[AleBzk, 2017-09-13 07:38:23]

On 2017/09/13 07:19:50, peah-webrtc wrote:
> alessiob@ and aleloi@, could you take a look at this as well for whether you
> think the complexity increase is acceptable.

Thanks for this patch Per.

That code is executed every 1ms - i.e., every sub-frame.
The complexity increase is almost irrelevant when the code runs on 64 bit CPUs,
while in the 32-bit CPUs case, the following multiplication takes more time:
int64_t tmp64 = ((int64_t)(out[i][k * L + n])) * (gain32 >> 4);

An alternative fix would be to have less precision (by shifting right) and
computing the multiplication still using 32 bit integers.
If we consider this an option, we can investigate further to find a suitable
lower Q factor and check if the lower precision affects the stability of the
adaptive digital AGC.

[peah-webrtc, 2017-09-13 07:54:24]

On 2017/09/13 07:30:58, the sun wrote:
> On 2017/09/13 07:19:50, peah-webrtc wrote:
> > alessiob@ and aleloi@, could you take a look at this as well for whether you
> > think the complexity increase is acceptable.
> 
> Looking at the code again, we should cap the gain before iterating over
samples.


I don't think we can do it without larger changes for the iteration over samples
(note that the inner loop is bands, which is 1, 2 or 3), as gain32 is updated
gradually using delta.

However, the loop over bands can be somewhat simplified (also compared to the
former code). So I'll change that.

[peah-webrtc, 2017-09-13 07:55:18]

On 2017/09/13 07:54:24, peah-webrtc wrote:
> On 2017/09/13 07:30:58, the sun wrote:
> > On 2017/09/13 07:19:50, peah-webrtc wrote:
> > > alessiob@ and aleloi@, could you take a look at this as well for whether
you
> > > think the complexity increase is acceptable.
> > 
> > Looking at the code again, we should cap the gain before iterating over
> samples.
> 
> 
> I don't think we can do it without larger changes for the iteration over
samples
> (note that the inner loop is bands, which is 1, 2 or 3), as gain32 is updated
> gradually using delta.
> 
> However, the loop over bands can be somewhat simplified (also compared to the
> former code). So I'll change that.

Thanks! That makes sense. I also don't think we should try to increase the
accuracy by changing the Q values at this stage.

[the sun, 2017-09-13 11:01:03]

On 2017/09/13 07:55:18, peah-webrtc wrote:
> On 2017/09/13 07:54:24, peah-webrtc wrote:
> > On 2017/09/13 07:30:58, the sun wrote:
> > > On 2017/09/13 07:19:50, peah-webrtc wrote:
> > > > alessiob@ and aleloi@, could you take a look at this as well for whether
> you
> > > > think the complexity increase is acceptable.
> > > 
> > > Looking at the code again, we should cap the gain before iterating over
> > samples.
> > 
> > 
> > I don't think we can do it without larger changes for the iteration over
> samples
> > (note that the inner loop is bands, which is 1, 2 or 3), as gain32 is
updated
> > gradually using delta.

Oh, yeah, I read the closing } wrong - I thought gain32 was updated outside of
the samples loop.

> > 
> > However, the loop over bands can be somewhat simplified (also compared to
the
> > former code). So I'll change that.
> 
> Thanks! That makes sense. I also don't think we should try to increase the
> accuracy by changing the Q values at this stage.

[aleloi, 2017-09-13 12:17:40]

LGTM! alessiob@ explained what this is about, I can't spot any issues. I think
it's not a problem complexity-wise, because the digital AGC is one of the
cheaper components.

[peah-webrtc, 2017-09-13 12:43:16]

The CQ bit was checked by peah@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-13 12:43:28]

Dry run: 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, 2017-09-13 12:52:34]

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

[commit-bot: I haz the power, 2017-09-13 12:52:35]

Dry run: Try jobs failed on following builders:
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/29631)

[peah-webrtc, 2017-09-13 13:25:07]

On 2017/09/13 12:52:35, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_rel on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/29631)

I'll delete the latest patch as that fails in the unit tests due to the accuracy
of that being lower. Even though it makes sense to have it as it is, and the
reason the unittest fail is that they are too tight, I think the fact that the
complexity cost of that is acceptable, and this code it is scheduled for
replacement, I think it makes sense to go with the more exact, but complex, code
construct.

[peah-webrtc, 2017-09-13 13:25:19]

Patchset #2 (id:60001) has been deleted

[peah-webrtc, 2017-09-13 13:25:26]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2017-09-13 13:25:35]

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, 2017-09-13 13:28:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1505309126833190,
"parent_rev": "2ab9879af0ef661b9afde3f1a56ef7073c4006cf", "commit_rev":
"fb2fa3f54e97c4465a6fd6fd992903a52397d739"}

[commit-bot: I haz the power, 2017-09-13 13:28:22]

Description was changed from

==========
Fixed the overflow in the AGC

BUG=webrtc:8236
==========

to

==========
Fixed the overflow in the AGC

BUG=webrtc:8236

Review-Url: https://codereview.webrtc.org/3009373002
Cr-Commit-Position: refs/heads/master@{#19818}
Committed:
https://chromium.googlesource.com/external/webrtc/+/fb2fa3f54e97c4465a6fd6fd9...
==========

[commit-bot: I haz the power, 2017-09-13 13:28:23]

Committed patchset #1 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/fb2fa3f54e97c4465a6fd6fd9...