Utilizing the AEC3 config struct for constants.

Utilizing the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

BUG=webrtc:5298
Review-Url: https://codereview.webrtc.org/3003733002
Cr-Commit-Position: refs/heads/master@{#19507}
Committed: https://chromium.googlesource.com/external/webrtc/+/8cee56f2546c329d193d9478e29894cf9f6ad2ff

[peah-webrtc, 2017-08-23 13:43:39]

Description was changed from

==========
AEC test

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

to

==========
Utilizating the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
 constants.

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

[peah-webrtc, 2017-08-23 13:44:00]

Description was changed from

==========
Utilizating the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
 constants.

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

to

==========
Utilizating the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

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

[peah-webrtc, 2017-08-23 13:45:15]

Description was changed from

==========
Utilizating the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

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

to

==========
Utilizing the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

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

[peah-webrtc, 2017-08-23 13:45:46]

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

[hlundin-webrtc, 2017-08-23 14:52:22]

LG. I have a few comments.

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_remover.cc:108:
residual_echo_estimator_(config_),
This is probably a bug. I guess you would be using the default values for
config_ rather than the ones passed through config.

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right):

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:113: void
SuppressionGain::UpdateMaxGainIncrease(
I think you should avoid littering the h-file with these private const methods.
Instead, add config_ as input to the (already long) parameter list of the local
functions you had, and keep them in the unnamed namespace.

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:291: struct
Thresholds {
A little too generic. Threshold for what?

[peah-webrtc, 2017-08-24 06:59:42]

Description was changed from

==========
Utilizing the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

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

to

==========
Utilizing the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

BUG=webrtc:5298
==========

[peah-webrtc, 2017-08-24 07:00:52]

Hi,
Thanks for the comments. I have updated the code with changes in response to the
comments. PTAL

Please also note that there are some rebase changes that by mistake were
included. Sorry for that.

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/echo_remover.cc (right):

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/echo_remover.cc:108:
residual_echo_estimator_(config_),
On 2017/08/23 14:52:22, hlundin-webrtc wrote:
> This is probably a bug. I guess you would be using the default values for
> config_ rather than the ones passed through config.

Thanks!!! That definitely was a bug. Good find! I'll change the code so that
config_ is set first. That should minimize the risk for error.

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/suppression_gain.cc (right):

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/suppression_gain.cc:113: void
SuppressionGain::UpdateMaxGainIncrease(
On 2017/08/23 14:52:22, hlundin-webrtc wrote:
> I think you should avoid littering the h-file with these private const
methods.
> Instead, add config_ as input to the (already long) parameter list of the
local
> functions you had, and keep them in the unnamed namespace.

Good suggestion. That makes sense, I'll do that.

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/3003733002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:291: struct
Thresholds {
On 2017/08/23 14:52:22, hlundin-webrtc wrote:
> A little too generic. Threshold for what?

I agree. A changed the naming a bit. PTAL

[hlundin-webrtc, 2017-08-24 07:06:42]

lgtm

[peah-webrtc, 2017-08-24 07:22:35]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2017-08-24 07:22:44]

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-08-24 07:27:16]

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

[commit-bot: I haz the power, 2017-08-24 07:27:17]

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

[peah-webrtc, 2017-08-24 08:55:08]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-08-24 08:55:08]

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/3003733002/#ps60001 (title:
"Fixed unittest build errors occurring in debug mode")

[commit-bot: I haz the power, 2017-08-24 08:55:16]

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-08-24 09:09:03]

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

[commit-bot: I haz the power, 2017-08-24 09:09:05]

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

[peah-webrtc, 2017-08-24 09:43:32]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-08-24 09:43:33]

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/3003733002/#ps80001 (title:
"Corrected failing unittest")

[commit-bot: I haz the power, 2017-08-24 09:43:45]

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-08-24 10:35:33]

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

[commit-bot: I haz the power, 2017-08-24 10:35:34]

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

[peah-webrtc, 2017-08-24 10:47:55]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2017-08-24 10:48:05]

CQ is trying da patch.

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

[peah-webrtc, 2017-08-24 13:03:59]

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

[peah-webrtc, 2017-08-24 13:04:00]

