Move smoothing filter to common audio.

Move smoothing filter to common audio.

This will make the smoothing filter a basic tool that is going to be used by both voice engine and ANA.

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

[michaelt, 2016-11-08 09:33:55]

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

[commit-bot: I haz the power, 2016-11-08 09:34:00]

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

[michaelt, 2016-11-08 09:34:53]

Description was changed from

==========
Move smoothing filter to base.
To be able to smooth bwe to pass it to ANA.

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

to

==========
To be able to smooth bwe to pass it to ANA.

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

[michaelt, 2016-11-08 09:34:53]

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

[michaelt, 2016-11-08 09:50:55]

Hi i moved smoothing filter to base to be able to smooth the BWE to send it to
ANA.

[minyue-webrtc, 2016-11-08 09:56:26]

Description was changed from

==========
To be able to smooth bwe to pass it to ANA.

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

to

==========
Move smoothing filter to base.

To be able to smooth bwe to pass it to ANA.

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

[minyue-webrtc, 2016-11-08 09:57:27]

Description was changed from

==========
Move smoothing filter to base.

To be able to smooth bwe to pass it to ANA.

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

to

==========
Move smoothing filter to base.

This will make the smoothing filter a basic tool that is going to be used by
both BWE and ANA.

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

[minyue-webrtc, 2016-11-08 09:58:43]

I updated the description. See if you like it. Otherwise, LGTM.

[commit-bot: I haz the power, 2016-11-08 10:01:10]

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

[commit-bot: I haz the power, 2016-11-08 10:01:11]

Dry run: This issue passed the CQ dry run.

[michaelt, 2016-11-08 10:03:33]

On 2016/11/08 09:58:43, minyue-webrtc wrote:
> I updated the description. See if you like it. Otherwise, LGTM.

Sounds fine to me.

