Injectable audio encoders: voice_engine/channel changes.

Injectable audio encoders: voice_engine/channel changes.

Adds a SetEncoder call to voe::Channel, so that we can move encoder setup outside of Voice Engine.

BUG=webrtc:5806
Review-Url: https://codereview.webrtc.org/2703373006
Cr-Commit-Position: refs/heads/master@{#17572}
Committed: https://chromium.googlesource.com/external/webrtc/+/1ffbd6c93cf322de23b3772c39021c58abe4b7bc

[ossu, 2017-02-22 13:21:03]

ossu@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, solenberg@webrtc.org

[ossu, 2017-02-22 13:21:04]

Split off a chunk of the webrtcvoiceengine CL into this one. Should make it
easier to review.

[the sun, 2017-02-22 14:08:38]

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

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1218: // TODO(ossu): Make a CodecInst up for now.
You need to explain why we should accept your lies. This is not politics.

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:189: AudioEncoderFactory* factory);  // Not part
of VoECodec!
Can you just move it up to e.g. line 173-ish and avoid that comment?

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:58: AudioEncoderFactory* factory);
Why are we sending the factory all the way down here? Isn't it possible to
instantiate the encoder in WVoMC and pass it in AudioSendStream::Config?

[ossu, 2017-02-22 14:24:41]

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

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1218: // TODO(ossu): Make a CodecInst up for now.
On 2017/02/22 14:08:38, the sun wrote:
> You need to explain why we should accept your lies. This is not politics.

Alright. I'll elaborate and give it a less offensive name. :)

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:189: AudioEncoderFactory* factory);  // Not part
of VoECodec!
On 2017/02/22 14:08:38, the sun wrote:
> Can you just move it up to e.g. line 173-ish and avoid that comment? 

Sure. I thought it logically fit better with SetSendCodec, since they provide
the same type of functionality. This is a bit weird as well, though.

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2703373006/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:58: AudioEncoderFactory* factory);
On 2017/02/22 14:08:38, the sun wrote:
> Why are we sending the factory all the way down here? Isn't it possible to
> instantiate the encoder in WVoMC and pass it in AudioSendStream::Config?


Ah, yes: since we need the information from SdpAudioFormat to register the
payload with the RtpRtcpModule, we'd need to send along the SdpAudioFormat
anyway. To me, it seemed strange to send the encoder and the format together,
since they could theoretically be out-of-sync. This way, the format and audio
encoder will be as in-sync as the factory allows. We're at least not handing any
ownership over to Channel here.

I'm willing to entertain other variants, such as making it possible to query the
SdpAudioFormat from an AudioEncoder.

I believe this could be made clearer after an overhaul to the audio parts of the
RtpRtcpModule. I'm planning that as a follow-up to this (if only to remove its
use of CodecInst).

[ossu, 2017-02-23 19:28:22]

Patchset #3 (id:40001) has been deleted

[ossu, 2017-02-23 19:41:47]

New patch set here as well. SetSendFormat is now replaced by SetEncoder(). It's
simpler but lies even a bit more to calm the rtpRtcpModule.

[kwiberg-webrtc, 2017-02-24 11:54:44]

lgtm

https://codereview.webrtc.org/2703373006/diff/60001/webrtc/test/mock_voe_chan...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/2703373006/diff/60001/webrtc/test/mock_voe_chan...
webrtc/test/mock_voe_channel_proxy.h:31: std::unique_ptr<AudioEncoder>&
encoder));
For style guide compliance, make this a pointer instead. (BTW, isn't the
move-only restriction only for return values? Or maybe it was return values that
worked fine...)

[ossu, 2017-02-24 16:20:58]

https://codereview.webrtc.org/2703373006/diff/60001/webrtc/test/mock_voe_chan...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/2703373006/diff/60001/webrtc/test/mock_voe_chan...
webrtc/test/mock_voe_channel_proxy.h:31: std::unique_ptr<AudioEncoder>&
encoder));
On 2017/02/24 11:54:44, kwiberg-webrtc wrote:
> For style guide compliance, make this a pointer instead. (BTW, isn't the
> move-only restriction only for return values? Or maybe it was return values
that
> worked fine...)

Neither works, I believe. Without this work-around, the compiler complains about
trying to use a deleted copy constructor and this is the solution I found. I'll
change it to a pointer for style-guide compliance, albeit begrudgingly.

[the sun, 2017-03-02 20:44:22]

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

https://codereview.webrtc.org/2703373006/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:32: std::unique_ptr<AudioEncoder> encoder)
{
add thread check + dcheck assumptions on pt range

[ossu, 2017-03-04 01:38:52]

Addressed the issues in comments.

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

https://codereview.webrtc.org/2703373006/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.cc:32: std::unique_ptr<AudioEncoder> encoder)
{
On 2017/03/02 20:44:22, the sun wrote:
> add thread check + dcheck assumptions on pt range

I've added the thread check here (since ChannelProxy is where all other thread
checks are) and the payload type dchecks in Channel (since that is where the
actual implementation resides).

[the sun, 2017-03-10 12:55:43]

lgtm

https://codereview.webrtc.org/2703373006/diff/80001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2703373006/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:174: virtual bool SetEncoder(int payload_type,
Remove "virtual"?

[ossu, 2017-03-20 18:06:32]

Patchset #5 (id:100001) has been deleted

[ossu, 2017-04-06 17:42:09]

Description was changed from

==========
Injectable audio encoders: voice_engine/channel changes.

BUG=webrtc:5806
==========

to

==========
Injectable audio encoders: voice_engine/channel changes.

Adds a SetEncoder call to voe::Channel, so that we can move encoder setup
outside of Voice Engine.

BUG=webrtc:5806
==========

[ossu, 2017-04-06 17:49:18]

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

[commit-bot: I haz the power, 2017-04-06 17:49:26]

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

[ossu, 2017-04-06 18:34:57]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2017-04-06 18:34:58]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org,
solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2703373006/#ps180001 (title:
"Removed 'virtual' from Channel::SetEncoder.")

[commit-bot: I haz the power, 2017-04-06 18:35:04]

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-04-06 19:05:05]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491503697732830,
"parent_rev": "5f4aaeb0a431d4e465f7d68b574b88147f7d101f", "commit_rev":
"1ffbd6c93cf322de23b3772c39021c58abe4b7bc"}

[commit-bot: I haz the power, 2017-04-06 19:05:08]

Description was changed from

==========
Injectable audio encoders: voice_engine/channel changes.

Adds a SetEncoder call to voe::Channel, so that we can move encoder setup
outside of Voice Engine.

BUG=webrtc:5806
==========

to

==========
Injectable audio encoders: voice_engine/channel changes.

Adds a SetEncoder call to voe::Channel, so that we can move encoder setup
outside of Voice Engine.

BUG=webrtc:5806

Review-Url: https://codereview.webrtc.org/2703373006
Cr-Commit-Position: refs/heads/master@{#17572}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1ffbd6c93cf322de23b3772c3...
==========

[commit-bot: I haz the power, 2017-04-06 19:05:09]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/1ffbd6c93cf322de23b3772c3...