Moving/renaming webrtc/common.h.

Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and webrtc/test/common_unittests.cc are moved to modules/audio_processing and the few remaining uses of webrtc::Config are replaced with simpler code.

- For NetEq and pacing configuration, a VoEBase::ChannelConfig is passed to VoEBase::CreateChannel().
- Removes the need for VoiceEngine::Create(const Config& config). No need to store the webrtc::Config in VoE shared state.

BUG=webrtc:5879
Committed: https://crrev.com/88499ecaca050be2dd21fff17140d6681c3bac95
Cr-Commit-Position: refs/heads/master@{#14109}

[the sun, 2016-09-01 19:10:18]

Description was changed from

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/call/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code (plain
structs and flags).

BUG=webrtc:5879
==========

to

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code (plain
structs and flags).

BUG=webrtc:5879
==========

[the sun, 2016-09-01 19:27:02]

Description was changed from

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code (plain
structs and flags).

BUG=webrtc:5879
==========

to

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code.

- VoicePacing is now turned on always (it was always enabled in WVoE anyway).
- For NetEq configuration, an AudioCodingModule::Config is passed to
VoEBase::CreateChannel().

BUG=webrtc:5879
==========

[the sun, 2016-09-01 19:27:02]

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

[the sun, 2016-09-01 19:40:26]

solenberg@webrtc.org changed reviewers:
- henrik.lundin@webrtc.org

[the sun, 2016-09-01 20:24:48]

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

[commit-bot: I haz the power, 2016-09-01 20:24:51]

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

[the sun, 2016-09-01 20:26:30]

solenberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, kjellander@webrtc.org, tommi@webrtc.org

[commit-bot: I haz the power, 2016-09-01 20:47:17]

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

[commit-bot: I haz the power, 2016-09-01 20:47:18]

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

[tommi, 2016-09-01 21:02:11]

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/test/mock_voice_en...
File webrtc/test/mock_voice_engine.h (right):

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/test/mock_voice_en...
webrtc/test/mock_voice_engine.h:33: : VoiceEngineImpl(),
necessary?

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/voice_engine/voe_b...
File webrtc/voice_engine/voe_base_impl.cc (right):

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:367: AudioCodingModule::Config
acm_config_copy;
what is this a copy of?  should it be acm_config_copy(acm_config)?

[kjellander_webrtc, 2016-09-02 07:00:37]

Way to go! I guess you need to sort out the voe_auto_test issue first though.
lgtm from a build perspective.

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/BUILD.gn
File webrtc/BUILD.gn (left):

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/BUILD.gn#oldcode333
webrtc/BUILD.gn:333: "audio_sink.h",
This should have been moved in https://codereview.webrtc.org/2059703002/ but I
apparently missed that.

[the sun, 2016-09-02 07:02:45]

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

[the sun, 2016-09-02 07:02:46]

+stefan@, for input on always enabling voice pacing

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/test/mock_voice_en...
File webrtc/test/mock_voice_engine.h (right):

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/test/mock_voice_en...
webrtc/test/mock_voice_engine.h:33: : VoiceEngineImpl(),
On 2016/09/01 21:02:11, tommi (webrtc) wrote:
> necessary?

Done.

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/voice_engine/voe_b...
File webrtc/voice_engine/voe_base_impl.cc (right):

https://codereview.webrtc.org/2307533004/diff/40001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:367: AudioCodingModule::Config
acm_config_copy;
On 2016/09/01 21:02:11, tommi (webrtc) wrote:
> what is this a copy of?  should it be acm_config_copy(acm_config)?

Yes, good catch!

[tommi, 2016-09-02 07:59:52]

one more last thing...

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtcvoiceengine.h:157: size_t neteq_capacity = -1;
assigning a negative value to an unsigned type doesn't feel right (or comparing
against one for that matter).  is ~0 more appropriate (e.g. define a constant
for that special value)?

[kwiberg-webrtc, 2016-09-02 08:05:50]

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

[kwiberg-webrtc, 2016-09-02 08:05:51]

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtcvoiceengine.h:157: size_t neteq_capacity = -1;
On 2016/09/02 07:59:52, tommi (webrtc) wrote:
> assigning a negative value to an unsigned type doesn't feel right (or
comparing
> against one for that matter).  is ~0 more appropriate (e.g. define a constant
> for that special value)?

Now, sing with me:

 ♪ rtc::Optional, rtc::Optional, rtc::Optional, ... ♪♪

[stefan-webrtc, 2016-09-02 08:23:18]

lgtm for always enabling pacing.

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:883: pacing_enabled_(true),  // TODO(solenberg):
Remove flag.
Doesn't look too complicated to actually remove the flag in this CL. Great to
always having it enabled now anyway!

In case you forgot why we added it, it was to make sure the old VoE API still
worked since it wasn't possible to add a pacer in that case, which would cause
packets to be dropped (if I recall correctly).

[the sun, 2016-09-02 08:54:40]

