Update common_audio/smoothing_filter.

Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying the output, it assumes the input to continue even if no actual new samples are added.

The new implementation does not rely on base/exp_filter any longer. This is because it would be a bit cumbersome. base/exp_filter does pre-extrapolate, i.e., it assumes the all missing samples since the last sample equals the new sample.

BUG=webrtc:6443
Committed: https://crrev.com/301fc4a712fa507fe14a3bd4688856441bd60011
Cr-Commit-Position: refs/heads/master@{#15575}

[minyue-webrtc, 2016-12-05 22:53:32]

Description was changed from

==========
update common_audio/soomthing_filter.

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

to

==========
Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying
the output, it assumes the input to continue even if no actual new samples are
added.

The new implementation does not rely on base/exp_filter any longer. This is
because it would be a bit cumbersome. base/exp_filter does pre-extrapolate,
i.e., it assumes the all missing samples since the last sample equals the new
sample.

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

[minyue-webrtc, 2016-12-05 22:53:32]

minyue@webrtc.org changed reviewers:
+ michaelt@webrtc.org

[michaelt, 2016-12-07 09:44:17]

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
File webrtc/common_audio/smoothing_filter.cc (left):

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
webrtc/common_audio/smoothing_filter.cc:39: if (!initialized_) {
Would move this if statement to ExtrapolateLastSample.

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
File webrtc/common_audio/smoothing_filter.cc (right):

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
webrtc/common_audio/smoothing_filter.cc:65: if (!initialized_) {
would change the impl. of this function to:

init_time_constant_ms_ = time_constant_ms;
if(initialized_)
 UpdateAlpha(time_constant_ms);


|init_time_constant_ms_| should the may be |time_constant_ms_|.

[minyue-webrtc, 2016-12-07 17:16:32]

Hi Michael,

Thanks for your comments. I have some different opinion. Look forward to your
further discussion.

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
File webrtc/common_audio/smoothing_filter.cc (left):

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
webrtc/common_audio/smoothing_filter.cc:39: if (!initialized_) {
On 2016/12/07 09:44:16, michaelt wrote:
> Would move this if statement to ExtrapolateLastSample.

It feels weird that GetAverage changes the states. (it becomes non-constant
method)

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
File webrtc/common_audio/smoothing_filter.cc (right):

https://codereview.webrtc.org/2551363002/diff/20001/webrtc/common_audio/smoot...
webrtc/common_audio/smoothing_filter.cc:65: if (!initialized_) {
On 2016/12/07 09:44:16, michaelt wrote:
> would change the impl. of this function to:
> 
> init_time_constant_ms_ = time_constant_ms;
> if(initialized_)
>  UpdateAlpha(time_constant_ms);
> 
> 
> |init_time_constant_ms_| should the may be |time_constant_ms_|.

It feels weird that SetTimeConstantMs has two meanings: 1) change initialization
period before  it ends, and 2) set time constant after initialization ends.

[minyue-webrtc, 2016-12-11 12:04:45]

Patchset #3 (id:40001) has been deleted

[minyue-webrtc, 2016-12-11 12:05:38]

Hi Michael,

Please review the new patch set, it contains a new initialization scheme.

[minyue-webrtc, 2016-12-12 09:01:52]

Description was changed from

==========
Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying
the output, it assumes the input to continue even if no actual new samples are
added.

The new implementation does not rely on base/exp_filter any longer. This is
because it would be a bit cumbersome. base/exp_filter does pre-extrapolate,
i.e., it assumes the all missing samples since the last sample equals the new
sample.

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

to

==========
Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying
the output, it assumes the input to continue even if no actual new samples are
added.

The new implementation does not rely on base/exp_filter any longer. This is
because it would be a bit cumbersome. base/exp_filter does pre-extrapolate,
i.e., it assumes the all missing samples since the last sample equals the new
sample.

BUG=webrtc:6443
==========

[minyue-webrtc, 2016-12-12 09:02:54]

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

[michaelt, 2016-12-12 09:05:30]

looks good just this nit.

https://codereview.webrtc.org/2551363002/diff/80001/webrtc/common_audio/smoot...
File webrtc/common_audio/smoothing_filter.cc (right):

https://codereview.webrtc.org/2551363002/diff/80001/webrtc/common_audio/smoot...
webrtc/common_audio/smoothing_filter.cc:81: // With algebraic derivation, we can
find that the state can be updated
Could you add the algebraic derivation ?

[minyue-webrtc, 2016-12-12 09:12:14]

Thanks for the comments!

+Henrik,

Would you take a look at this CL too?

There are some mathematics, you would be perfect for reviewing it :) A design
doc can be found here
https://docs.google.com/document/d/1de_tDk5z5KRMxwCjku6oRr7yAHnlcwVB2FEur5pMH...

https://codereview.webrtc.org/2551363002/diff/80001/webrtc/common_audio/smoot...
File webrtc/common_audio/smoothing_filter.cc (right):

https://codereview.webrtc.org/2551363002/diff/80001/webrtc/common_audio/smoot...
webrtc/common_audio/smoothing_filter.cc:81: // With algebraic derivation, we can
find that the state can be updated
On 2016/12/12 09:05:30, michaelt wrote:
> Could you add the algebraic derivation ?

I will try. But pure text will be a bit cumbersome to write derivation. I think
it would good to add LaTeX syntax in an appendix?

[minyue-webrtc, 2016-12-12 12:20:03]

Added Latex-syntaxed derivation.

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
File webrtc/common_audio/smoothing_filter_unittest.cc (right):

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
webrtc/common_audio/smoothing_filter_unittest.cc:62: //   import math
added an extra space to each line since I think it reads better.

[michaelt, 2016-12-12 12:51:32]

lgtm

[hlundin-webrtc, 2016-12-13 12:02:51]

LGTM with a few nits and a suggestion.

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
File webrtc/common_audio/smoothing_filter.cc (right):

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
webrtc/common_audio/smoothing_filter.cc:35: int64_t now_ms =
clock_->TimeInMilliseconds();
nit: const

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
webrtc/common_audio/smoothing_filter.cc:57: void
SmoothingFilterImpl::SetTimeConstantMs(int time_constant_ms) {
I think this method should return a bool, indicating if the time constant could
be set or not.

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
webrtc/common_audio/smoothing_filter.cc:112: // Taking $\alpha_{n} =
\exp{\gamma^n}$, $\gamma$ denotes |init_factor_|, the
Won't you have to escape the underscore characters (like '\_') outside of math
environments?

[minyue-webrtc, 2016-12-13 12:23:52]

Patchset #6 (id:120001) has been deleted

[minyue-webrtc, 2016-12-13 12:25:26]

Thanks!

comments are addressed.

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
File webrtc/common_audio/smoothing_filter.cc (right):

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
webrtc/common_audio/smoothing_filter.cc:35: int64_t now_ms =
clock_->TimeInMilliseconds();
On 2016/12/13 12:02:51, hlundin-webrtc wrote:
> nit: const

Done.

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
webrtc/common_audio/smoothing_filter.cc:57: void
SmoothingFilterImpl::SetTimeConstantMs(int time_constant_ms) {
On 2016/12/13 12:02:51, hlundin-webrtc wrote:
> I think this method should return a bool, indicating if the time constant
could
> be set or not.

Done.

https://codereview.webrtc.org/2551363002/diff/100001/webrtc/common_audio/smoo...
webrtc/common_audio/smoothing_filter.cc:112: // Taking $\alpha_{n} =
\exp{\gamma^n}$, $\gamma$ denotes |init_factor_|, the
On 2016/12/13 12:02:51, hlundin-webrtc wrote:
> Won't you have to escape the underscore characters (like '\_') outside of math
> environments?

I did not try to compile the text part. But yes, it is good to avoid mixing.

[minyue-webrtc, 2016-12-13 12:35:19]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-12-13 12:35:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2551363002/#ps140001 (title:
"on Henrik's comments")

[commit-bot: I haz the power, 2016-12-13 12:35:24]

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

[minyue-webrtc, 2016-12-13 12:35:29]

The CQ bit was unchecked by minyue@webrtc.org

[minyue-webrtc, 2016-12-13 12:36:25]

On 2016/12/13 12:35:24, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

An error make, third_party/android_protobuf should not be checked in. Adding a
Patch set to remove it.

[minyue-webrtc, 2016-12-13 12:37:59]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-12-13 12:38:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2551363002/#ps160001 (title:
"removing mistakenly added folder.")

[commit-bot: I haz the power, 2016-12-13 12:38:03]

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-13 12:39:13]

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

[commit-bot: I haz the power, 2016-12-13 12:39:14]

Try jobs failed on following builders:
  android_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...)
  android_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13378)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15737)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15594)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20955)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19679)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[minyue-webrtc, 2016-12-13 13:30:05]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-12-13 13:30:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2551363002/#ps200001 (title:
