Allow an external audio processing module to be used in WebRTC

Allow an external audio processing module to be used in WebRTC

[This CL is a rebase of an original CL by solenberg@:
https://codereview.webrtc.org/2948763002/ which in turn was a
rebase of an original CL by peah@:
https://chromium-review.googlesource.com/c/527032/]

Allow an external audio processing module to be used in WebRTC

This CL adds support for optionally using an externally created audio
processing module in a peerconnection. The ownership is shared
between the peerconnection and the external creator of the module.

As part of this the internal ownership of the audio processing module
is moved from VoiceEngine to WebRtcVoiceEngine.

BUG=webrtc:7775
Review-Url: https://codereview.webrtc.org/2961723004
Cr-Commit-Position: refs/heads/master@{#18837}
Committed: https://chromium.googlesource.com/external/webrtc/+/a9cc40b7d263cfce28bd3f126481e99283623d75

[peah-webrtc, 2017-06-27 14:51:17]

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

[commit-bot: I haz the power, 2017-06-27 14:51:30]

Dry run: CQ is trying da patch.

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

[peah-webrtc, 2017-06-27 14:51:36]

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

[peah-webrtc, 2017-06-27 15:07:25]

Description was changed from

==========
Allow an external audio processing module to be used in WebRTC

[This CL is a rebase of an original CL by solenberg@:
https://codereview.webrtc.org/2948763002/ which in turn was a
rebase of an original CL by peah@:
https://chromium-review.googlesource.com/c/527032/]

Allow an external audio processing module to be used in WebRTC

This CL adds support for optionally using an externally created audio
processing module in a peerconnection. The ownership is shared
between the peerconnection and the external creator of the module.

As part of this the internal ownership of the audio processing module
is moved from VoiceEngine to WebRtcVoiceEngine.

BUG=webrtc:7775
==========

to

==========
Allow an external audio processing module to be used in WebRTC

[This CL is a rebase of an original CL by solenberg@:
https://codereview.webrtc.org/2948763002/ which in turn was a
rebase of an original CL by peah@:
https://chromium-review.googlesource.com/c/527032/]

Allow an external audio processing module to be used in WebRTC

This CL adds support for optionally using an externally created audio
processing module in a peerconnection. The ownership is shared
between the peerconnection and the external creator of the module.

As part of this the internal ownership of the audio processing module
is moved from VoiceEngine to WebRtcVoiceEngine.

BUG=webrtc:7775
==========

[peah-webrtc, 2017-06-27 15:07:26]

peah@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, holmer@google.com, sprang@webrtc.org

[commit-bot: I haz the power, 2017-06-27 15:19:59]

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

[commit-bot: I haz the power, 2017-06-27 15:19:59]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-27 19:26:01]

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

[commit-bot: I haz the power, 2017-06-27 19:26:08]

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-06-27 19:45:39]

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

[commit-bot: I haz the power, 2017-06-27 19:45:40]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-27 21:32:29]

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

[commit-bot: I haz the power, 2017-06-27 21:32:32]

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-06-27 21:58:01]

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

[commit-bot: I haz the power, 2017-06-27 21:58:02]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-06-28 07:19:38]

lgtm with nits

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

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtcvoiceengine.h:75: // TODO(peah): Remove this when
upstream dependencies have properly been
nit: upstream or downstream? Is it something we're dependent on (up) or that's
dependent on us (down)?

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/inclu...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_base.h:137: // Returns NULL before Init() is
called.
nit: We generally use "null" in comments, reserving "nullptr"/"NULL" for code

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_base.h:138: // TODO(peah): Remove this when
upstream dependencies have properly been
nit: upstream vs downstream question here too

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

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:230: // TODO(peah): Add a DCHECK for
external_apmwhen upstream dependencies have
Here too, and space between "apm" and "when"

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:349: shared_->SetLastError(VE_NO_MEMORY);
Can you add a comment explaining why failure to create AudioProcessing can be
assumed to be a "no memory" error? Or change the Create method to return an
error?

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
File webrtc/voice_engine/voe_base_impl.h (right):

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.h:36: // TODO(peah): Remove this when upstream
dependencies have properly been
Here too

[peah-webrtc, 2017-06-28 09:08:58]

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

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

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-06-28 09:32:52]

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

[commit-bot: I haz the power, 2017-06-28 09:32:53]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-28 10:43:57]

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

[commit-bot: I haz the power, 2017-06-28 10:44: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, 2017-06-28 11:14:17]

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

[commit-bot: I haz the power, 2017-06-28 11:14:18]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-28 22:01:34]

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