[the sun, 2016-11-09 10:02:59]

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
File webrtc/base/mocks/mock_smoothing_filter.h (right):

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
webrtc/base/mocks/mock_smoothing_filter.h:19: class MockSmoothingFilter : public
SmoothingFilter {
Do you really need a mock smoothing filter? The tests that use it don't do
anything with the mock.

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
webrtc/base/mocks/mock_smoothing_filter.h:22: MOCK_METHOD0(Die, void());
The mock shouldn't introduce methods that are not on the class it mocks.

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter.h
File webrtc/base/smoothing_filter.h (right):

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter....
webrtc/base/smoothing_filter.h:23: virtual ~SmoothingFilter() = default;
Virtual methods are not without cost. Is it truly necessary to add these just in
order to mock the class?

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter....
webrtc/base/smoothing_filter.h:49:
RTC_DISALLOW_COPY_AND_ASSIGN(SmoothingFilterImpl);
...IMPLICIT_CONSTRUCTORS, since you're not default initing anything here.

[michaelt, 2016-11-09 10:46:35]

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
File webrtc/base/mocks/mock_smoothing_filter.h (right):

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
webrtc/base/mocks/mock_smoothing_filter.h:19: class MockSmoothingFilter : public
SmoothingFilter {
fec_controller_unittest.cc, the place where the mock is use depends on the mock.
Test something like that without a mock would lead to a strong dependency
between the filter implementation and the unittest of the fec controller. Beside
that the test would be more complicated since we would have to feed the filter
till we get the value we would like to test with.

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
webrtc/base/mocks/mock_smoothing_filter.h:22: MOCK_METHOD0(Die, void());
It seams this is quite common in webrtc code. With this function you can make
sure the object was destroyed.

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter.h
File webrtc/base/smoothing_filter.h (right):

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter....
webrtc/base/smoothing_filter.h:23: virtual ~SmoothingFilter() = default;
Abstract classes should always have a virtual destructor. Otherwise on
"
SmoothingFilter* filter = ....;
delete filter;
"
The dtor of SmoothingFilter would be called and not the one of the derived
class.

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter....
webrtc/base/smoothing_filter.h:49:
RTC_DISALLOW_COPY_AND_ASSIGN(SmoothingFilterImpl);
On 2016/11/09 10:02:59, the sun wrote:
> ...IMPLICIT_CONSTRUCTORS, since you're not default initing anything here.

Done.

[the sun, 2016-11-09 11:12:17]

lgtm

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
File webrtc/base/mocks/mock_smoothing_filter.h (right):

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
webrtc/base/mocks/mock_smoothing_filter.h:19: class MockSmoothingFilter : public
SmoothingFilter {
On 2016/11/09 10:46:34, michaelt wrote:
> fec_controller_unittest.cc, the place where the mock is use depends on the
mock.
> Test something like that without a mock would lead to a strong dependency
> between the filter implementation and the unittest of the fec controller.
Beside
> that the test would be more complicated since we would have to feed the filter
> till we get the value we would like to test with.
> 

Ah, sorry, I didn't notice it was also used as "packet_loss_smoothed".

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
webrtc/base/mocks/mock_smoothing_filter.h:22: MOCK_METHOD0(Die, void());
On 2016/11/09 10:46:34, michaelt wrote:
> It seams this is quite common in webrtc code. With this function you can make
> sure the object was destroyed. 
> 

Sure, but given that the SmoothingFilter doesn't release any resources in its
dtor I'm not sure I see the point. In fec_controller_unittest.cc you're
basically testing that unique_ptr works by doing this.

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter.h
File webrtc/base/smoothing_filter.h (right):

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/smoothing_filter....
webrtc/base/smoothing_filter.h:23: virtual ~SmoothingFilter() = default;
On 2016/11/09 10:46:34, michaelt wrote:
> Abstract classes should always have a virtual destructor. Otherwise on
> "
> SmoothingFilter* filter = ....;
> delete filter;
> "
> The dtor of SmoothingFilter would be called and not the one of the derived
> class.

Yes, I was wondering whether you could perhaps not mock it and just make this a
simple value class that can be copied around. But that was before I learned that
you were actually using the mock in the test.

[michaelt, 2016-11-09 12:33:52]

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
File webrtc/base/mocks/mock_smoothing_filter.h (right):

https://codereview.webrtc.org/2484153002/diff/1/webrtc/base/mocks/mock_smooth...
webrtc/base/mocks/mock_smoothing_filter.h:22: MOCK_METHOD0(Die, void());
On 2016/11/09 11:12:17, the sun wrote:
> On 2016/11/09 10:46:34, michaelt wrote:
> > It seams this is quite common in webrtc code. With this function you can
make
> > sure the object was destroyed. 
> > 
> 
> Sure, but given that the SmoothingFilter doesn't release any resources in its
> dtor I'm not sure I see the point. In fec_controller_unittest.cc you're
> basically testing that unique_ptr works by doing this.

Acknowledged. Removed "Die" Function.

[michaelt, 2016-11-09 12:34:10]

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

[commit-bot: I haz the power, 2016-11-09 12:34:18]

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

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

still lgtm

[commit-bot: I haz the power, 2016-11-09 12:54:07]

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

[commit-bot: I haz the power, 2016-11-09 12:54:08]

Dry run: This issue passed the CQ dry run.

[michaelt, 2016-11-09 13:00:02]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2016-11-09 13:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2484153002/#ps20001 (title:
"Resond to comments.")

[commit-bot: I haz the power, 2016-11-09 13:00:07]

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-11-09 13:02:50]

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

[commit-bot: I haz the power, 2016-11-09 13:02:51]

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

[michaelt, 2016-11-09 14:42:24]

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

[michaelt, 2016-11-09 14:43:21]

@perkj: I need your owner's approval for this CL.

[perkj_webrtc, 2016-11-10 12:09:06]

perkj@webrtc.org changed reviewers:
+ tommi@chromium.org

[perkj_webrtc, 2016-11-10 12:09:07]

Tommi, this is for rtc_base_approved. 
Would you mind taking a look?

[minyue-webrtc, 2016-11-14 09:02:29]

On 2016/11/10 12:09:07, perkj_webrtc wrote:
> Tommi, this is for rtc_base_approved. 
> Would you mind taking a look?

Hi Tommi,

Would you take a look?

[minyue-webrtc, 2016-11-16 09:02:04]

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

[minyue-webrtc, 2016-11-16 09:02:04]

Tommi,

Would you take a look?

[tommi, 2016-11-16 09:54:19]

tommi@webrtc.org changed reviewers:
- tommi@chromium.org

[tommi, 2016-11-16 09:58:03]

On 2016/11/16 09:02:04, minyue-webrtc wrote:
> Tommi,
> 
> Would you take a look?

Thanks Minyue.  Unfortunately this does not lgtm as is.
An audio filter does not belong in base approved.  base_approved is for things
that are even more common than things that are in 'base' (aka libjingle's base).
Can we discuss offline what would be a suitable place to put this in?  Maybe
even a new target?

[the sun, 2016-11-16 10:04:58]

On 2016/11/16 09:58:03, tommi (webrtc) wrote:
> On 2016/11/16 09:02:04, minyue-webrtc wrote:
> > Tommi,
> > 
> > Would you take a look?
> 
> Thanks Minyue.  Unfortunately this does not lgtm as is.
> An audio filter does not belong in base approved.  base_approved is for things
> that are even more common than things that are in 'base' (aka libjingle's
base).
> Can we discuss offline what would be a suitable place to put this in?  Maybe
> even a new target?

I'm interested in this discussion as well. I had similar concerns but LGTMed the
change since exp_filter is already in base_approved (it should probably be
moved).

[tommi, 2016-11-16 10:11:46]

On 2016/11/16 10:04:58, the sun wrote:
> On 2016/11/16 09:58:03, tommi (webrtc) wrote:
> > On 2016/11/16 09:02:04, minyue-webrtc wrote:
> > > Tommi,
> > > 
> > > Would you take a look?
> > 
> > Thanks Minyue.  Unfortunately this does not lgtm as is.
> > An audio filter does not belong in base approved.  base_approved is for
things
> > that are even more common than things that are in 'base' (aka libjingle's
> base).
> > Can we discuss offline what would be a suitable place to put this in?  Maybe
> > even a new target?
> 
> I'm interested in this discussion as well. I had similar concerns but LGTMed
the
> change since exp_filter is already in base_approved (it should probably be
> moved).

Yes, probably.  I see it was added though when 'approved' was initially created:
https://webrtc-codereview.appspot.com/22649004
Btw, I used the word "distilled" this morning, but as you can see from the
description of that CL, "purified" is another term :D

[michaelt, 2016-11-17 10:37:10]

As discussed offline, I moved smoothing filter to common audio. And created a
target for exp_filter in base.

[michaelt, 2016-11-17 10:37:43]

Description was changed from

==========
Move smoothing filter to base.

This will make the smoothing filter a basic tool that is going to be used by
both BWE and ANA.

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

to

==========
Move smoothing filter to common audio.

This will make the smoothing filter a basic tool that is going to be used by
both voice engine and ANA.

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

[michaelt, 2016-11-17 10:38:03]

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

[commit-bot: I haz the power, 2016-11-17 10:38:12]

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, 2016-11-17 10:40:39]

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

[commit-bot: I haz the power, 2016-11-17 10:40:41]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/18508)
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/19584)
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/13189)