"fixing after rebasing")

[commit-bot: I haz the power, 2016-12-13 13:30:08]

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-13 13:35:08]

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

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

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

[minyue-webrtc, 2016-12-13 13:44:46]

Patchset #10 (id:220001) has been deleted

[minyue-webrtc, 2016-12-13 14:26:06]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-12-13 14:26:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2551363002/#ps240001 (title:
"fixing some bots")

[commit-bot: I haz the power, 2016-12-13 14:26:19]

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-13 14:52:56]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481639166789530,
"parent_rev": "dc1dbdf5b86ebeaab900cd54dbaec131879010ac", "commit_rev":
"65faed32344a4a94eb8107da8f3c46d4aea17625"}

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

Description was changed from

==========
Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying
the output, it assumes the input to continue even if no actual new samples are
added.

The new implementation does not rely on base/exp_filter any longer. This is
because it would be a bit cumbersome. base/exp_filter does pre-extrapolate,
i.e., it assumes the all missing samples since the last sample equals the new
sample.

BUG=webrtc:6443
==========

to

==========
Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying
the output, it assumes the input to continue even if no actual new samples are
added.

The new implementation does not rely on base/exp_filter any longer. This is
because it would be a bit cumbersome. base/exp_filter does pre-extrapolate,
i.e., it assumes the all missing samples since the last sample equals the new
sample.

