Open backdoor in VoiceEngineImpl to get at the actual voe::Channel objects from an ID.

Open backdoor in VoiceEngineImpl to get at the actual voe::Channel objects from an ID.
This will allow Audio[Send|Receive]Stream to bypass the VoE interfaces in many cases and talk directly to the channel.

BUG=webrtc:4690
Committed: https://crrev.com/13725089ef91f932b37b2447c3f05d9cd9f89984
Cr-Commit-Position: refs/heads/master@{#10788}

[the sun, 2015-11-23 11:37:47]

Description was changed from

==========
Open backdoor in VoiceEngineImpl to get at the actual voe::Channel objects from
an ID.
This will allow Audio[Send|Receive]Stream to bypass the VoE interfaces in many
cases and talk directly to the channel.

BUG=webrtc:4690
==========

to

==========
Open backdoor in VoiceEngineImpl to get at the actual voe::Channel objects from
an ID.
This will allow Audio[Send|Receive]Stream to bypass the VoE interfaces in many
cases and talk directly to the channel.

BUG=webrtc:4690
==========

[the sun, 2015-11-24 09:49:21]

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

[the sun, 2015-11-24 16:13:38]

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

[the sun, 2015-11-24 16:13:38]

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

[the sun, 2015-11-24 16:13:52]

On 2015/11/24 09:49:21, the sun wrote:

Added pbos@ for webrtc/test

[pbos-webrtc, 2015-11-24 16:28:48]

rs-lgtm for webrtc/test

[kwiberg-webrtc, 2015-11-25 10:44:59]

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:68: .WillOnce(Invoke(this,
&ConfigHelper::CreateChannelProxy));
Can you do this with a lambda instead? Something like

.WillOnce(Invoke([this](int channel_id) {
  EXPECT_FALSE(channel_proxy_);
  channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>();
  EXPECT_CALL(*channel_proxy_, SetLocalSSRC(kLocalSsrc)).Times(1);
  return channel_proxy_;
}));

Although I don't quite see what you gain by storing the thing in a member
variable.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream_unittest.cc:54: .WillOnce(Invoke(this,
&ConfigHelper::CreateChannelProxy));
See earlier comment about lambda and whether channel_proxy_ is needed.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voe_cha...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voe_cha...
webrtc/test/mock_voe_channel_proxy.h:22: MockVoEChannelProxy() :
voe::ChannelProxy() {}
Do you need this? It looks like the default constructor the compiler would write
for you.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voe_cha...
webrtc/test/mock_voe_channel_proxy.h:23: ~MockVoEChannelProxy() override {}
Same here.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voice_e...
File webrtc/test/mock_voice_engine.h (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voice_e...
webrtc/test/mock_voice_engine.h:33: testing::Invoke(this,
&MockVoiceEngine::CreateChannelProxy));
Lambda here too?

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:19: }
What's this constructor for?

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:39: std::string c_name_limited =
c_name.substr(0, 255);
Would it make sense to CHECK the size instead, or do we actually need to handle
this case?

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/voic...
File webrtc/voice_engine/voice_engine_impl.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/voic...
webrtc/voice_engine/voice_engine_impl.cc:88: }
I think this should return a scoped_ptr instead of a raw pointer, to make it
clear that it relinquishes ownership of the returned value. Also, all callers
that I can recall just ended up storing it in a scoped_ptr anyway.

[the sun, 2015-11-25 12:28:02]

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:68: .WillOnce(Invoke(this,
&ConfigHelper::CreateChannelProxy));
On 2015/11/25 10:44:58, kwiberg-webrtc wrote:
> Can you do this with a lambda instead? Something like
> 
> .WillOnce(Invoke([this](int channel_id) {
>   EXPECT_FALSE(channel_proxy_);
>   channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>();
>   EXPECT_CALL(*channel_proxy_, SetLocalSSRC(kLocalSsrc)).Times(1);
>   return channel_proxy_;
> }));
> 
> Although I don't quite see what you gain by storing the thing in a member
> variable.

Great idea. I simply didn't think a lambda would work in this case.

channel_proxy_ isn't used right now, but will be once GetStats() direct VoE
calls through the ChannelProxy, and SetupMockForGetStats() as a consequence
needs to add expectations on the ChannelProxy mock.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream_unittest.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream_unittest.cc:54: .WillOnce(Invoke(this,
&ConfigHelper::CreateChannelProxy));
On 2015/11/25 10:44:59, kwiberg-webrtc wrote:
> See earlier comment about lambda and whether channel_proxy_ is needed.

