Made AudioReceiveStream a mixer participant.

Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=webrtc:6346
Committed: https://crrev.com/aed581a4f3d31cac3bf5aa7284009ff129d5510c
Cr-Commit-Position: refs/heads/master@{#14707}

[aleloi, 2016-09-30 17:09:41]

Description was changed from

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

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

to

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=6346
==========

[aleloi, 2016-09-30 17:09:41]

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

[aleloi, 2016-09-30 17:10:28]

Here is another one! Could you please take a look?

[the sun, 2016-10-03 11:41:26]

https://codereview.webrtc.org/2378143004/diff/1/webrtc/audio/BUILD.gn
File webrtc/audio/BUILD.gn (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/audio/BUILD.gn#newcode32
webrtc/audio/BUILD.gn:32: "../modules/audio_mixer",
Should really just depend on interface here - not the whole module, but that
requires changing the structure first.

https://codereview.webrtc.org/2378143004/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.h:35: public MixerAudioSource {
Multiple inheritance with implementation classes is a slippery slope. Can we
make MixerAudioSource a pure interface before we do this?

Also, the *public* mix_history_ *pointer* in MixerAudioSource is a dangerous
construct. It seems to me MixerAudioSource should be split up into a) the
interface and b) the object containing the mix state for AudioMixerImpl. The
latter would be internal to AudioMixerImpl.

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:703: MixerAudioSource::AudioFrameInfo
new_audio_frame_info;
Init to kError and omit the default section of the switch

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:475: AudioFrame _audioFrame, mix_audio_frame_;
One member per line, please.

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:221: return
channel()->GetAudioFrameWithMuted(id, sample_rate_hz);
Add separate thread checker for audio output thread.

[aleloi, 2016-10-03 12:57:29]

https://codereview.webrtc.org/2378143004/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.h:35: public MixerAudioSource {
On 2016/10/03 11:41:26, the sun wrote:
> Multiple inheritance with implementation classes is a slippery slope. Can we
> make MixerAudioSource a pure interface before we do this?
> 
> Also, the *public* mix_history_ *pointer* in MixerAudioSource is a dangerous
> construct. It seems to me MixerAudioSource should be split up into a) the
> interface and b) the object containing the mix state for AudioMixerImpl. The
> latter would be internal to AudioMixerImpl.

ossu@ has pointed out the naked pointer problems a while ago. I have made a CL
that removes mix_history_ a dependency. webrtc::AudioReceiveStream is a pure
interface, so this is strictly speaking a no-multiple-inheritance rule violation
(at least not yet).

Splitting MixerAudioSource in two would have consequence on the way participants
can find out if they are mixed or not. Now a participant can call
this->WasMixed(). If I remove that, the functionality could either be moved to
AudioMixer or maybe be dropped completely. I think it's better to remove it
completely since it's not used anywhere.

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:703: MixerAudioSource::AudioFrameInfo
new_audio_frame_info;
On 2016/10/03 11:41:26, the sun wrote:
> Init to kError and omit the default section of the switch

Then the compiler warns about not handling every case.

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:475: AudioFrame _audioFrame, mix_audio_frame_;
On 2016/10/03 11:41:26, the sun wrote:
> One member per line, please.

Done.

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:221: return
channel()->GetAudioFrameWithMuted(id, sample_rate_hz);
On 2016/10/03 11:41:26, the sun wrote:
> Add separate thread checker for audio output thread.