BUG=webrtc:6443

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

[commit-bot: I haz the power, 2016-12-13 14:53:00]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-12-13 14:53:15]

Description was changed from

==========
Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying
the output, it assumes the input to continue even if no actual new samples are
added.

The new implementation does not rely on base/exp_filter any longer. This is
because it would be a bit cumbersome. base/exp_filter does pre-extrapolate,
i.e., it assumes the all missing samples since the last sample equals the new
sample.

BUG=webrtc:6443

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

to

==========
Update common_audio/smoothing_filter.

The improvement is mainly to extrapolate missing samples so that when querying
the output, it assumes the input to continue even if no actual new samples are
added.

The new implementation does not rely on base/exp_filter any longer. This is
because it would be a bit cumbersome. base/exp_filter does pre-extrapolate,
i.e., it assumes the all missing samples since the last sample equals the new
sample.

BUG=webrtc:6443

Committed: https://crrev.com/301fc4a712fa507fe14a3bd4688856441bd60011
Cr-Commit-Position: refs/heads/master@{#15575}
==========

[commit-bot: I haz the power, 2016-12-13 14:53:16]

Patchset 10 (id:??) landed as
https://crrev.com/301fc4a712fa507fe14a3bd4688856441bd60011
Cr-Commit-Position: refs/heads/master@{#15575}