Thanks for your patience - it appears I was a bit trigger happy sending this
piece out for review, but now it should be ok.

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtcvoiceengine.h:157: size_t neteq_capacity = -1;
On 2016/09/02 08:05:51, kwiberg-webrtc wrote:
> On 2016/09/02 07:59:52, tommi (webrtc) wrote:
> > assigning a negative value to an unsigned type doesn't feel right (or
> comparing
> > against one for that matter).  is ~0 more appropriate (e.g. define a
constant
> > for that special value)?
> 
> Now, sing with me:
> 
>  ♪ rtc::Optional, rtc::Optional, rtc::Optional, ... ♪♪

In this context, setting it to zero will work fine though.

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2307533004/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:883: pacing_enabled_(true),  // TODO(solenberg):
Remove flag.
On 2016/09/02 08:23:17, stefan-webrtc (holmer) wrote:
> Doesn't look too complicated to actually remove the flag in this CL. Great to
> always having it enabled now anyway!
> 
> In case you forgot why we added it, it was to make sure the old VoE API still
> worked since it wasn't possible to add a pacer in that case, which would cause
> packets to be dropped (if I recall correctly).

Yes, I realized, and that's why, unfortunately, we need to keep the flag. For
now. :/

[tommi, 2016-09-02 09:02:25]

lgtm

[the sun, 2016-09-02 09:41:41]

Description was changed from

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code.

- VoicePacing is now turned on always (it was always enabled in WVoE anyway).
- For NetEq configuration, an AudioCodingModule::Config is passed to
VoEBase::CreateChannel().

BUG=webrtc:5879
==========

to

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code.

- For NetEq and pacing configuration, a VoEBase::ChannelConfig is passed to
VoEBase::CreateChannel().
- Removes the need for VoiceEngine::Create(const Config& config). No need to
store the webrtc::Config in VoE shared state.

BUG=webrtc:5879
==========

[the sun, 2016-09-04 20:38:39]

On 2016/09/02 09:02:25, tommi (webrtc) wrote:
> lgtm

kwiberg, hlundin, do you have any comments?

[hlundin-webrtc, 2016-09-05 11:28:42]