channel_proxy_ *will* be needed. Once the methods for obtaining stats are added
to ChannelProxy, the below method SetupMockForGetStats() will start adding
expectations on the channel_proxy_ mock.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voe_cha...
File webrtc/test/mock_voe_channel_proxy.h (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voe_cha...
webrtc/test/mock_voe_channel_proxy.h:22: MockVoEChannelProxy() :
voe::ChannelProxy() {}
On 2015/11/25 10:44:59, kwiberg-webrtc wrote:
> Do you need this? It looks like the default constructor the compiler would
write
> for you.

Done.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voe_cha...
webrtc/test/mock_voe_channel_proxy.h:23: ~MockVoEChannelProxy() override {}
On 2015/11/25 10:44:59, kwiberg-webrtc wrote:
> Same here.

Done.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voice_e...
File webrtc/test/mock_voice_engine.h (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/test/mock_voice_e...
webrtc/test/mock_voice_engine.h:33: testing::Invoke(this,
&MockVoiceEngine::CreateChannelProxy));
On 2015/11/25 10:44:59, kwiberg-webrtc wrote:
> Lambda here too?

Done.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:19: }
On 2015/11/25 10:44:59, kwiberg-webrtc wrote:
> What's this constructor for?

For the mock.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:39: std::string c_name_limited =
c_name.substr(0, 255);
On 2015/11/25 10:44:59, kwiberg-webrtc wrote:
> Would it make sense to CHECK the size instead, or do we actually need to
handle
> this case?

AFAIU this string comes directly from the SDP, so yes, this is mitigating a
potential buffer overrun issue.

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/voic...
File webrtc/voice_engine/voice_engine_impl.cc (right):

https://codereview.webrtc.org/1459083007/diff/100001/webrtc/voice_engine/voic...
webrtc/voice_engine/voice_engine_impl.cc:88: }
On 2015/11/25 10:44:59, kwiberg-webrtc wrote:
> I think this should return a scoped_ptr instead of a raw pointer, to make it
> clear that it relinquishes ownership of the returned value. Also, all callers
> that I can recall just ended up storing it in a scoped_ptr anyway.

Yes, that works great. Thanks.

[kwiberg-webrtc, 2015-11-25 12:56:57]

lgtm

https://codereview.webrtc.org/1459083007/diff/140001/webrtc/voice_engine/voe_...
File webrtc/voice_engine/voe_base_impl.h (right):

https://codereview.webrtc.org/1459083007/diff/140001/webrtc/voice_engine/voe_...
webrtc/voice_engine/voe_base_impl.h:66: uint32_t& newMicLevel) override;
Making arguments const like this is usually not with it; the only effect is to
constrain the function implementation, so to callers it's just noise. (Ideally
we'd like to omit consts in the declarations but have them in the definitions,
but last I tried it that didn't work with MSVC; IIRC it complained that
declaration and definition didn't match.)

Besides, you're being inconsistent by not making audioSamples const as well.

[the sun, 2015-11-25 13:06:44]

https://codereview.webrtc.org/1459083007/diff/140001/webrtc/voice_engine/voe_...
File webrtc/voice_engine/voe_base_impl.h (right):

https://codereview.webrtc.org/1459083007/diff/140001/webrtc/voice_engine/voe_...
webrtc/voice_engine/voe_base_impl.h:66: uint32_t& newMicLevel) override;
On 2015/11/25 12:56:57, kwiberg-webrtc wrote:
> Making arguments const like this is usually not with it; the only effect is to
> constrain the function implementation, so to callers it's just noise. (Ideally
> we'd like to omit consts in the declarations but have them in the definitions,
> but last I tried it that didn't work with MSVC; IIRC it complained that
> declaration and definition didn't match.)
> 
> Besides, you're being inconsistent by not making audioSamples const as well.

It's not me, it's the system.

Agree that this is bogus too. For some reason, the Win64 GN build started
failing because this implementation of the AudioTransport interface didn't match
the definition in audio_device_defines.h. So I've simply aligned the impl here
with that, to make the compiler happy.

Yes, the interface as such is in need of a hug or two...

[the sun, 2015-11-25 13:07:04]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-11-25 13:07:04]

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

[commit-bot: I haz the power, 2015-11-25 13:07:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459083007/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459083007/140001

[commit-bot: I haz the power, 2015-11-25 13:09:28]

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

[commit-bot: I haz the power, 2015-11-25 13:09:29]

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

[the sun, 2015-11-25 13:23:15]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-11-25 13:23:15]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1459083007/#ps160001 (title:
"presubmit complaints")

[commit-bot: I haz the power, 2015-11-25 13:23:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459083007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459083007/160001

[commit-bot: I haz the power, 2015-11-25 15:23:37]

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

[commit-bot: I haz the power, 2015-11-25 15:23:37]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[the sun, 2015-11-25 15:36:37]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2015-11-25 15:36:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459083007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459083007/160001

[commit-bot: I haz the power, 2015-11-25 16:16:56]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-25 16:17:05]

Patchset 9 (id:??) landed as
https://crrev.com/13725089ef91f932b37b2447c3f05d9cd9f89984
Cr-Commit-Position: refs/heads/master@{#10788}