[commit-bot: I haz the power, 2017-06-28 22:01:40]

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-06-28 22:03:58]

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

[commit-bot: I haz the power, 2017-06-28 22:04:00]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-28 22:22:41]

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

[commit-bot: I haz the power, 2017-06-28 22:22:48]

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-06-28 22:25:08]

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

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

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-29 03:57:01]

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

[commit-bot: I haz the power, 2017-06-29 03:57: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, 2017-06-29 04:34:37]

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

[commit-bot: I haz the power, 2017-06-29 04:34:38]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-29 10:33:21]

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

[commit-bot: I haz the power, 2017-06-29 10:33:31]

Dry run: CQ is trying da patch.

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

[the sun, 2017-06-29 10:41:52]

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/audio/audio_state.h
File webrtc/audio/audio_state.h (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/audio/audio_state...
webrtc/audio/audio_state.h:32: // constructor call when upstream dependencies
have properly been resolved.
comment is not right, mentioning transport proxy ctor

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:227: # "audio_coding:audio_coding_unittests",
Uh-oh! Please revert these changes - I was trying to track the linux32_rel
problem.

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl_unittest.cc (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl_unittest.cc:11: #if 0
Uh-oh!

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_unittest.cc (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_unittest.cc:11: #if 0
Uh-oh!

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc
(right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:72:
PacketFeedback received_packet(0, 665, kSeqNo, 101, kPacingInfo);
Revert!

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/voice_engine/test...
File webrtc/voice_engine/test/auto_test/fakes/conference_transport.h (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/voice_engine/test...
webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:158:
std::unique_ptr<webrtc::AudioProcessing> remote_apm_;
Use scoped for both

[commit-bot: I haz the power, 2017-06-29 10:56:15]

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

[commit-bot: I haz the power, 2017-06-29 10:56:16]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-29 11:45:36]

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

[commit-bot: I haz the power, 2017-06-29 11:45:45]

Dry run: CQ is trying da patch.

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

[peah-webrtc, 2017-06-29 11:46:32]

Thanks for the comments! I have addressed them, and uploaded a new patch.
PTAL

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

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtcvoiceengine.h:75: // TODO(peah): Remove this when
upstream dependencies have properly been
On 2017/06/28 07:19:38, Taylor Brandstetter wrote:
> nit: upstream or downstream? Is it something we're dependent on (up) or that's
> dependent on us (down)?

Good point!

Done.

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/inclu...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_base.h:137: // Returns NULL before Init() is
called.
On 2017/06/28 07:19:38, Taylor Brandstetter wrote:
> nit: We generally use "null" in comments, reserving "nullptr"/"NULL" for code

Done.

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_base.h:138: // TODO(peah): Remove this when
upstream dependencies have properly been
On 2017/06/28 07:19:38, Taylor Brandstetter wrote:
> nit: upstream vs downstream question here too

Done.

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

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:230: // TODO(peah): Add a DCHECK for
external_apmwhen upstream dependencies have
On 2017/06/28 07:19:38, Taylor Brandstetter wrote:
> Here too, and space between "apm" and "when"

Done.

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:349: shared_->SetLastError(VE_NO_MEMORY);
On 2017/06/28 07:19:38, Taylor Brandstetter wrote:
> Can you add a comment explaining why failure to create AudioProcessing can be
> assumed to be a "no memory" error? Or change the Create method to return an
> error?

This makes sense and this is not clear at all. However, this code will be
removed within a month. I added a short comment for now.

Done.

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
File webrtc/voice_engine/voe_base_impl.h (right):

https://codereview.webrtc.org/2961723004/diff/60001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.h:36: // TODO(peah): Remove this when upstream
dependencies have properly been
On 2017/06/28 07:19:38, Taylor Brandstetter wrote:
> Here too

Done.

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/audio/audio_state.h
File webrtc/audio/audio_state.h (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/audio/audio_state...
webrtc/audio/audio_state.h:32: // constructor call when upstream dependencies
have properly been resolved.
On 2017/06/29 10:41:52, the sun (OOO until July 24th) wrote:
> comment is not right, mentioning transport proxy ctor

Done.

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:227: # "audio_coding:audio_coding_unittests",
On 2017/06/29 10:41:52, the sun (OOO until July 24th) wrote:
> Uh-oh! Please revert these changes - I was trying to track the linux32_rel
> problem.

Done.

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl_unittest.cc (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl_unittest.cc:11: #if 0
On 2017/06/29 10:41:52, the sun (OOO until July 24th) wrote:
> Uh-oh!

Done.

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_unittest.cc (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_unittest.cc:11: #if 0
On 2017/06/29 10:41:52, the sun (OOO until July 24th) wrote:
> Uh-oh!

Done.

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc
(right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:72:
PacketFeedback received_packet(0, 665, kSeqNo, 101, kPacingInfo);
On 2017/06/29 10:41:52, the sun (OOO until July 24th) wrote:
> Revert!

Done.

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/voice_engine/test...
File webrtc/voice_engine/test/auto_test/fakes/conference_transport.h (right):

https://codereview.webrtc.org/2961723004/diff/140001/webrtc/voice_engine/test...
webrtc/voice_engine/test/auto_test/fakes/conference_transport.h:158:
std::unique_ptr<webrtc::AudioProcessing> remote_apm_;
On 2017/06/29 10:41:52, the sun (OOO until July 24th) wrote:
> Use scoped for both

Done.

[commit-bot: I haz the power, 2017-06-29 12:19:06]

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

[commit-bot: I haz the power, 2017-06-29 12:19:07]

Dry run: This issue passed the CQ dry run.

[peah-webrtc, 2017-06-29 13:32:10]

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

[peah-webrtc, 2017-06-29 13:32:47]

+magjed@ as an OWNER of 	webrtc/sdk

[sprang_webrtc, 2017-06-29 13:35:39]

rs lgtm for webrtc/video

[the sun, 2017-06-29 13:40:22]

lgtm

[stefan-webrtc, 2017-06-29 14:27:51]

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

[stefan-webrtc, 2017-06-29 14:27:52]

https://codereview.webrtc.org/2961723004/diff/160001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2961723004/diff/160001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:381: apm_send_ = AudioProcessing::Create();
This method is supposed to include legacy voice engine calls that should be
going away once the new voice engine API is finished. Therefore I think this
should be moved out of this function and put in RunBaseTest instead, similar to
AudioMixerImpl::Create().

[magjed_webrtc, 2017-06-29 14:32:30]

webrtc/sdk/android lgtm

[peah-webrtc, 2017-06-29 14:43:47]

Thanks for the comments!
I have uploaded a new path which moved creation of APMs from CreateVoiceEngines
according to the comment.

[stefan-webrtc, 2017-06-29 15:01:00]

lgtm

[peah-webrtc, 2017-06-29 15:04:11]

peah@webrtc.org changed reviewers:
- holmer@google.com

[peah-webrtc, 2017-06-29 15:05:19]

Thanks everyone for the reviews!!!!

[peah-webrtc, 2017-06-29 15:05:34]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-06-29 15:05:35]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
sprang@webrtc.org, magjed@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2961723004/#ps180001 (title:
"Moved creation of APMs from CreateVoiceEngines")

[commit-bot: I haz the power, 2017-06-29 15:05:38]

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-06-29 15:32:10]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498748734789580,
"parent_rev": "3dbfac3515db2da6a69c00c5061add0e2eb97ead", "commit_rev":
"a9cc40b7d263cfce28bd3f126481e99283623d75"}

[commit-bot: I haz the power, 2017-06-29 15:32:17]

Description was changed from

==========
Allow an external audio processing module to be used in WebRTC

[This CL is a rebase of an original CL by solenberg@:
https://codereview.webrtc.org/2948763002/ which in turn was a
rebase of an original CL by peah@:
https://chromium-review.googlesource.com/c/527032/]

Allow an external audio processing module to be used in WebRTC

This CL adds support for optionally using an externally created audio
processing module in a peerconnection. The ownership is shared
between the peerconnection and the external creator of the module.

As part of this the internal ownership of the audio processing module
is moved from VoiceEngine to WebRtcVoiceEngine.

BUG=webrtc:7775
==========

to

==========
Allow an external audio processing module to be used in WebRTC

[This CL is a rebase of an original CL by solenberg@:
https://codereview.webrtc.org/2948763002/ which in turn was a
rebase of an original CL by peah@:
https://chromium-review.googlesource.com/c/527032/]

Allow an external audio processing module to be used in WebRTC

This CL adds support for optionally using an externally created audio
processing module in a peerconnection. The ownership is shared
between the peerconnection and the external creator of the module.

As part of this the internal ownership of the audio processing module
is moved from VoiceEngine to WebRtcVoiceEngine.

BUG=webrtc:7775

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

[commit-bot: I haz the power, 2017-06-29 15:32:19]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/a9cc40b7d263cfce28bd3f126...