That would probably have to wait a little while, because audio is requested from
two different threads in OpenSL ES: the first two frames are requested by the
user thread that does PeerConnection API calls (at least I think it's the user
thread, I've pasted a stack trace below). henrika@ has told me that this could
be changed. Alternatively we could wait actually connecting the mixer until the
behavior is changed.

(frames now shown: 
ChannelProxy::GetAudioFrameWithMuted()
AudioReceiveStream::GetAudioFrameWithMuted()
AudioMixerImpl::GetNonAnonymousAudio()
)

0  in webrtc::AudioMixerImpl::Mix of
../../webrtc/modules/audio_mixer/audio_mixer_impl.cc:194
1  in webrtc::(anonymous namespace)::MiMAudioTransport::NeedMorePlayData of
../../webrtc/audio/audio_state.cc:71
2  in webrtc::AudioDeviceBuffer::RequestPlayoutData of
../../webrtc/modules/audio_device/audio_device_buffer.cc:347
3  in webrtc::FineAudioBuffer::GetPlayoutData of
../../webrtc/modules/audio_device/fine_audio_buffer.cc:88
4  in webrtc::OpenSLESPlayer::EnqueuePlayoutData of
../../webrtc/modules/audio_device/android/opensles_player.cc:396
5  in webrtc::OpenSLESPlayer::StartPlayout of
../../webrtc/modules/audio_device/android/opensles_player.cc:123
6  in webrtc::AudioDeviceTemplate<webrtc::AudioRecordJni,
webrtc::OpenSLESPlayer>::StartPlayout of
../../webrtc/modules/audio_device/android/audio_device_template.h:183
7  in webrtc::AudioDeviceModuleImpl::StartPlayout of
../../webrtc/modules/audio_device/audio_device_impl.cc:1405
8  in webrtc::internal::AudioReceiveStream::Start of
../../webrtc/audio/audio_receive_stream.cc:177
9  in cricket::WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream::SetPlayout of
../../webrtc/media/engine/webrtcvoiceengine.cc:1425
10 in cricket::WebRtcVoiceMediaChannel::AddRecvStream of
../../webrtc/media/engine/webrtcvoiceengine.cc:2236
11 in cricket::BaseChannel::AddRecvStream_w of ../../webrtc/pc/channel.cc:1239
12 in cricket::BaseChannel::UpdateRemoteStreams_w of
../../webrtc/pc/channel.cc:1380
13 in cricket::VoiceChannel::SetRemoteContent_w of
../../webrtc/pc/channel.cc:1765
14 in rtc::MethodFunctor3<cricket::VoiceChannel, bool
(cricket::VoiceChannel::*)(unsigned int, int, int), bool, unsigned int, int,
int>::operator() of ../../webrtc/base/bind.h:389
15 in rtc::FunctorMessageHandler<bool,
rtc::MethodFunctor3<cricket::VoiceChannel, bool
(cricket::VoiceChannel::*)(unsigned int, int, int), bool, unsigned int, int,
int> >::OnMessage of ../../webrtc/base/messagehandler.h:44
16 in rtc::Thread::Send of ../../webrtc/base/thread.cc:361
17 in rtc::Thread::InvokeInternal of ../../webrtc/base/thread.cc:455
18 in rtc::Thread::Invoke<bool, rtc::MethodFunctor3<cricket::BaseChannel, bool
(cricket::BaseChannel::*)(cricket::MediaContentDescription const*,
cricket::ContentAction, std::__ndk1::basic_string<char,
std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >*), bool,
cricket::MediaContentDescription const*, cricket::ContentAction,
std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>,
std::__ndk1::allocator<char> >*> > of ../../webrtc/base/thread.h:170
19 in
cricket::BaseChannel::InvokeOnWorker<rtc::MethodFunctor3<cricket::BaseChannel,
bool (cricket::BaseChannel::*)(cricket::MediaContentDescription const*,
cricket::ContentAction, std::__ndk1::basic_string<char,
std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >*), bool,
cricket::MediaContentDescription const*, cricket::ContentAction,
std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>,
std::__ndk1::allocator<char> >*> > of ../../webrtc/pc/channel.h:352
20 in cricket::BaseChannel::SetRemoteContent of ../../webrtc/pc/channel.cc:440
21 in cricket::BaseChannel::PushdownRemoteDescription of
../../webrtc/pc/channel.cc:860
22 in webrtc::WebRtcSession::PushdownMediaDescription of
../../webrtc/api/webrtcsession.cc:935
23 in webrtc::WebRtcSession::UpdateSessionState of
../../webrtc/api/webrtcsession.cc:897
24 in webrtc::WebRtcSession::SetRemoteDescription of
../../webrtc/api/webrtcsession.cc:766
25 in webrtc::PeerConnection::SetRemoteDescription of
../../webrtc/api/peerconnection.cc:1169
26 in webrtc::MethodCall2<webrtc::PeerConnectionInterface, void,
webrtc::SetSessionDescriptionObserver*,
webrtc::SessionDescriptionInterface*>::Marshal of ../../webrtc/api/proxy.h:218
27 in
webrtc::PeerConnectionProxyWithInternal<webrtc::PeerConnectionInterface>::SetRemoteDescription
of ../../webrtc/api/peerconnectionproxy.h:64
28 in PeerConnectionInterfaceTest::DoSetSessionDescription of
../../webrtc/api/peerconnectioninterface_unittest.cc:782
29 in PeerConnectionInterfaceTest::DoSetRemoteDescription of
../../webrtc/api/peerconnectioninterface_unittest.cc:795
30 in PeerConnectionInterfaceTest::CreateAnswerAsRemoteDescription of
../../webrtc/api/peerconnectioninterface_unittest.cc:919
31 in PeerConnectionInterfaceTest::CreateOfferReceiveAnswer of
../../webrtc/api/peerconnectioninterface_unittest.cc:889
32 in PeerConnectionInterfaceTest::InitiateCall of
../../webrtc/api/peerconnectioninterface_unittest.cc:815

[aleloi, 2016-10-03 12:58:20]

Patchset #2 (id:20001) has been deleted

[aleloi, 2016-10-04 14:32:05]

Patchset #3 (id:60001) has been deleted

[aleloi, 2016-10-04 16:15:38]

Patchset #3 (id:80001) has been deleted

[the sun, 2016-10-04 20:41:12]

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:703: MixerAudioSource::AudioFrameInfo
new_audio_frame_info;
On 2016/10/03 12:57:28, aleloi wrote:
> On 2016/10/03 11:41:26, the sun wrote:
> > Init to kError and omit the default section of the switch
> 
> Then the compiler warns about not handling every case.

Right, handle all cases in the switch but leave out the catch-all "default"
since you will be warned if you're missing something. And also always initialize
variables, such as this one. It happens that code is moved around, and a defined
state is much easier to track if a problem surfaces.

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:221: return
channel()->GetAudioFrameWithMuted(id, sample_rate_hz);
On 2016/10/03 12:57:28, aleloi wrote:
> On 2016/10/03 11:41:26, the sun wrote:
> > Add separate thread checker for audio output thread.
> 
> That would probably have to wait a little while, because audio is requested
from
> two different threads in OpenSL ES: the first two frames are requested by the
> user thread that does PeerConnection API calls (at least I think it's the user
> thread, I've pasted a stack trace below). henrika@ has told me that this could
> be changed. Alternatively we could wait actually connecting the mixer until
the
> behavior is changed.
> 
> (frames now shown: 
> ChannelProxy::GetAudioFrameWithMuted()
> AudioReceiveStream::GetAudioFrameWithMuted()
> AudioMixerImpl::GetNonAnonymousAudio()
> )
> 
> 0  in webrtc::AudioMixerImpl::Mix of
> ../../webrtc/modules/audio_mixer/audio_mixer_impl.cc:194
> 1  in webrtc::(anonymous namespace)::MiMAudioTransport::NeedMorePlayData of
> ../../webrtc/audio/audio_state.cc:71
> 2  in webrtc::AudioDeviceBuffer::RequestPlayoutData of
> ../../webrtc/modules/audio_device/audio_device_buffer.cc:347
> 3  in webrtc::FineAudioBuffer::GetPlayoutData of
> ../../webrtc/modules/audio_device/fine_audio_buffer.cc:88
> 4  in webrtc::OpenSLESPlayer::EnqueuePlayoutData of
> ../../webrtc/modules/audio_device/android/opensles_player.cc:396
> 5  in webrtc::OpenSLESPlayer::StartPlayout of
> ../../webrtc/modules/audio_device/android/opensles_player.cc:123
> 6  in webrtc::AudioDeviceTemplate<webrtc::AudioRecordJni,
> webrtc::OpenSLESPlayer>::StartPlayout of
> ../../webrtc/modules/audio_device/android/audio_device_template.h:183
> 7  in webrtc::AudioDeviceModuleImpl::StartPlayout of
> ../../webrtc/modules/audio_device/audio_device_impl.cc:1405
> 8  in webrtc::internal::AudioReceiveStream::Start of
> ../../webrtc/audio/audio_receive_stream.cc:177
> 9  in cricket::WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream::SetPlayout
of
> ../../webrtc/media/engine/webrtcvoiceengine.cc:1425
> 10 in cricket::WebRtcVoiceMediaChannel::AddRecvStream of
> ../../webrtc/media/engine/webrtcvoiceengine.cc:2236
> 11 in cricket::BaseChannel::AddRecvStream_w of ../../webrtc/pc/channel.cc:1239
> 12 in cricket::BaseChannel::UpdateRemoteStreams_w of
> ../../webrtc/pc/channel.cc:1380
> 13 in cricket::VoiceChannel::SetRemoteContent_w of
> ../../webrtc/pc/channel.cc:1765
> 14 in rtc::MethodFunctor3<cricket::VoiceChannel, bool
> (cricket::VoiceChannel::*)(unsigned int, int, int), bool, unsigned int, int,
> int>::operator() of ../../webrtc/base/bind.h:389
> 15 in rtc::FunctorMessageHandler<bool,
> rtc::MethodFunctor3<cricket::VoiceChannel, bool
> (cricket::VoiceChannel::*)(unsigned int, int, int), bool, unsigned int, int,
> int> >::OnMessage of ../../webrtc/base/messagehandler.h:44
> 16 in rtc::Thread::Send of ../../webrtc/base/thread.cc:361
> 17 in rtc::Thread::InvokeInternal of ../../webrtc/base/thread.cc:455
> 18 in rtc::Thread::Invoke<bool, rtc::MethodFunctor3<cricket::BaseChannel, bool
> (cricket::BaseChannel::*)(cricket::MediaContentDescription const*,
> cricket::ContentAction, std::__ndk1::basic_string<char,
> std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >*), bool,
> cricket::MediaContentDescription const*, cricket::ContentAction,
> std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>,
> std::__ndk1::allocator<char> >*> > of ../../webrtc/base/thread.h:170
> 19 in
> cricket::BaseChannel::InvokeOnWorker<rtc::MethodFunctor3<cricket::BaseChannel,
> bool (cricket::BaseChannel::*)(cricket::MediaContentDescription const*,
> cricket::ContentAction, std::__ndk1::basic_string<char,
> std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >*), bool,
> cricket::MediaContentDescription const*, cricket::ContentAction,
> std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>,
> std::__ndk1::allocator<char> >*> > of ../../webrtc/pc/channel.h:352
> 20 in cricket::BaseChannel::SetRemoteContent of ../../webrtc/pc/channel.cc:440
> 21 in cricket::BaseChannel::PushdownRemoteDescription of
> ../../webrtc/pc/channel.cc:860
> 22 in webrtc::WebRtcSession::PushdownMediaDescription of
> ../../webrtc/api/webrtcsession.cc:935
> 23 in webrtc::WebRtcSession::UpdateSessionState of
> ../../webrtc/api/webrtcsession.cc:897
> 24 in webrtc::WebRtcSession::SetRemoteDescription of
> ../../webrtc/api/webrtcsession.cc:766
> 25 in webrtc::PeerConnection::SetRemoteDescription of
> ../../webrtc/api/peerconnection.cc:1169
> 26 in webrtc::MethodCall2<webrtc::PeerConnectionInterface, void,
> webrtc::SetSessionDescriptionObserver*,
> webrtc::SessionDescriptionInterface*>::Marshal of ../../webrtc/api/proxy.h:218
> 27 in
>
webrtc::PeerConnectionProxyWithInternal<webrtc::PeerConnectionInterface>::SetRemoteDescription
> of ../../webrtc/api/peerconnectionproxy.h:64
> 28 in PeerConnectionInterfaceTest::DoSetSessionDescription of
> ../../webrtc/api/peerconnectioninterface_unittest.cc:782
> 29 in PeerConnectionInterfaceTest::DoSetRemoteDescription of
> ../../webrtc/api/peerconnectioninterface_unittest.cc:795
> 30 in PeerConnectionInterfaceTest::CreateAnswerAsRemoteDescription of
> ../../webrtc/api/peerconnectioninterface_unittest.cc:919
> 31 in PeerConnectionInterfaceTest::CreateOfferReceiveAnswer of
> ../../webrtc/api/peerconnectioninterface_unittest.cc:889
> 32 in PeerConnectionInterfaceTest::InitiateCall of
> ../../webrtc/api/peerconnectioninterface_unittest.cc:815

Discussed offline: use RaceChecker and e.g. RTC_DCHECK_RUNS_SERIALIZED(x).

[aleloi, 2016-10-05 13:54:51]

Patchset #3 (id:100001) has been deleted

[aleloi, 2016-10-05 13:55:12]

Patchset #2 (id:40001) has been deleted

[aleloi, 2016-10-05 13:56:51]

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:703: MixerAudioSource::AudioFrameInfo
new_audio_frame_info;
On 2016/10/04 20:41:12, the sun wrote:
> On 2016/10/03 12:57:28, aleloi wrote:
> > On 2016/10/03 11:41:26, the sun wrote:
> > > Init to kError and omit the default section of the switch
> > 
> > Then the compiler warns about not handling every case.
> 
> Right, handle all cases in the switch but leave out the catch-all "default"
> since you will be warned if you're missing something. And also always
initialize
> variables, such as this one. It happens that code is moved around, and a
defined
> state is much easier to track if a problem surfaces.

Thanks, that seems a good tip for the future :)

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/2378143004/diff/1/webrtc/voice_engine/channel_p...
webrtc/voice_engine/channel_proxy.cc:221: return
channel()->GetAudioFrameWithMuted(id, sample_rate_hz);
On 2016/10/04 20:41:12, the sun wrote:
> On 2016/10/03 12:57:28, aleloi wrote:
> > On 2016/10/03 11:41:26, the sun wrote:
> > > Add separate thread checker for audio output thread.
> > 
> > That would probably have to wait a little while, because audio is requested
> from
> > two different threads in OpenSL ES: the first two frames are requested by
the
> > user thread that does PeerConnection API calls (at least I think it's the
user
> > thread, I've pasted a stack trace below). henrika@ has told me that this
could
> > be changed. Alternatively we could wait actually connecting the mixer until
> the
> > behavior is changed.
> > 
> > (frames now shown: 
> > ChannelProxy::GetAudioFrameWithMuted()
> > AudioReceiveStream::GetAudioFrameWithMuted()
> > AudioMixerImpl::GetNonAnonymousAudio()
> > )
> > 
> > 0  in webrtc::AudioMixerImpl::Mix of
> > ../../webrtc/modules/audio_mixer/audio_mixer_impl.cc:194
> > 1  in webrtc::(anonymous namespace)::MiMAudioTransport::NeedMorePlayData of
> > ../../webrtc/audio/audio_state.cc:71
> > 2  in webrtc::AudioDeviceBuffer::RequestPlayoutData of
> > ../../webrtc/modules/audio_device/audio_device_buffer.cc:347
> > 3  in webrtc::FineAudioBuffer::GetPlayoutData of
> > ../../webrtc/modules/audio_device/fine_audio_buffer.cc:88
> > 4  in webrtc::OpenSLESPlayer::EnqueuePlayoutData of
> > ../../webrtc/modules/audio_device/android/opensles_player.cc:396
> > 5  in webrtc::OpenSLESPlayer::StartPlayout of
> > ../../webrtc/modules/audio_device/android/opensles_player.cc:123
> > 6  in webrtc::AudioDeviceTemplate<webrtc::AudioRecordJni,
> > webrtc::OpenSLESPlayer>::StartPlayout of
> > ../../webrtc/modules/audio_device/android/audio_device_template.h:183
> > 7  in webrtc::AudioDeviceModuleImpl::StartPlayout of
> > ../../webrtc/modules/audio_device/audio_device_impl.cc:1405
> > 8  in webrtc::internal::AudioReceiveStream::Start of
> > ../../webrtc/audio/audio_receive_stream.cc:177
> > 9  in cricket::WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream::SetPlayout
> of
> > ../../webrtc/media/engine/webrtcvoiceengine.cc:1425
> > 10 in cricket::WebRtcVoiceMediaChannel::AddRecvStream of
> > ../../webrtc/media/engine/webrtcvoiceengine.cc:2236
> > 11 in cricket::BaseChannel::AddRecvStream_w of
../../webrtc/pc/channel.cc:1239
> > 12 in cricket::BaseChannel::UpdateRemoteStreams_w of
> > ../../webrtc/pc/channel.cc:1380
> > 13 in cricket::VoiceChannel::SetRemoteContent_w of
> > ../../webrtc/pc/channel.cc:1765
> > 14 in rtc::MethodFunctor3<cricket::VoiceChannel, bool
> > (cricket::VoiceChannel::*)(unsigned int, int, int), bool, unsigned int, int,
> > int>::operator() of ../../webrtc/base/bind.h:389
> > 15 in rtc::FunctorMessageHandler<bool,
> > rtc::MethodFunctor3<cricket::VoiceChannel, bool
> > (cricket::VoiceChannel::*)(unsigned int, int, int), bool, unsigned int, int,
> > int> >::OnMessage of ../../webrtc/base/messagehandler.h:44
> > 16 in rtc::Thread::Send of ../../webrtc/base/thread.cc:361
> > 17 in rtc::Thread::InvokeInternal of ../../webrtc/base/thread.cc:455
> > 18 in rtc::Thread::Invoke<bool, rtc::MethodFunctor3<cricket::BaseChannel,
bool
> > (cricket::BaseChannel::*)(cricket::MediaContentDescription const*,
> > cricket::ContentAction, std::__ndk1::basic_string<char,
> > std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >*), bool,
> > cricket::MediaContentDescription const*, cricket::ContentAction,
> > std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>,
> > std::__ndk1::allocator<char> >*> > of ../../webrtc/base/thread.h:170
> > 19 in
> >
cricket::BaseChannel::InvokeOnWorker<rtc::MethodFunctor3<cricket::BaseChannel,
> > bool (cricket::BaseChannel::*)(cricket::MediaContentDescription const*,
> > cricket::ContentAction, std::__ndk1::basic_string<char,
> > std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >*), bool,
> > cricket::MediaContentDescription const*, cricket::ContentAction,
> > std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>,
> > std::__ndk1::allocator<char> >*> > of ../../webrtc/pc/channel.h:352
> > 20 in cricket::BaseChannel::SetRemoteContent of
../../webrtc/pc/channel.cc:440
> > 21 in cricket::BaseChannel::PushdownRemoteDescription of
> > ../../webrtc/pc/channel.cc:860
> > 22 in webrtc::WebRtcSession::PushdownMediaDescription of
> > ../../webrtc/api/webrtcsession.cc:935
> > 23 in webrtc::WebRtcSession::UpdateSessionState of
> > ../../webrtc/api/webrtcsession.cc:897
> > 24 in webrtc::WebRtcSession::SetRemoteDescription of
> > ../../webrtc/api/webrtcsession.cc:766
> > 25 in webrtc::PeerConnection::SetRemoteDescription of
> > ../../webrtc/api/peerconnection.cc:1169
> > 26 in webrtc::MethodCall2<webrtc::PeerConnectionInterface, void,
> > webrtc::SetSessionDescriptionObserver*,
> > webrtc::SessionDescriptionInterface*>::Marshal of
../../webrtc/api/proxy.h:218
> > 27 in
> >
>
webrtc::PeerConnectionProxyWithInternal<webrtc::PeerConnectionInterface>::SetRemoteDescription
> > of ../../webrtc/api/peerconnectionproxy.h:64
> > 28 in PeerConnectionInterfaceTest::DoSetSessionDescription of
> > ../../webrtc/api/peerconnectioninterface_unittest.cc:782
> > 29 in PeerConnectionInterfaceTest::DoSetRemoteDescription of
> > ../../webrtc/api/peerconnectioninterface_unittest.cc:795
> > 30 in PeerConnectionInterfaceTest::CreateAnswerAsRemoteDescription of
> > ../../webrtc/api/peerconnectioninterface_unittest.cc:919
> > 31 in PeerConnectionInterfaceTest::CreateOfferReceiveAnswer of
> > ../../webrtc/api/peerconnectioninterface_unittest.cc:889
> > 32 in PeerConnectionInterfaceTest::InitiateCall of
> > ../../webrtc/api/peerconnectioninterface_unittest.cc:815
> 
> Discussed offline: use RaceChecker and e.g. RTC_DCHECK_RUNS_SERIALIZED(x).

Done.

[the sun, 2016-10-06 06:51:40]

Hi, it's not clear to me in which order you intend to land the outstanding CLs.
I'd prefer the interface cleanups before hooking the AudioReceiveStream up to
the new mixer, but I can be swayed if you have compelling arguments. Let me
know, and ping if necessary so I'm not blocking your progress.

[aleloi, 2016-10-07 08:50:17]

Patchset #3 (id:140001) has been deleted

[aleloi, 2016-10-13 11:49:02]

Patchset #3 (id:160001) has been deleted

[aleloi, 2016-10-13 11:49:10]

Patchset #3 (id:180001) has been deleted

[aleloi, 2016-10-13 11:52:39]

Patchset #3 (id:200001) has been deleted

[aleloi, 2016-10-13 12:01:15]

Patchset #3 (id:220001) has been deleted

[aleloi, 2016-10-13 12:01:31]

Patchset #3 (id:240001) has been deleted

[aleloi, 2016-10-13 12:02:45]

Could you take a look at this again? This CL has actually become smaller after
the mixer API split because we don't have to modify DEPS files now.

[the sun, 2016-10-13 12:16:57]

lg, remember to add webrtc: to the BUG= line, otherwise clicking the link leads
to the Chromium issue tracker, not WebRTC.

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:382: // From AudioMixer::Source
total nit: comments should en with a "."

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

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:222: // From AudioMixer::Source
remove comment.

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:105: rtc::ThreadChecker
mixer_callback_thread_checker_;
Use an rtc::RaceChecker here instead, and use i.e.
RTC_DCHECK_RUNS_SERIALIZED(audio_render_race_checker_) in the .cc

The reason is there are situations when both the calling capture and render
threads may change and "ownership" of the WebRTC resource is passed on. What we
do want to ensure is non-concurrent access - not necessarily lock to a single
thread *ever* calling these methods.

[aleloi, 2016-10-13 12:32:45]

Description was changed from

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=6346
==========

to

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=webrtc:6346
==========

[aleloi, 2016-10-13 13:33:57]

Patchset #4 (id:280001) has been deleted

[aleloi, 2016-10-13 13:34:37]

Patchset #4 (id:300001) has been deleted

[aleloi, 2016-10-13 13:34:56]

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:382: // From AudioMixer::Source
On 2016/10/13 12:16:56, the sun wrote:
> total nit: comments should en with a "."

Done.

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

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:222: // From AudioMixer::Source
On 2016/10/13 12:16:56, the sun wrote:
> remove comment.

Done.

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:105: rtc::ThreadChecker
mixer_callback_thread_checker_;
On 2016/10/13 12:16:56, the sun wrote:
> Use an rtc::RaceChecker here instead, and use i.e.
> RTC_DCHECK_RUNS_SERIALIZED(audio_render_race_checker_) in the .cc
> 
> The reason is there are situations when both the calling capture and render
> threads may change and "ownership" of the WebRTC resource is passed on. What
we
> do want to ensure is non-concurrent access - not necessarily lock to a single
> thread *ever* calling these methods.

Then I had an incorrect understanding of the threading model. I thought
previously that AudioMixer::Mix is called from a single thread. With this
assumption I "simplified" the locking scheme in the mixer. When the assumption
turned out to be not entirely correct (two threads asked for audio in Android
OpenSL ES, some tests restarted their audio poll thread) I changed the tests and
asked henrika@ to update OpenSL ES.

This means that a race checker will have to be added to AudioMixerImpl as well.
Depending on the difference between a thread checker and a race checker, locks
may have to be re-introduced to the mixer.

[the sun, 2016-10-13 13:51:33]

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2378143004/diff/260001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:105: rtc::ThreadChecker
mixer_callback_thread_checker_;
On 2016/10/13 13:34:56, aleloi wrote:
> On 2016/10/13 12:16:56, the sun wrote:
> > Use an rtc::RaceChecker here instead, and use i.e.
> > RTC_DCHECK_RUNS_SERIALIZED(audio_render_race_checker_) in the .cc
> > 
> > The reason is there are situations when both the calling capture and render
> > threads may change and "ownership" of the WebRTC resource is passed on. What
> we
> > do want to ensure is non-concurrent access - not necessarily lock to a
single
> > thread *ever* calling these methods.
> 
> Then I had an incorrect understanding of the threading model. I thought
> previously that AudioMixer::Mix is called from a single thread. With this
> assumption I "simplified" the locking scheme in the mixer. When the assumption
> turned out to be not entirely correct (two threads asked for audio in Android
> OpenSL ES, some tests restarted their audio poll thread) I changed the tests
and
> asked henrika@ to update OpenSL ES.
> 
> This means that a race checker will have to be added to AudioMixerImpl as
well.
> Depending on the difference between a thread checker and a race checker, locks
> may have to be re-introduced to the mixer.

Acknowledged.

[the sun, 2016-10-13 13:55:39]

lgtm

https://codereview.webrtc.org/2378143004/diff/340001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/2378143004/diff/340001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.h:17: #include "webrtc/base/race_checker.h"
nit: order

[aleloi, 2016-10-20 12:22:18]

The CQ bit was checked by aleloi@webrtc.org

[aleloi, 2016-10-20 12:22:19]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2378143004/#ps370001 (title:
"Changed 'ssrc' into 'Ssrc' because of change upstream.")

[aleloi, 2016-10-20 12:22:23]

The CQ bit was unchecked by aleloi@webrtc.org

[aleloi, 2016-10-20 12:22:29]

The CQ bit was checked by aleloi@webrtc.org

[commit-bot: I haz the power, 2016-10-20 12:22:40]

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-10-20 13:32:41]

Description was changed from

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=webrtc:6346
==========

to

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=webrtc:6346
==========

[commit-bot: I haz the power, 2016-10-20 13:32:43]

Committed patchset #7 (id:370001)

[commit-bot: I haz the power, 2016-10-20 13:37:21]

Description was changed from

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=webrtc:6346
==========

to

==========
Made AudioReceiveStream a mixer participant.

Methods to facilitate this are added to ChannelProxy and voe::Channel.

BUG=webrtc:6346

Committed: https://crrev.com/aed581a4f3d31cac3bf5aa7284009ff129d5510c
Cr-Commit-Position: refs/heads/master@{#14707}
==========

[commit-bot: I haz the power, 2016-10-20 13:37:22]

Patchset 7 (id:??) landed as
https://crrev.com/aed581a4f3d31cac3bf5aa7284009ff129d5510c
Cr-Commit-Position: refs/heads/master@{#14707}