Nice. A few comments/questions.

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/BUILD.gn
File webrtc/BUILD.gn (left):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/BUILD.gn#oldcode333
webrtc/BUILD.gn:333: "audio_sink.h",
Why did audio_sink.h have to go?

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
webrtc/voice_engine/include/voe_base.h:99: struct ChannelConfig {
We tend to let the Config struct of a class reside in the declaration of that
class. Following that pattern, the ChannelConfig should go inside the Channel
class as a public struct?

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/voe_...
File webrtc/voice_engine/voe_base_impl.cc (right):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/voe_...
webrtc/voice_engine/voe_base_impl.cc:368: config_copy.acm_config.decoder_factory
= decoder_factory_;
decoder_factory in the config will never be used. Document this behavior in the
h-file? (Might be slightly confusing for someone writing a test for
voe_base_impl, wondering why their fake decoder factory is never used.)

[kjellander_webrtc, 2016-09-05 11:49:47]

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/BUILD.gn
File webrtc/BUILD.gn (left):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/BUILD.gn#oldcode333
webrtc/BUILD.gn:333: "audio_sink.h",
On 2016/09/05 11:28:42, hlundin-webrtc wrote:
> Why did audio_sink.h have to go?

My comment in PS#3: This should have been moved in
https://codereview.webrtc.org/2059703002/ but I
apparently missed that.

[hlundin-webrtc, 2016-09-05 11:56:19]

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/BUILD.gn
File webrtc/BUILD.gn (left):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/BUILD.gn#oldcode333
webrtc/BUILD.gn:333: "audio_sink.h",
On 2016/09/05 11:49:46, kjellander_webrtc wrote:
> On 2016/09/05 11:28:42, hlundin-webrtc wrote:
> > Why did audio_sink.h have to go?
> 
> My comment in PS#3: This should have been moved in
> https://codereview.webrtc.org/2059703002/ but I
> apparently missed that.

Acknowledged.

[kwiberg-webrtc, 2016-09-05 14:36:24]

lgtm

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
webrtc/voice_engine/include/voe_base.h:99: struct ChannelConfig {
On 2016/09/05 11:28:42, hlundin-webrtc wrote:
> We tend to let the Config struct of a class reside in the declaration of that
> class. Following that pattern, the ChannelConfig should go inside the Channel
> class as a public struct?

+1

[the sun, 2016-09-05 15:25:04]

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
webrtc/voice_engine/include/voe_base.h:99: struct ChannelConfig {
On 2016/09/05 14:36:24, kwiberg-webrtc wrote:
> On 2016/09/05 11:28:42, hlundin-webrtc wrote:
> > We tend to let the Config struct of a class reside in the declaration of
that
> > class. Following that pattern, the ChannelConfig should go inside the
Channel
> > class as a public struct?
> 
> +1

Agree in principle, but

-2

The Channel class is not, nor should it be, part of the public API, which it
would have to be if we were to place the config there. Hence, the somewhat
awkward name.

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/voe_...
File webrtc/voice_engine/voe_base_impl.cc (right):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/voe_...
webrtc/voice_engine/voe_base_impl.cc:368: config_copy.acm_config.decoder_factory
= decoder_factory_;
On 2016/09/05 11:28:42, hlundin-webrtc wrote:
> decoder_factory in the config will never be used. Document this behavior in
the
> h-file? (Might be slightly confusing for someone writing a test for
> voe_base_impl, wondering why their fake decoder factory is never used.)

Done. See comment in voe_base.h

[hlundin-webrtc, 2016-09-06 12:07:21]

LGTM

You included a rebase in the last patch set, which unfortunately made the diff
from my last review tremendously noisy.

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/incl...
webrtc/voice_engine/include/voe_base.h:99: struct ChannelConfig {
On 2016/09/05 15:25:04, the sun wrote:
> On 2016/09/05 14:36:24, kwiberg-webrtc wrote:
> > On 2016/09/05 11:28:42, hlundin-webrtc wrote:
> > > We tend to let the Config struct of a class reside in the declaration of
> that
> > > class. Following that pattern, the ChannelConfig should go inside the
> Channel
> > > class as a public struct?
> > 
> > +1
> 
> Agree in principle, but
> 
> -2
> 
> The Channel class is not, nor should it be, part of the public API, which it
> would have to be if we were to place the config there. Hence, the somewhat
> awkward name.

Acknowledged.

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/voe_...
File webrtc/voice_engine/voe_base_impl.cc (right):

https://codereview.webrtc.org/2307533004/diff/100001/webrtc/voice_engine/voe_...
webrtc/voice_engine/voe_base_impl.cc:368: config_copy.acm_config.decoder_factory
= decoder_factory_;
On 2016/09/05 15:25:04, the sun wrote:
> On 2016/09/05 11:28:42, hlundin-webrtc wrote:
> > decoder_factory in the config will never be used. Document this behavior in
> the
> > h-file? (Might be slightly confusing for someone writing a test for
> > voe_base_impl, wondering why their fake decoder factory is never used.)
> 
> Done. See comment in voe_base.h

Acknowledged.

[the sun, 2016-09-07 08:49:31]

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

[commit-bot: I haz the power, 2016-09-07 08:49:43]

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-09-07 10:50:11]

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

[commit-bot: I haz the power, 2016-09-07 10:50:12]

Dry run: Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[the sun, 2016-09-07 11:58:06]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-09-07 11:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, stefan@webrtc.org, tommi@webrtc.org, kwiberg@webrtc.org,
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2307533004/#ps140001 (title:
"rebase")

[commit-bot: I haz the power, 2016-09-07 11:58:15]

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-09-07 12:55:14]

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

[commit-bot: I haz the power, 2016-09-07 12:55:15]

Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED, no build URL)

[the sun, 2016-09-07 14:25:58]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2016-09-07 14:26: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-09-07 14:34:44]

Description was changed from

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code.

- For NetEq and pacing configuration, a VoEBase::ChannelConfig is passed to
VoEBase::CreateChannel().
- Removes the need for VoiceEngine::Create(const Config& config). No need to
store the webrtc::Config in VoE shared state.

BUG=webrtc:5879
==========

to

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code.

- For NetEq and pacing configuration, a VoEBase::ChannelConfig is passed to
VoEBase::CreateChannel().
- Removes the need for VoiceEngine::Create(const Config& config). No need to
store the webrtc::Config in VoE shared state.

BUG=webrtc:5879
==========

[commit-bot: I haz the power, 2016-09-07 14:34:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-07 14:34:49]

Description was changed from

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code.

- For NetEq and pacing configuration, a VoEBase::ChannelConfig is passed to
VoEBase::CreateChannel().
- Removes the need for VoiceEngine::Create(const Config& config). No need to
store the webrtc::Config in VoE shared state.

BUG=webrtc:5879
==========

to

==========
Moving/renaming webrtc/common.h.

This file defines webrtc::Config which was mostly used by
modules/audio_processing. The files webrtc/common.h, webrtc/common.cc and
webrtc/test/common_unittests.cc are moved to modules/audio_processing and the
few remaining uses of webrtc::Config are replaced with simpler code.

- For NetEq and pacing configuration, a VoEBase::ChannelConfig is passed to
VoEBase::CreateChannel().
- Removes the need for VoiceEngine::Create(const Config& config). No need to
store the webrtc::Config in VoE shared state.

BUG=webrtc:5879

Committed: https://crrev.com/88499ecaca050be2dd21fff17140d6681c3bac95
Cr-Commit-Position: refs/heads/master@{#14109}
==========

[commit-bot: I haz the power, 2016-09-07 14:34:50]

Patchset 8 (id:??) landed as
https://crrev.com/88499ecaca050be2dd21fff17140d6681c3bac95
Cr-Commit-Position: refs/heads/master@{#14109}