Fix Channel::GetSendCodec when used together with SetEncoder.

Fix Channel::GetSendCodec when used together with SetEncoder.

When using the SetEncoder interface, there's no actual CodecInst to return from Channel::GetSendCodec. Before this CL, this was done by calling the ACM, which has functionality for generating a CodecInst with the necessary values even when handed an external encoder. Unfortunately, this call takes a lock and does some extra processing which isn't strictly necessary in this case. Since GetSendCodec is called inside the audio input callback code, this can cause problems.

This CL instead generates a CodecInst in the SetEncoder call and has GetSendCodec simply return that one if it's available. If it isn't the value from codec_manager_ is returned instead, as was the case before injectable audio codec related changes were added to Channel.

BUG=b/38018041

Review-Url: https://codereview.webrtc.org/2924363004
Cr-Commit-Position: refs/heads/master@{#18515}
Committed: https://chromium.googlesource.com/external/webrtc/+/76d29f9bf8604a30e01f7cc0496297efbbeac51b

[ossu, 2017-06-09 12:47:13]

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

[commit-bot: I haz the power, 2017-06-09 12:47:18]

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-09 12:51:27]

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

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

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/17959)

[ossu, 2017-06-09 13:01:03]

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

[commit-bot: I haz the power, 2017-06-09 13:01: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, 2017-06-09 13:05:20]

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

[commit-bot: I haz the power, 2017-06-09 13:05:21]

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/17962)

[ossu, 2017-06-09 13:13:52]

Description was changed from

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

Do this by caching the value.

BUG=
==========

to

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

Do this by caching the value.

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

[ossu, 2017-06-09 13:28:03]

ossu@webrtc.org changed reviewers:
+ henrika@webrtc.org

[ossu, 2017-06-09 13:28:21]

PTAL. This should fix the issues we're seeing.

[minyue-webrtc, 2017-06-09 13:36:09]

On 2017/06/09 13:28:21, ossu wrote:
> PTAL. This should fix the issues we're seeing.

please add b/38018041 in BUG

[ossu, 2017-06-09 13:38:28]

Description was changed from

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

Do this by caching the value.

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

to

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

Do this by caching the value.

BUG=b/38018041
==========

[ossu, 2017-06-09 13:38:53]

On 2017/06/09 13:36:09, minyue-webrtc wrote:
> On 2017/06/09 13:28:21, ossu wrote:
> > PTAL. This should fix the issues we're seeing.
> 
> please add b/38018041 in BUG

Right. Forgot that about that. Thanks!

[henrika_webrtc, 2017-06-09 13:50:04]

Please extend the description of what this CL changes exactly.

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

https://codereview.webrtc.org/2924363004/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1296: // Store a similarily made-up CodecInst for
GetSendCodec to return.
Similar to what?

[ossu, 2017-06-09 13:52:10]

Description was changed from

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

Do this by caching the value.

BUG=b/38018041
==========

to

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

When using the SetEncoder interface, there's no actual CodecInst to return from
Channel::GetSendCodec. Before this CL, this was done by calling the ACM, which
has functionality for generating a CodecInst with the necessary values even when
handed an external encoder. Unfortunately, this call takes a lock and does some
extra processing which isn't strictly necessary in this case. Since GetSendCodec
is called inside the audio input callback code, this can cause problems.

This CL instead generates a CodecInst in the SetEncoder call and has
GetSendCodec simply return that one if it's available. If it isn't the value
from codec_manager_ is returned instead, as was the case before injectable audio
codec related changes were added to Channel.

BUG=b/38018041
==========

[ossu, 2017-06-09 13:54:00]

Updated the description.

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

https://codereview.webrtc.org/2924363004/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1296: // Store a similarily made-up CodecInst for
GetSendCodec to return.
On 2017/06/09 13:50:04, henrika_webrtc wrote:
> Similar to what?

On line 1281 I explain that I'm making a CodecInst up. Down here I'm also making
one up. I'll update the comments to be clearer and add a TODO for me on this one
as well.

[henrika_webrtc, 2017-06-09 13:59:42]

lgtm

[ossu, 2017-06-09 14:03:16]

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

[commit-bot: I haz the power, 2017-06-09 14:03:20]

Dry run: CQ is trying da patch.

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

[minyue-webrtc, 2017-06-09 14:04:06]

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

[minyue-webrtc, 2017-06-09 14:04:06]

lgtm

[commit-bot: I haz the power, 2017-06-09 14:25:01]

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

[commit-bot: I haz the power, 2017-06-09 14:25:01]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-06-09 14:28:28]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2017-06-09 14:28:28]

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

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

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-09 14:30:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497018508306750,
"parent_rev": "7fdd0676f94ccce8563034f75faba24f3a60e09d", "commit_rev":
"76d29f9bf8604a30e01f7cc0496297efbbeac51b"}

[commit-bot: I haz the power, 2017-06-09 14:30:18]

Description was changed from

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

When using the SetEncoder interface, there's no actual CodecInst to return from
Channel::GetSendCodec. Before this CL, this was done by calling the ACM, which
has functionality for generating a CodecInst with the necessary values even when
handed an external encoder. Unfortunately, this call takes a lock and does some
extra processing which isn't strictly necessary in this case. Since GetSendCodec
is called inside the audio input callback code, this can cause problems.

This CL instead generates a CodecInst in the SetEncoder call and has
GetSendCodec simply return that one if it's available. If it isn't the value
from codec_manager_ is returned instead, as was the case before injectable audio
codec related changes were added to Channel.

BUG=b/38018041
==========

to

==========
Fix Channel::GetSendCodec when used together with SetEncoder.

When using the SetEncoder interface, there's no actual CodecInst to return from
Channel::GetSendCodec. Before this CL, this was done by calling the ACM, which
has functionality for generating a CodecInst with the necessary values even when
handed an external encoder. Unfortunately, this call takes a lock and does some
extra processing which isn't strictly necessary in this case. Since GetSendCodec
is called inside the audio input callback code, this can cause problems.

This CL instead generates a CodecInst in the SetEncoder call and has
GetSendCodec simply return that one if it's available. If it isn't the value
from codec_manager_ is returned instead, as was the case before injectable audio
codec related changes were added to Channel.

BUG=b/38018041

Review-Url: https://codereview.webrtc.org/2924363004
Cr-Commit-Position: refs/heads/master@{#18515}
Committed:
https://chromium.googlesource.com/external/webrtc/+/76d29f9bf8604a30e01f7cc04...
==========

[commit-bot: I haz the power, 2017-06-09 14:30:19]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/76d29f9bf8604a30e01f7cc04...

[the sun, 2017-06-13 09:25:05]

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

[the sun, 2017-06-13 09:25:07]

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

https://codereview.webrtc.org/2924363004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1304: cached_send_codec_.emplace(send_codec);
Though this may work in practice - won't the TSAN bots complain at some point?
Writing on one thread, reading on another.

[ossu, 2017-06-13 09:39:15]

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

https://codereview.webrtc.org/2924363004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1304: cached_send_codec_.emplace(send_codec);
On 2017/06/13 09:25:07, the sun wrote:
> Though this may work in practice - won't the TSAN bots complain at some point?
> Writing on one thread, reading on another.

I was a bit worried about that as well, but having looked into it a bit, I can't
find that any of the previous code enforces thread safety either. There are
checkers in CodecManager and ChannelProxy, but no locks. C.f.
CodecManager::GetCodecInst (just returns a value in an Optional) and
CodecManager::RegisterEncoder (RTC_DCHECK(thread_checker...)).