kwiberg@
Could you please take a look at the copy constructor implementation in
audio_processing.h?

[kwiberg-webrtc, 2017-08-24 17:46:39]

The copy assignment lgtm.

https://codereview.webrtc.org/3003733002/diff/340001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/3003733002/diff/340001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/include/audio_processing.h:334: #endif
If we're lucky, you can do

#else
    static_assert(std::is_trivially_copy_assignable<Config>::value, "");
#endif

to let the compiler prove that the type is indeed trivially copy-assignable. (It
needs to be in the #else clause, because the presence of a non-default copy
assignment function technically makes the type non-trivial.)

[peah-webrtc, 2017-08-24 20:17:25]

Patchset #19 (id:360001) has been deleted

[peah-webrtc, 2017-08-24 20:18:29]

Patchset #12 (id:220001) has been deleted

[peah-webrtc, 2017-08-24 20:18:43]

Patchset #12 (id:240001) has been deleted

[peah-webrtc, 2017-08-24 20:18:55]

Patchset #12 (id:260001) has been deleted

[peah-webrtc, 2017-08-24 20:19:04]

Patchset #12 (id:280001) has been deleted

[peah-webrtc, 2017-08-24 20:19:15]

Patchset #12 (id:300001) has been deleted

[peah-webrtc, 2017-08-24 20:19:26]

Patchset #12 (id:320001) has been deleted

[peah-webrtc, 2017-08-24 20:22:32]

On 2017/08/24 17:46:39, kwiberg-webrtc wrote:
> The copy assignment lgtm.
> 
>
https://codereview.webrtc.org/3003733002/diff/340001/webrtc/modules/audio_pro...
> File webrtc/modules/audio_processing/include/audio_processing.h (right):
> 
>
https://codereview.webrtc.org/3003733002/diff/340001/webrtc/modules/audio_pro...
> webrtc/modules/audio_processing/include/audio_processing.h:334: #endif
> If we're lucky, you can do
> 
> #else
>     static_assert(std::is_trivially_copy_assignable<Config>::value, "");
> #endif
> 
> to let the compiler prove that the type is indeed trivially copy-assignable.
(It
> needs to be in the #else clause, because the presence of a non-default copy
> assignment function technically makes the type non-trivial.)

Thanks for the review! I tried the static_assert construct but it won't build
(error: static_assert failed). I guess that means that the type for some reason
is non-trivial?

[peah-webrtc, 2017-08-24 23:00:05]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-08-24 23:00:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3003733002/#ps420001 (title:
"Added missing include")

[commit-bot: I haz the power, 2017-08-24 23:00:16]

CQ is trying da patch.

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

[peah-webrtc, 2017-08-24 23:02:11]

Patchset #14 (id:400001) has been deleted

[commit-bot: I haz the power, 2017-08-24 23:43:26]

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

[commit-bot: I haz the power, 2017-08-24 23:43:27]

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

[peah-webrtc, 2017-08-25 05:06:09]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-08-25 05:06:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3003733002/#ps490001 (title:
"Added comment")

[commit-bot: I haz the power, 2017-08-25 05:06:17]

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-08-25 05:36:53]

CQ is committing da patch.

Bot data: {"patchset_id": 490001, "attempt_start_ts": 1503637569494750,
"parent_rev": "2dbc69fa64a7ea183e090dbfaa68ab16d4bcef7a", "commit_rev":
"8cee56f2546c329d193d9478e29894cf9f6ad2ff"}

[commit-bot: I haz the power, 2017-08-25 05:37:16]

Description was changed from

==========
Utilizing the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

BUG=webrtc:5298
==========

to

==========
Utilizing the AEC3 config struct for constants.

This CL replaces inline constants with config struct 
constants.

BUG=webrtc:5298

Review-Url: https://codereview.webrtc.org/3003733002
Cr-Commit-Position: refs/heads/master@{#19507}
Committed:
https://chromium.googlesource.com/external/webrtc/+/8cee56f2546c329d193d9478e...
==========

[commit-bot: I haz the power, 2017-08-25 05:37:17]

Committed patchset #18 (id:490001) as
https://chromium.googlesource.com/external/webrtc/+/8cee56f2546c329d193d9478e...