From 47136ddaea7fea55f9d54116f654a8bcf263a6e2 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Fri, 12 Jan 2018 10:49:35 -0800 Subject: [PATCH] Change RtpSenders to interact with the media channel directly Similar to the change for RtpReceivers, this removes the BaseChannel methods that would just proxy calls to the MediaChannel and instead gives the MediaChannel directly to the RtpSenders to make the calls directly. Bug: webrtc:8587 Change-Id: Ibab98d75ff1641e902281ad9e31ffdad36caff35 Reviewed-on: https://webrtc-review.googlesource.com/38983 Commit-Queue: Steve Anton Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#21608} --- ortc/ortcrtpsenderadapter.cc | 14 ++-- pc/channel.cc | 78 --------------------- pc/channel.h | 24 ------- pc/channel_unittest.cc | 115 ------------------------------- pc/peerconnection.cc | 25 ++++--- pc/rtpsender.cc | 107 ++++++++++++++++++---------- pc/rtpsender.h | 29 +++++--- pc/rtpsenderreceiver_unittest.cc | 39 ++++++----- pc/rtptransceiver.cc | 8 ++- 9 files changed, 141 insertions(+), 298 deletions(-) diff --git a/ortc/ortcrtpsenderadapter.cc b/ortc/ortcrtpsenderadapter.cc index 306db64a57..2b25d46eeb 100644 --- a/ortc/ortcrtpsenderadapter.cc +++ b/ortc/ortcrtpsenderadapter.cc @@ -156,14 +156,20 @@ OrtcRtpSenderAdapter::OrtcRtpSenderAdapter( void OrtcRtpSenderAdapter::CreateInternalSender() { switch (kind_) { case cricket::MEDIA_TYPE_AUDIO: { - auto* audio_sender = new AudioRtpSender(nullptr); - audio_sender->SetChannel(rtp_transport_controller_->voice_channel()); + auto* audio_sender = new AudioRtpSender( + rtp_transport_controller_->worker_thread(), nullptr); + auto* voice_channel = rtp_transport_controller_->voice_channel(); + RTC_DCHECK(voice_channel); + audio_sender->SetMediaChannel(voice_channel->media_channel()); internal_sender_ = audio_sender; break; } case cricket::MEDIA_TYPE_VIDEO: { - auto* video_sender = new VideoRtpSender(); - video_sender->SetChannel(rtp_transport_controller_->video_channel()); + auto* video_sender = + new VideoRtpSender(rtp_transport_controller_->worker_thread()); + auto* video_channel = rtp_transport_controller_->video_channel(); + RTC_DCHECK(video_channel); + video_sender->SetMediaChannel(video_channel->media_channel()); internal_sender_ = video_sender; break; } diff --git a/pc/channel.cc b/pc/channel.cc index 5c91c0b961..e1c103b39b 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -1212,42 +1212,6 @@ void VoiceChannel::SetEarlyMedia(bool enable) { } } -bool VoiceChannel::CanInsertDtmf() { - return InvokeOnWorker( - RTC_FROM_HERE, Bind(&VoiceMediaChannel::CanInsertDtmf, media_channel())); -} - -bool VoiceChannel::InsertDtmf(uint32_t ssrc, - int event_code, - int duration) { - return InvokeOnWorker( - RTC_FROM_HERE, - Bind(&VoiceChannel::InsertDtmf_w, this, ssrc, event_code, duration)); -} - -webrtc::RtpParameters VoiceChannel::GetRtpSendParameters(uint32_t ssrc) const { - return worker_thread()->Invoke( - RTC_FROM_HERE, Bind(&VoiceChannel::GetRtpSendParameters_w, this, ssrc)); -} - -webrtc::RtpParameters VoiceChannel::GetRtpSendParameters_w( - uint32_t ssrc) const { - return media_channel()->GetRtpSendParameters(ssrc); -} - -bool VoiceChannel::SetRtpSendParameters( - uint32_t ssrc, - const webrtc::RtpParameters& parameters) { - return InvokeOnWorker( - RTC_FROM_HERE, - Bind(&VoiceChannel::SetRtpSendParameters_w, this, ssrc, parameters)); -} - -bool VoiceChannel::SetRtpSendParameters_w(uint32_t ssrc, - webrtc::RtpParameters parameters) { - return media_channel()->SetRtpSendParameters(ssrc, parameters); -} - bool VoiceChannel::GetStats(VoiceMediaInfo* stats) { return InvokeOnWorker(RTC_FROM_HERE, Bind(&VoiceMediaChannel::GetStats, media_channel(), stats)); @@ -1441,15 +1405,6 @@ void VoiceChannel::HandleEarlyMediaTimeout() { } } -bool VoiceChannel::InsertDtmf_w(uint32_t ssrc, - int event, - int duration) { - if (!enabled()) { - return false; - } - return media_channel()->InsertDtmf(ssrc, event, duration); -} - void VoiceChannel::OnMessage(rtc::Message *pmsg) { switch (pmsg->message_id) { case MSG_EARLYMEDIATIMEOUT: @@ -1501,39 +1456,6 @@ VideoChannel::~VideoChannel() { Deinit(); } -bool VideoChannel::SetVideoSend( - uint32_t ssrc, - bool mute, - const VideoOptions* options, - rtc::VideoSourceInterface* source) { - return InvokeOnWorker( - RTC_FROM_HERE, Bind(&VideoMediaChannel::SetVideoSend, media_channel(), - ssrc, mute, options, source)); -} - -webrtc::RtpParameters VideoChannel::GetRtpSendParameters(uint32_t ssrc) const { - return worker_thread()->Invoke( - RTC_FROM_HERE, Bind(&VideoChannel::GetRtpSendParameters_w, this, ssrc)); -} - -webrtc::RtpParameters VideoChannel::GetRtpSendParameters_w( - uint32_t ssrc) const { - return media_channel()->GetRtpSendParameters(ssrc); -} - -bool VideoChannel::SetRtpSendParameters( - uint32_t ssrc, - const webrtc::RtpParameters& parameters) { - return InvokeOnWorker( - RTC_FROM_HERE, - Bind(&VideoChannel::SetRtpSendParameters_w, this, ssrc, parameters)); -} - -bool VideoChannel::SetRtpSendParameters_w(uint32_t ssrc, - webrtc::RtpParameters parameters) { - return media_channel()->SetRtpSendParameters(ssrc, parameters); -} - void VideoChannel::UpdateMediaSendRecvState_w() { // Send outgoing data if we're the active call, we have the remote content, // and we have had some form of connectivity. diff --git a/pc/channel.h b/pc/channel.h index ec9f9138ee..ca36dcdfe1 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -482,18 +482,6 @@ class VoiceChannel : public BaseChannel { // own ringing sound sigslot::signal1 SignalEarlyMediaTimeout; - // Returns if the telephone-event has been negotiated. - bool CanInsertDtmf(); - // Send and/or play a DTMF |event| according to the |flags|. - // The DTMF out-of-band signal will be used on sending. - // The |ssrc| should be either 0 or a valid send stream ssrc. - // The valid value for the |event| are 0 which corresponding to DTMF - // event 0-9, *, #, A-D. - bool InsertDtmf(uint32_t ssrc, int event_code, int duration); - webrtc::RtpParameters GetRtpSendParameters(uint32_t ssrc) const; - bool SetRtpSendParameters(uint32_t ssrc, - const webrtc::RtpParameters& parameters); - // Get statistics about the current media session. bool GetStats(VoiceMediaInfo* stats); @@ -530,7 +518,6 @@ class VoiceChannel : public BaseChannel { webrtc::SdpType type, std::string* error_desc) override; void HandleEarlyMediaTimeout(); - bool InsertDtmf_w(uint32_t ssrc, int event, int duration); void OnMessage(rtc::Message* pmsg) override; void OnConnectionMonitorUpdate( @@ -582,15 +569,6 @@ class VideoChannel : public BaseChannel { void StopMediaMonitor(); sigslot::signal2 SignalMediaMonitor; - // Register a source and set options. - // The |ssrc| must correspond to a registered send stream. - bool SetVideoSend(uint32_t ssrc, - bool enable, - const VideoOptions* options, - rtc::VideoSourceInterface* source); - webrtc::RtpParameters GetRtpSendParameters(uint32_t ssrc) const; - bool SetRtpSendParameters(uint32_t ssrc, - const webrtc::RtpParameters& parameters); cricket::MediaType media_type() override { return cricket::MEDIA_TYPE_VIDEO; } private: @@ -603,8 +581,6 @@ class VideoChannel : public BaseChannel { webrtc::SdpType type, std::string* error_desc) override; bool GetStats_w(VideoMediaInfo* stats); - webrtc::RtpParameters GetRtpSendParameters_w(uint32_t ssrc) const; - bool SetRtpSendParameters_w(uint32_t ssrc, webrtc::RtpParameters parameters); void OnConnectionMonitorUpdate( ConnectionMonitor* monitor, diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index ae5782c915..7e3b45993a 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -1885,25 +1885,6 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { rtc::nullopt); } - void CanChangeMaxBitrate() { - CreateChannels(0, 0); - EXPECT_TRUE(channel1_->SetLocalContent(&local_media_content1_, - SdpType::kOffer, NULL)); - - EXPECT_TRUE(channel1_->SetRtpSendParameters( - kSsrc1, BitrateLimitedParameters(1000))); - VerifyMaxBitrate(channel1_->GetRtpSendParameters(kSsrc1), 1000); - VerifyMaxBitrate(media_channel1_->GetRtpSendParameters(kSsrc1), 1000); - EXPECT_EQ(-1, media_channel1_->max_bps()); - - EXPECT_TRUE(channel1_->SetRtpSendParameters( - kSsrc1, BitrateLimitedParameters(rtc::nullopt))); - VerifyMaxBitrate(channel1_->GetRtpSendParameters(kSsrc1), rtc::nullopt); - VerifyMaxBitrate(media_channel1_->GetRtpSendParameters(kSsrc1), - rtc::nullopt); - EXPECT_EQ(-1, media_channel1_->max_bps()); - } - // Test that when a channel gets new transports with a call to // |SetTransports|, the socket options from the old transports are merged with // the options on the new transport. @@ -2347,26 +2328,6 @@ TEST_F(VoiceChannelSingleThreadTest, TestMediaMonitor) { Base::TestMediaMonitor(); } -// Test that InsertDtmf properly forwards to the media channel. -TEST_F(VoiceChannelSingleThreadTest, TestInsertDtmf) { - CreateChannels(0, 0); - EXPECT_TRUE(SendInitiate()); - EXPECT_TRUE(SendAccept()); - EXPECT_EQ(0U, media_channel1_->dtmf_info_queue().size()); - - EXPECT_TRUE(channel1_->InsertDtmf(1, 3, 100)); - EXPECT_TRUE(channel1_->InsertDtmf(2, 5, 110)); - EXPECT_TRUE(channel1_->InsertDtmf(3, 7, 120)); - - ASSERT_EQ(3U, media_channel1_->dtmf_info_queue().size()); - EXPECT_TRUE(CompareDtmfInfo(media_channel1_->dtmf_info_queue()[0], - 1, 3, 100)); - EXPECT_TRUE(CompareDtmfInfo(media_channel1_->dtmf_info_queue()[1], - 2, 5, 110)); - EXPECT_TRUE(CompareDtmfInfo(media_channel1_->dtmf_info_queue()[2], - 3, 7, 120)); -} - TEST_F(VoiceChannelSingleThreadTest, TestSetContentFailure) { Base::TestSetContentFailure(); } @@ -2419,10 +2380,6 @@ TEST_F(VoiceChannelSingleThreadTest, DefaultMaxBitrateIsUnlimited) { Base::DefaultMaxBitrateIsUnlimited(); } -TEST_F(VoiceChannelSingleThreadTest, CanChangeMaxBitrate) { - Base::CanChangeMaxBitrate(); -} - TEST_F(VoiceChannelSingleThreadTest, SocketOptionsMergedOnSetTransport) { Base::SocketOptionsMergedOnSetTransport(); } @@ -2636,26 +2593,6 @@ TEST_F(VoiceChannelDoubleThreadTest, TestMediaMonitor) { Base::TestMediaMonitor(); } -// Test that InsertDtmf properly forwards to the media channel. -TEST_F(VoiceChannelDoubleThreadTest, TestInsertDtmf) { - CreateChannels(0, 0); - EXPECT_TRUE(SendInitiate()); - EXPECT_TRUE(SendAccept()); - EXPECT_EQ(0U, media_channel1_->dtmf_info_queue().size()); - - EXPECT_TRUE(channel1_->InsertDtmf(1, 3, 100)); - EXPECT_TRUE(channel1_->InsertDtmf(2, 5, 110)); - EXPECT_TRUE(channel1_->InsertDtmf(3, 7, 120)); - - ASSERT_EQ(3U, media_channel1_->dtmf_info_queue().size()); - EXPECT_TRUE( - CompareDtmfInfo(media_channel1_->dtmf_info_queue()[0], 1, 3, 100)); - EXPECT_TRUE( - CompareDtmfInfo(media_channel1_->dtmf_info_queue()[1], 2, 5, 110)); - EXPECT_TRUE( - CompareDtmfInfo(media_channel1_->dtmf_info_queue()[2], 3, 7, 120)); -} - TEST_F(VoiceChannelDoubleThreadTest, TestSetContentFailure) { Base::TestSetContentFailure(); } @@ -2708,10 +2645,6 @@ TEST_F(VoiceChannelDoubleThreadTest, DefaultMaxBitrateIsUnlimited) { Base::DefaultMaxBitrateIsUnlimited(); } -TEST_F(VoiceChannelDoubleThreadTest, CanChangeMaxBitrate) { - Base::CanChangeMaxBitrate(); -} - TEST_F(VoiceChannelDoubleThreadTest, SocketOptionsMergedOnSetTransport) { Base::SocketOptionsMergedOnSetTransport(); } @@ -2753,26 +2686,6 @@ TEST_F(VideoChannelSingleThreadTest, TestPlayoutAndSendingStates) { Base::TestPlayoutAndSendingStates(); } -TEST_F(VideoChannelSingleThreadTest, TestMuteStream) { - CreateChannels(0, 0); - // Test that we can Mute the default channel even though the sending SSRC - // is unknown. - EXPECT_FALSE(media_channel1_->IsStreamMuted(0)); - EXPECT_TRUE(channel1_->SetVideoSend(0, false, nullptr, nullptr)); - EXPECT_TRUE(media_channel1_->IsStreamMuted(0)); - EXPECT_TRUE(channel1_->SetVideoSend(0, true, nullptr, nullptr)); - EXPECT_FALSE(media_channel1_->IsStreamMuted(0)); - // Test that we can not mute an unknown SSRC. - EXPECT_FALSE(channel1_->SetVideoSend(kSsrc1, false, nullptr, nullptr)); - SendInitiate(); - // After the local session description has been set, we can mute a stream - // with its SSRC. - EXPECT_TRUE(channel1_->SetVideoSend(kSsrc1, false, nullptr, nullptr)); - EXPECT_TRUE(media_channel1_->IsStreamMuted(kSsrc1)); - EXPECT_TRUE(channel1_->SetVideoSend(kSsrc1, true, nullptr, nullptr)); - EXPECT_FALSE(media_channel1_->IsStreamMuted(kSsrc1)); -} - TEST_F(VideoChannelSingleThreadTest, TestMediaContentDirection) { Base::TestMediaContentDirection(); } @@ -2931,10 +2844,6 @@ TEST_F(VideoChannelSingleThreadTest, DefaultMaxBitrateIsUnlimited) { Base::DefaultMaxBitrateIsUnlimited(); } -TEST_F(VideoChannelSingleThreadTest, CanChangeMaxBitrate) { - Base::CanChangeMaxBitrate(); -} - TEST_F(VideoChannelSingleThreadTest, SocketOptionsMergedOnSetTransport) { Base::SocketOptionsMergedOnSetTransport(); } @@ -2976,26 +2885,6 @@ TEST_F(VideoChannelDoubleThreadTest, TestPlayoutAndSendingStates) { Base::TestPlayoutAndSendingStates(); } -TEST_F(VideoChannelDoubleThreadTest, TestMuteStream) { - CreateChannels(0, 0); - // Test that we can Mute the default channel even though the sending SSRC - // is unknown. - EXPECT_FALSE(media_channel1_->IsStreamMuted(0)); - EXPECT_TRUE(channel1_->SetVideoSend(0, false, nullptr, nullptr)); - EXPECT_TRUE(media_channel1_->IsStreamMuted(0)); - EXPECT_TRUE(channel1_->SetVideoSend(0, true, nullptr, nullptr)); - EXPECT_FALSE(media_channel1_->IsStreamMuted(0)); - // Test that we can not mute an unknown SSRC. - EXPECT_FALSE(channel1_->SetVideoSend(kSsrc1, false, nullptr, nullptr)); - SendInitiate(); - // After the local session description has been set, we can mute a stream - // with its SSRC. - EXPECT_TRUE(channel1_->SetVideoSend(kSsrc1, false, nullptr, nullptr)); - EXPECT_TRUE(media_channel1_->IsStreamMuted(kSsrc1)); - EXPECT_TRUE(channel1_->SetVideoSend(kSsrc1, true, nullptr, nullptr)); - EXPECT_FALSE(media_channel1_->IsStreamMuted(kSsrc1)); -} - TEST_F(VideoChannelDoubleThreadTest, TestMediaContentDirection) { Base::TestMediaContentDirection(); } @@ -3154,10 +3043,6 @@ TEST_F(VideoChannelDoubleThreadTest, DefaultMaxBitrateIsUnlimited) { Base::DefaultMaxBitrateIsUnlimited(); } -TEST_F(VideoChannelDoubleThreadTest, CanChangeMaxBitrate) { - Base::CanChangeMaxBitrate(); -} - TEST_F(VideoChannelDoubleThreadTest, SocketOptionsMergedOnSetTransport) { Base::SocketOptionsMergedOnSetTransport(); } diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 0888acced2..6281d57e3d 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1073,7 +1073,7 @@ PeerConnection::AddTrackPlanB( auto new_sender = CreateSender(media_type, track, stream_labels); if (track->kind() == MediaStreamTrackInterface::kAudioKind) { static_cast(new_sender->internal()) - ->SetChannel(voice_channel()); + ->SetMediaChannel(voice_media_channel()); GetAudioTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_audio_sender_infos_, @@ -1084,7 +1084,7 @@ PeerConnection::AddTrackPlanB( } else { RTC_DCHECK_EQ(MediaStreamTrackInterface::kVideoKind, track->kind()); static_cast(new_sender->internal()) - ->SetChannel(video_channel()); + ->SetMediaChannel(video_media_channel()); GetVideoTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_video_sender_infos_, @@ -1289,7 +1289,8 @@ PeerConnection::CreateSender( (track->kind() == MediaStreamTrackInterface::kAudioKind)); sender = RtpSenderProxyWithInternal::Create( signaling_thread(), - new AudioRtpSender(static_cast(track.get()), + new AudioRtpSender(worker_thread(), + static_cast(track.get()), stream_labels, stats_.get())); } else { RTC_DCHECK_EQ(media_type, cricket::MEDIA_TYPE_VIDEO); @@ -1297,7 +1298,8 @@ PeerConnection::CreateSender( (track->kind() == MediaStreamTrackInterface::kVideoKind)); sender = RtpSenderProxyWithInternal::Create( signaling_thread(), - new VideoRtpSender(static_cast(track.get()), + new VideoRtpSender(worker_thread(), + static_cast(track.get()), stream_labels)); } sender->internal()->set_stream_ids(stream_labels); @@ -1368,15 +1370,16 @@ rtc::scoped_refptr PeerConnection::CreateSender( // TODO(steveanton): Move construction of the RtpSenders to RtpTransceiver. rtc::scoped_refptr> new_sender; if (kind == MediaStreamTrackInterface::kAudioKind) { - auto* audio_sender = - new AudioRtpSender(nullptr, stream_labels, stats_.get()); - audio_sender->SetChannel(voice_channel()); + auto* audio_sender = new AudioRtpSender(worker_thread(), nullptr, + stream_labels, stats_.get()); + audio_sender->SetMediaChannel(voice_media_channel()); new_sender = RtpSenderProxyWithInternal::Create( signaling_thread(), audio_sender); GetAudioTransceiver()->internal()->AddSender(new_sender); } else if (kind == MediaStreamTrackInterface::kVideoKind) { - auto* video_sender = new VideoRtpSender(nullptr, stream_labels); - video_sender->SetChannel(video_channel()); + auto* video_sender = + new VideoRtpSender(worker_thread(), nullptr, stream_labels); + video_sender->SetMediaChannel(video_media_channel()); new_sender = RtpSenderProxyWithInternal::Create( signaling_thread(), video_sender); GetVideoTransceiver()->internal()->AddSender(new_sender); @@ -2828,7 +2831,7 @@ void PeerConnection::AddAudioTrack(AudioTrackInterface* track, auto new_sender = CreateSender(cricket::MEDIA_TYPE_AUDIO, track, {stream->label()}); static_cast(new_sender->internal()) - ->SetChannel(voice_channel()); + ->SetMediaChannel(voice_media_channel()); GetAudioTransceiver()->internal()->AddSender(new_sender); // If the sender has already been configured in SDP, we call SetSsrc, // which will connect the sender to the underlying transport. This can @@ -2872,7 +2875,7 @@ void PeerConnection::AddVideoTrack(VideoTrackInterface* track, auto new_sender = CreateSender(cricket::MEDIA_TYPE_VIDEO, track, {stream->label()}); static_cast(new_sender->internal()) - ->SetChannel(video_channel()); + ->SetMediaChannel(video_media_channel()); GetVideoTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_video_sender_infos_, stream->label(), track->id()); diff --git a/pc/rtpsender.cc b/pc/rtpsender.cc index cad86f2f77..7c85764279 100644 --- a/pc/rtpsender.cc +++ b/pc/rtpsender.cc @@ -57,13 +57,17 @@ void LocalAudioSinkAdapter::SetSink(cricket::AudioSource::Sink* sink) { sink_ = sink; } -AudioRtpSender::AudioRtpSender(StatsCollector* stats) - : AudioRtpSender(nullptr, {rtc::CreateRandomUuid()}, stats) {} +AudioRtpSender::AudioRtpSender(rtc::Thread* worker_thread, + StatsCollector* stats) + : AudioRtpSender(worker_thread, nullptr, {rtc::CreateRandomUuid()}, stats) { +} -AudioRtpSender::AudioRtpSender(rtc::scoped_refptr track, +AudioRtpSender::AudioRtpSender(rtc::Thread* worker_thread, + rtc::scoped_refptr track, const std::vector& stream_labels, StatsCollector* stats) - : id_(track ? track->id() : rtc::CreateRandomUuid()), + : worker_thread_(worker_thread), + id_(track ? track->id() : rtc::CreateRandomUuid()), stream_ids_(stream_labels), stats_(stats), track_(track), @@ -73,6 +77,7 @@ AudioRtpSender::AudioRtpSender(rtc::scoped_refptr track, cached_track_enabled_(track ? track->enabled() : false), sink_adapter_(new LocalAudioSinkAdapter()), attachment_id_(track ? GenerateUniqueId() : 0) { + RTC_DCHECK(worker_thread); // TODO(bugs.webrtc.org/7932): Remove once zero or multiple streams are // supported. RTC_DCHECK_EQ(stream_labels.size(), 1u); @@ -89,7 +94,7 @@ AudioRtpSender::~AudioRtpSender() { } bool AudioRtpSender::CanInsertDtmf() { - if (!channel_) { + if (!media_channel_) { RTC_LOG(LS_ERROR) << "CanInsertDtmf: No audio channel exists."; return false; } @@ -99,11 +104,12 @@ bool AudioRtpSender::CanInsertDtmf() { RTC_LOG(LS_ERROR) << "CanInsertDtmf: Sender does not have SSRC."; return false; } - return channel_->CanInsertDtmf(); + return worker_thread_->Invoke( + RTC_FROM_HERE, [&] { return media_channel_->CanInsertDtmf(); }); } bool AudioRtpSender::InsertDtmf(int code, int duration) { - if (!channel_) { + if (!media_channel_) { RTC_LOG(LS_ERROR) << "CanInsertDtmf: No audio channel exists."; return false; } @@ -111,11 +117,13 @@ bool AudioRtpSender::InsertDtmf(int code, int duration) { RTC_LOG(LS_ERROR) << "CanInsertDtmf: Sender does not have SSRC."; return false; } - if (!channel_->InsertDtmf(ssrc_, code, duration)) { + bool success = worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->InsertDtmf(ssrc_, code, duration); + }); + if (!success) { RTC_LOG(LS_ERROR) << "Failed to insert DTMF to channel."; - return false; } - return true; + return success; } sigslot::signal0<>* AudioRtpSender::GetOnDestroyedSignal() { @@ -182,18 +190,22 @@ bool AudioRtpSender::SetTrack(MediaStreamTrackInterface* track) { } RtpParameters AudioRtpSender::GetParameters() const { - if (!channel_ || stopped_) { + if (!media_channel_ || stopped_) { return RtpParameters(); } - return channel_->GetRtpSendParameters(ssrc_); + return worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->GetRtpSendParameters(ssrc_); + }); } bool AudioRtpSender::SetParameters(const RtpParameters& parameters) { TRACE_EVENT0("webrtc", "AudioRtpSender::SetParameters"); - if (!channel_ || stopped_) { + if (!media_channel_ || stopped_) { return false; } - return channel_->SetRtpSendParameters(ssrc_, parameters); + return worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->SetRtpSendParameters(ssrc_, parameters); + }); } rtc::scoped_refptr AudioRtpSender::GetDtmfSender() const { @@ -243,7 +255,7 @@ void AudioRtpSender::Stop() { void AudioRtpSender::SetAudioSend() { RTC_DCHECK(!stopped_); RTC_DCHECK(can_send_track()); - if (!channel_) { + if (!media_channel_) { RTC_LOG(LS_ERROR) << "SetAudioSend: No audio channel exists."; return; } @@ -260,9 +272,14 @@ void AudioRtpSender::SetAudioSend() { } #endif - cricket::AudioSource* source = sink_adapter_.get(); - RTC_DCHECK(source != nullptr); - if (!channel_->SetAudioSend(ssrc_, track_->enabled(), &options, source)) { + // |track_->enabled()| hops to the signaling thread, so call it before we hop + // to the worker thread or else it will deadlock. + bool track_enabled = track_->enabled(); + bool success = worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->SetAudioSend(ssrc_, track_enabled, &options, + sink_adapter_.get()); + }); + if (!success) { RTC_LOG(LS_ERROR) << "SetAudioSend: ssrc is incorrect: " << ssrc_; } } @@ -270,29 +287,35 @@ void AudioRtpSender::SetAudioSend() { void AudioRtpSender::ClearAudioSend() { RTC_DCHECK(ssrc_ != 0); RTC_DCHECK(!stopped_); - if (!channel_) { + if (!media_channel_) { RTC_LOG(LS_WARNING) << "ClearAudioSend: No audio channel exists."; return; } cricket::AudioOptions options; - if (!channel_->SetAudioSend(ssrc_, false, &options, nullptr)) { + bool success = worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->SetAudioSend(ssrc_, false, &options, nullptr); + }); + if (!success) { RTC_LOG(LS_WARNING) << "ClearAudioSend: ssrc is incorrect: " << ssrc_; } } -VideoRtpSender::VideoRtpSender() - : VideoRtpSender(nullptr, {rtc::CreateRandomUuid()}) {} +VideoRtpSender::VideoRtpSender(rtc::Thread* worker_thread) + : VideoRtpSender(worker_thread, nullptr, {rtc::CreateRandomUuid()}) {} -VideoRtpSender::VideoRtpSender(rtc::scoped_refptr track, +VideoRtpSender::VideoRtpSender(rtc::Thread* worker_thread, + rtc::scoped_refptr track, const std::vector& stream_labels) - : id_(track ? track->id() : rtc::CreateRandomUuid()), + : worker_thread_(worker_thread), + id_(track ? track->id() : rtc::CreateRandomUuid()), stream_ids_(stream_labels), track_(track), cached_track_enabled_(track ? track->enabled() : false), - cached_track_content_hint_(track - ? track->content_hint() - : VideoTrackInterface::ContentHint::kNone), + cached_track_content_hint_( + track ? track->content_hint() + : VideoTrackInterface::ContentHint::kNone), attachment_id_(track ? GenerateUniqueId() : 0) { + RTC_DCHECK(worker_thread); // TODO(bugs.webrtc.org/7932): Remove once zero or multiple streams are // supported. RTC_DCHECK_EQ(stream_labels.size(), 1u); @@ -359,18 +382,22 @@ bool VideoRtpSender::SetTrack(MediaStreamTrackInterface* track) { } RtpParameters VideoRtpSender::GetParameters() const { - if (!channel_ || stopped_) { + if (!media_channel_ || stopped_) { return RtpParameters(); } - return channel_->GetRtpSendParameters(ssrc_); + return worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->GetRtpSendParameters(ssrc_); + }); } bool VideoRtpSender::SetParameters(const RtpParameters& parameters) { TRACE_EVENT0("webrtc", "VideoRtpSender::SetParameters"); - if (!channel_ || stopped_) { + if (!media_channel_ || stopped_) { return false; } - return channel_->SetRtpSendParameters(ssrc_, parameters); + return worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->SetRtpSendParameters(ssrc_, parameters); + }); } rtc::scoped_refptr VideoRtpSender::GetDtmfSender() const { @@ -411,7 +438,7 @@ void VideoRtpSender::Stop() { void VideoRtpSender::SetVideoSend() { RTC_DCHECK(!stopped_); RTC_DCHECK(can_send_track()); - if (!channel_) { + if (!media_channel_) { RTC_LOG(LS_ERROR) << "SetVideoSend: No video channel exists."; return; } @@ -431,22 +458,28 @@ void VideoRtpSender::SetVideoSend() { options.is_screencast = true; break; } - if (!channel_->SetVideoSend(ssrc_, track_->enabled(), &options, track_)) { - RTC_NOTREACHED(); - } + // |track_->enabled()| hops to the signaling thread, so call it before we hop + // to the worker thread or else it will deadlock. + bool track_enabled = track_->enabled(); + bool success = worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->SetVideoSend(ssrc_, track_enabled, &options, track_); + }); + RTC_DCHECK(success); } void VideoRtpSender::ClearVideoSend() { RTC_DCHECK(ssrc_ != 0); RTC_DCHECK(!stopped_); - if (!channel_) { + if (!media_channel_) { RTC_LOG(LS_WARNING) << "SetVideoSend: No video channel exists."; return; } // Allow SetVideoSend to fail since |enable| is false and |source| is null. // This the normal case when the underlying media channel has already been // deleted. - channel_->SetVideoSend(ssrc_, false, nullptr, nullptr); + worker_thread_->Invoke(RTC_FROM_HERE, [&] { + return media_channel_->SetVideoSend(ssrc_, false, nullptr, nullptr); + }); } } // namespace webrtc diff --git a/pc/rtpsender.h b/pc/rtpsender.h index eb09d0b13f..119d91f11a 100644 --- a/pc/rtpsender.h +++ b/pc/rtpsender.h @@ -26,7 +26,6 @@ // Adding 'nogncheck' to disable the gn include headers check to support modular // WebRTC build targets. #include "media/base/audiosource.h" // nogncheck -#include "pc/channel.h" #include "pc/dtmfsender.h" #include "pc/statscollector.h" @@ -85,11 +84,12 @@ class AudioRtpSender : public DtmfProviderInterface, // Construct an AudioRtpSender with a null track, a single, randomly generated // stream label, and a randomly generated ID. - explicit AudioRtpSender(StatsCollector* stats); + AudioRtpSender(rtc::Thread* worker_thread, StatsCollector* stats); // Construct an AudioRtpSender with the given track and stream labels. The // sender ID will be set to the track's ID. - AudioRtpSender(rtc::scoped_refptr track, + AudioRtpSender(rtc::Thread* worker_thread, + rtc::scoped_refptr track, const std::vector& stream_labels, StatsCollector* stats); @@ -140,8 +140,10 @@ class AudioRtpSender : public DtmfProviderInterface, int AttachmentId() const override { return attachment_id_; } // Does not take ownership. - // Should call SetChannel(nullptr) before |channel| is destroyed. - void SetChannel(cricket::VoiceChannel* channel) { channel_ = channel; } + // Should call SetMediaChannel(nullptr) before |media_channel| is destroyed. + void SetMediaChannel(cricket::VoiceMediaChannel* media_channel) { + media_channel_ = media_channel; + } private: // TODO(nisse): Since SSRC == 0 is technically valid, figure out @@ -155,11 +157,12 @@ class AudioRtpSender : public DtmfProviderInterface, sigslot::signal0<> SignalDestroyed; + rtc::Thread* const worker_thread_; const std::string id_; // TODO(steveanton): Until more Unified Plan work is done, this can only have // exactly one element. std::vector stream_ids_; - cricket::VoiceChannel* channel_ = nullptr; + cricket::VoiceMediaChannel* media_channel_ = nullptr; StatsCollector* stats_; rtc::scoped_refptr track_; rtc::scoped_refptr dtmf_sender_proxy_; @@ -178,11 +181,12 @@ class VideoRtpSender : public ObserverInterface, public: // Construct a VideoRtpSender with a null track, a single, randomly generated // stream label, and a randomly generated ID. - VideoRtpSender(); + explicit VideoRtpSender(rtc::Thread* worker_thread); // Construct a VideoRtpSender with the given track and stream labels. The // sender ID will be set to the track's ID. - VideoRtpSender(rtc::scoped_refptr track, + VideoRtpSender(rtc::Thread* worker_thread, + rtc::scoped_refptr track, const std::vector& stream_labels); virtual ~VideoRtpSender(); @@ -226,8 +230,10 @@ class VideoRtpSender : public ObserverInterface, int AttachmentId() const override { return attachment_id_; } // Does not take ownership. - // Should call SetChannel(nullptr) before |channel| is destroyed. - void SetChannel(cricket::VideoChannel* channel) { channel_ = channel; } + // Should call SetMediaChannel(nullptr) before |media_channel| is destroyed. + void SetMediaChannel(cricket::VideoMediaChannel* media_channel) { + media_channel_ = media_channel; + } private: bool can_send_track() const { return track_ && ssrc_; } @@ -237,11 +243,12 @@ class VideoRtpSender : public ObserverInterface, // Helper function to call SetVideoSend with "stop sending" parameters. void ClearVideoSend(); + rtc::Thread* worker_thread_; const std::string id_; // TODO(steveanton): Until more Unified Plan work is done, this can only have // exactly one element. std::vector stream_ids_; - cricket::VideoChannel* channel_ = nullptr; + cricket::VideoMediaChannel* media_channel_ = nullptr; rtc::scoped_refptr track_; uint32_t ssrc_ = 0; bool cached_track_enabled_ = false; diff --git a/pc/rtpsenderreceiver_unittest.cc b/pc/rtpsenderreceiver_unittest.cc index 9ccbe95026..9da9c6dc01 100644 --- a/pc/rtpsenderreceiver_unittest.cc +++ b/pc/rtpsenderreceiver_unittest.cc @@ -55,13 +55,15 @@ class RtpSenderReceiverTest : public testing::Test, public sigslot::has_slots<> { public: RtpSenderReceiverTest() - : // Create fake media engine/etc. so we can create channels to use to - // test RtpSenders/RtpReceivers. + : network_thread_(rtc::Thread::Current()), + worker_thread_(rtc::Thread::Current()), + // Create fake media engine/etc. so we can create channels to use to + // test RtpSenders/RtpReceivers. media_engine_(new cricket::FakeMediaEngine()), channel_manager_(rtc::WrapUnique(media_engine_), rtc::MakeUnique(), - rtc::Thread::Current(), - rtc::Thread::Current()), + worker_thread_, + network_thread_), fake_call_(Call::Config(&event_log_)), local_stream_(MediaStream::Create(kStreamLabel1)) { // Create channels to be used by the RtpSenders and RtpReceivers. @@ -134,9 +136,10 @@ class RtpSenderReceiverTest : public testing::Test, const rtc::scoped_refptr& source) { audio_track_ = AudioTrack::Create(kAudioTrackId, source); EXPECT_TRUE(local_stream_->AddTrack(audio_track_)); - audio_rtp_sender_ = new AudioRtpSender(local_stream_->GetAudioTracks()[0], - {local_stream_->label()}, nullptr); - audio_rtp_sender_->SetChannel(voice_channel_); + audio_rtp_sender_ = + new AudioRtpSender(worker_thread_, local_stream_->GetAudioTracks()[0], + {local_stream_->label()}, nullptr); + audio_rtp_sender_->SetMediaChannel(voice_media_channel_); audio_rtp_sender_->SetSsrc(kAudioSsrc); audio_rtp_sender_->GetOnDestroyedSignal()->connect( this, &RtpSenderReceiverTest::OnAudioSenderDestroyed); @@ -144,8 +147,8 @@ class RtpSenderReceiverTest : public testing::Test, } void CreateAudioRtpSenderWithNoTrack() { - audio_rtp_sender_ = new AudioRtpSender(nullptr); - audio_rtp_sender_->SetChannel(voice_channel_); + audio_rtp_sender_ = new AudioRtpSender(worker_thread_, nullptr); + audio_rtp_sender_->SetMediaChannel(voice_media_channel_); } void OnAudioSenderDestroyed() { audio_sender_destroyed_signal_fired_ = true; } @@ -154,16 +157,17 @@ class RtpSenderReceiverTest : public testing::Test, void CreateVideoRtpSender(bool is_screencast) { AddVideoTrack(is_screencast); - video_rtp_sender_ = new VideoRtpSender(local_stream_->GetVideoTracks()[0], - {local_stream_->label()}); - video_rtp_sender_->SetChannel(video_channel_); + video_rtp_sender_ = + new VideoRtpSender(worker_thread_, local_stream_->GetVideoTracks()[0], + {local_stream_->label()}); + video_rtp_sender_->SetMediaChannel(video_media_channel_); video_rtp_sender_->SetSsrc(kVideoSsrc); VerifyVideoChannelInput(); } void CreateVideoRtpSenderWithNoTrack() { - video_rtp_sender_ = new VideoRtpSender(); - video_rtp_sender_->SetChannel(video_channel_); + video_rtp_sender_ = new VideoRtpSender(worker_thread_); + video_rtp_sender_->SetMediaChannel(video_media_channel_); } void DestroyAudioRtpSender() { @@ -259,6 +263,8 @@ class RtpSenderReceiverTest : public testing::Test, } protected: + rtc::Thread* const network_thread_; + rtc::Thread* const worker_thread_; webrtc::RtcEventLogNullImpl event_log_; // |media_engine_| is actually owned by |channel_manager_|. cricket::FakeMediaEngine* media_engine_; @@ -772,9 +778,10 @@ TEST_F(RtpSenderReceiverTest, // Setting detailed overrides the default non-screencast mode. This should be // applied even if the track is set on construction. video_track_->set_content_hint(VideoTrackInterface::ContentHint::kDetailed); - video_rtp_sender_ = new VideoRtpSender(local_stream_->GetVideoTracks()[0], + video_rtp_sender_ = new VideoRtpSender(worker_thread_, + local_stream_->GetVideoTracks()[0], {local_stream_->label()}); - video_rtp_sender_->SetChannel(video_channel_); + video_rtp_sender_->SetMediaChannel(video_media_channel_); video_track_->set_enabled(true); // Sender is not ready to send (no SSRC) so no option should have been set. diff --git a/pc/rtptransceiver.cc b/pc/rtptransceiver.cc index 0dfc8b52a3..ecb8ec879f 100644 --- a/pc/rtptransceiver.cc +++ b/pc/rtptransceiver.cc @@ -60,11 +60,15 @@ void RtpTransceiver::SetChannel(cricket::BaseChannel* channel) { for (auto sender : senders_) { if (media_type() == cricket::MEDIA_TYPE_AUDIO) { + auto* voice_channel = static_cast(channel); static_cast(sender->internal()) - ->SetChannel(static_cast(channel)); + ->SetMediaChannel(voice_channel ? voice_channel->media_channel() + : nullptr); } else { + auto* video_channel = static_cast(channel); static_cast(sender->internal()) - ->SetChannel(static_cast(channel)); + ->SetMediaChannel(video_channel ? video_channel->media_channel() + : nullptr); } }