[michaelt, 2016-11-17 10:45:02]

Patchset #3 (id:40001) has been deleted

[michaelt, 2016-11-17 10:45:56]

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

[commit-bot: I haz the power, 2016-11-17 10:46:07]

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, 2016-11-17 11:02:58]

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

[commit-bot: I haz the power, 2016-11-17 11:02:59]

Dry run: This issue passed the CQ dry run.

[minyue-webrtc, 2016-11-17 11:06:55]

On 2016/11/17 10:37:10, michaelt wrote:
> As discussed offline, I moved smoothing filter to common audio. And created a
> target for exp_filter in base.

Thanks!

Tommi,

Would you take a look? Michael tried to separate exp_filter as a new target.

He even considered moving exp_filter.h|cc out of base, but it is used in both
video and audio, and therefore cannot find a good home. Keeping it in base
folder can be good so far.

[tommi, 2016-11-17 11:25:26]

lgtm. Thanks for making the change.

[minyue-webrtc, 2016-11-18 08:21:29]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-11-18 08:21:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2484153002/#ps60001 (title:
"Move smoothing filter to common audio and creat target for exp_filter.")

[commit-bot: I haz the power, 2016-11-18 08:21:39]

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-11-18 08:23:21]

Description was changed from

==========
Move smoothing filter to common audio.

This will make the smoothing filter a basic tool that is going to be used by
both voice engine and ANA.

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

to

==========
Move smoothing filter to common audio.

This will make the smoothing filter a basic tool that is going to be used by
both voice engine and ANA.

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

[commit-bot: I haz the power, 2016-11-18 08:23:22]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-18 08:23:28]

Description was changed from

==========
Move smoothing filter to common audio.

This will make the smoothing filter a basic tool that is going to be used by
both voice engine and ANA.

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

to

==========
Move smoothing filter to common audio.

This will make the smoothing filter a basic tool that is going to be used by
both voice engine and ANA.

BUG=webrtc:6443

Committed: https://crrev.com/a82395bf7cd15b7396456df06fe952ede8db0c39
Cr-Commit-Position: refs/heads/master@{#15146}
==========

[commit-bot: I haz the power, 2016-11-18 08:23:29]

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

[magjed_webrtc, 2016-11-18 09:31:03]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.webrtc.org/2510373002/ by magjed@webrtc.org.

The reason for reverting is: Breaks downstream projects:
error: undefined reference to 'rtc::ExpFilter::kValueUndefined'
error: undefined reference to 'rtc::ExpFilter::Apply(float, float)'
error: undefined reference to 'rtc::ExpFilter::Reset(float)'
rror: undefined reference to 'rtc::ExpFilter::UpdateBase(float)'.