From dd9390c4910e0cf4d0f223fdaf9f38961a74d4e1 Mon Sep 17 00:00:00 2001 From: Amit Hilbuch Date: Tue, 13 Nov 2018 16:26:05 -0800 Subject: [PATCH] Prevent channels being set on stopped transceiver. Fixing bug that allows a channel to be set on a stopped transceiver. This CL contains the following refactoring: 1. Extracted ChannelInterface from BaseChannel 2. Unified SetXxxMediaChannel (Voice, Video) into SetMediaChannel Bug: webrtc:9932 Change-Id: I2fbf00c823b7848ad4f2acb6e80b1b58ac45ee38 Reviewed-on: https://webrtc-review.googlesource.com/c/110564 Reviewed-by: Seth Hampson Reviewed-by: Steve Anton Commit-Queue: Amit Hilbuch Cr-Commit-Position: refs/heads/master@{#25641} --- media/base/mediachannel.cc | 12 +++++ media/base/mediachannel.h | 6 +++ pc/BUILD.gn | 3 ++ pc/channel.cc | 2 +- pc/channel.h | 42 +++++++++++------- pc/channelinterface.h | 68 ++++++++++++++++++++++++++++ pc/peerconnection.cc | 41 ++++++++--------- pc/peerconnection.h | 8 ++-- pc/peerconnection_ice_unittest.cc | 7 ++- pc/rtcstatscollector.cc | 2 +- pc/rtpreceiver.cc | 14 +++--- pc/rtpreceiver.h | 27 +++--------- pc/rtpsender.cc | 14 +++--- pc/rtpsender.h | 28 +++--------- pc/rtpsenderreceiver_unittest.cc | 20 ++++----- pc/rtptransceiver.cc | 40 +++++++---------- pc/rtptransceiver.h | 8 ++-- pc/rtptransceiver_unittest.cc | 71 ++++++++++++++++++++++++++++++ pc/test/mock_channelinterface.h | 47 ++++++++++++++++++++ pc/test/mock_rtpreceiverinternal.h | 3 +- pc/test/mock_rtpsenderinternal.h | 3 +- 21 files changed, 326 insertions(+), 140 deletions(-) create mode 100644 pc/channelinterface.h create mode 100644 pc/rtptransceiver_unittest.cc create mode 100644 pc/test/mock_channelinterface.h diff --git a/media/base/mediachannel.cc b/media/base/mediachannel.cc index a5dadd240e..0634b2e575 100644 --- a/media/base/mediachannel.cc +++ b/media/base/mediachannel.cc @@ -87,6 +87,10 @@ std::map AudioSendParameters::ToStringMap() const { return params; } +cricket::MediaType VoiceMediaChannel::media_type() const { + return cricket::MediaType::MEDIA_TYPE_AUDIO; +} + VideoSendParameters::VideoSendParameters() = default; VideoSendParameters::~VideoSendParameters() = default; @@ -96,11 +100,19 @@ std::map VideoSendParameters::ToStringMap() const { return params; } +cricket::MediaType VideoMediaChannel::media_type() const { + return cricket::MediaType::MEDIA_TYPE_VIDEO; +} + DataMediaChannel::DataMediaChannel() = default; DataMediaChannel::DataMediaChannel(const MediaConfig& config) : MediaChannel(config) {} DataMediaChannel::~DataMediaChannel() = default; +cricket::MediaType DataMediaChannel::media_type() const { + return cricket::MediaType::MEDIA_TYPE_DATA; +} + bool DataMediaChannel::GetStats(DataMediaInfo* info) { return true; } diff --git a/media/base/mediachannel.h b/media/base/mediachannel.h index 69c7b00cf0..108ae26864 100644 --- a/media/base/mediachannel.h +++ b/media/base/mediachannel.h @@ -184,6 +184,8 @@ class MediaChannel : public sigslot::has_slots<> { MediaChannel(); ~MediaChannel() override; + virtual cricket::MediaType media_type() const = 0; + // Sets the abstract interface class for sending RTP/RTCP data and // interface for media transport (experimental). If media transport is // provided, it should be used instead of RTP/RTCP. @@ -700,6 +702,8 @@ class VoiceMediaChannel : public MediaChannel { explicit VoiceMediaChannel(const MediaConfig& config) : MediaChannel(config) {} ~VoiceMediaChannel() override {} + + cricket::MediaType media_type() const override; virtual bool SetSendParameters(const AudioSendParameters& params) = 0; virtual bool SetRecvParameters(const AudioRecvParameters& params) = 0; virtual webrtc::RtpParameters GetRtpSendParameters(uint32_t ssrc) const = 0; @@ -772,6 +776,7 @@ class VideoMediaChannel : public MediaChannel { : MediaChannel(config) {} ~VideoMediaChannel() override {} + cricket::MediaType media_type() const override; virtual bool SetSendParameters(const VideoSendParameters& params) = 0; virtual bool SetRecvParameters(const VideoRecvParameters& params) = 0; virtual webrtc::RtpParameters GetRtpSendParameters(uint32_t ssrc) const = 0; @@ -883,6 +888,7 @@ class DataMediaChannel : public MediaChannel { explicit DataMediaChannel(const MediaConfig& config); ~DataMediaChannel() override; + cricket::MediaType media_type() const override; virtual bool SetSendParameters(const DataSendParameters& params) = 0; virtual bool SetRecvParameters(const DataRecvParameters& params) = 0; diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 97045ba66d..4be6052732 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -31,6 +31,7 @@ rtc_static_library("rtc_pc_base") { sources = [ "channel.cc", "channel.h", + "channelinterface.h", "channelmanager.cc", "channelmanager.h", "dtlssrtptransport.cc", @@ -380,6 +381,7 @@ if (rtc_include_tests) { "test/fakevideotrackrenderer.h", "test/fakevideotracksource.h", "test/framegeneratorcapturervideotracksource.h", + "test/mock_channelinterface.h", "test/mock_datachannel.h", "test/mock_rtpreceiverinternal.h", "test/mock_rtpsenderinternal.h", @@ -460,6 +462,7 @@ if (rtc_include_tests) { "rtpmediautils_unittest.cc", "rtpparametersconversion_unittest.cc", "rtpsenderreceiver_unittest.cc", + "rtptransceiver_unittest.cc", "sctputils_unittest.cc", "statscollector_unittest.cc", "test/fakeaudiocapturemodule_unittest.cc", diff --git a/pc/channel.cc b/pc/channel.cc index 50fa5673ec..0f857cdd3b 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -702,7 +702,7 @@ void BaseChannel::OnMessage(rtc::Message* pmsg) { break; } case MSG_FIRSTPACKETRECEIVED: { - SignalFirstPacketReceived(this); + SignalFirstPacketReceived_(this); break; } } diff --git a/pc/channel.h b/pc/channel.h index 65d64809dc..5644bb457d 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -29,6 +29,7 @@ #include "media/base/streamparams.h" #include "p2p/base/dtlstransportinternal.h" #include "p2p/base/packettransportinternal.h" +#include "pc/channelinterface.h" #include "pc/dtlssrtptransport.h" #include "pc/mediasession.h" #include "pc/rtptransport.h" @@ -48,7 +49,6 @@ class MediaTransportInterface; namespace cricket { struct CryptoParams; -class MediaContentDescription; // BaseChannel contains logic common to voice and video, including enable, // marshaling calls to a worker and network threads, and connection and media @@ -68,7 +68,8 @@ class MediaContentDescription; // vtable, and the media channel's thread using BaseChannel as the // NetworkInterface. -class BaseChannel : public rtc::MessageHandler, +class BaseChannel : public ChannelInterface, + public rtc::MessageHandler, public sigslot::has_slots<>, public MediaChannel::NetworkInterface, public webrtc::RtpPacketSinkInterface { @@ -94,10 +95,10 @@ class BaseChannel : public rtc::MessageHandler, rtc::Thread* worker_thread() const { return worker_thread_; } rtc::Thread* network_thread() const { return network_thread_; } - const std::string& content_name() const { return content_name_; } + const std::string& content_name() const override { return content_name_; } // TODO(deadbeef): This is redundant; remove this. - const std::string& transport_name() const { return transport_name_; } - bool enabled() const { return enabled_; } + const std::string& transport_name() const override { return transport_name_; } + bool enabled() const override { return enabled_; } // This function returns true if using SRTP (DTLS-based keying or SDES). bool srtp_active() const { @@ -110,17 +111,17 @@ class BaseChannel : public rtc::MessageHandler, // encryption, an SrtpTransport for SDES or a DtlsSrtpTransport for DTLS-SRTP. // This can be called from any thread and it hops to the network thread // internally. It would replace the |SetTransports| and its variants. - bool SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport); + bool SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) override; // Channel control bool SetLocalContent(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc); + std::string* error_desc) override; bool SetRemoteContent(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc); + std::string* error_desc) override; - bool Enable(bool enable); + bool Enable(bool enable) override; // TODO(zhihuang): These methods are used for testing and can be removed. bool AddRecvStream(const StreamParams& sp); @@ -140,7 +141,9 @@ class BaseChannel : public rtc::MessageHandler, void SignalDtlsSrtpSetupFailure_s(bool rtcp); // Used for latency measurements. - sigslot::signal1 SignalFirstPacketReceived; + sigslot::signal1& SignalFirstPacketReceived() override { + return SignalFirstPacketReceived_; + } // Forward SignalSentPacket to worker thread. sigslot::signal1 SignalSentPacket; @@ -176,8 +179,6 @@ class BaseChannel : public rtc::MessageHandler, int SetOption(SocketType type, rtc::Socket::Option o, int val) override; int SetOption_n(SocketType type, rtc::Socket::Option o, int val); - virtual cricket::MediaType media_type() = 0; - // RtpPacketSinkInterface overrides. void OnRtpPacket(const webrtc::RtpPacketReceived& packet) override; @@ -187,9 +188,9 @@ class BaseChannel : public rtc::MessageHandler, transport_name_ = transport_name; } - protected: - virtual MediaChannel* media_channel() const { return media_channel_.get(); } + MediaChannel* media_channel() const override { return media_channel_.get(); } + protected: bool was_ever_writable() const { return was_ever_writable_; } void set_local_content_direction(webrtc::RtpTransceiverDirection direction) { local_content_direction_ = direction; @@ -306,6 +307,7 @@ class BaseChannel : public rtc::MessageHandler, rtc::Thread* const network_thread_; rtc::Thread* const signaling_thread_; rtc::AsyncInvoker invoker_; + sigslot::signal1 SignalFirstPacketReceived_; const std::string content_name_; @@ -363,7 +365,9 @@ class VoiceChannel : public BaseChannel { return static_cast(BaseChannel::media_channel()); } - cricket::MediaType media_type() override { return cricket::MEDIA_TYPE_AUDIO; } + cricket::MediaType media_type() const override { + return cricket::MEDIA_TYPE_AUDIO; + } private: // overrides from BaseChannel @@ -402,7 +406,9 @@ class VideoChannel : public BaseChannel { void FillBitrateInfo(BandwidthEstimationInfo* bwe_info); - cricket::MediaType media_type() override { return cricket::MEDIA_TYPE_VIDEO; } + cricket::MediaType media_type() const override { + return cricket::MEDIA_TYPE_VIDEO; + } private: // overrides from BaseChannel @@ -454,7 +460,9 @@ class RtpDataChannel : public BaseChannel { // That occurs when the channel is enabled, the transport is writable, // both local and remote descriptions are set, and the channel is unblocked. sigslot::signal1 SignalReadyToSendData; - cricket::MediaType media_type() override { return cricket::MEDIA_TYPE_DATA; } + cricket::MediaType media_type() const override { + return cricket::MEDIA_TYPE_DATA; + } protected: // downcasts a MediaChannel. diff --git a/pc/channelinterface.h b/pc/channelinterface.h new file mode 100644 index 0000000000..8e4109a045 --- /dev/null +++ b/pc/channelinterface.h @@ -0,0 +1,68 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef PC_CHANNELINTERFACE_H_ +#define PC_CHANNELINTERFACE_H_ + +#include + +#include "api/jsep.h" +#include "api/mediatypes.h" +#include "media/base/mediachannel.h" +#include "pc/rtptransportinternal.h" + +namespace cricket { + +class MediaContentDescription; + +// ChannelInterface contains methods common to voice, video and data channels. +// As more methods are added to BaseChannel, they should be included in the +// interface as well. +class ChannelInterface { + public: + virtual cricket::MediaType media_type() const = 0; + + virtual MediaChannel* media_channel() const = 0; + + // TODO(deadbeef): This is redundant; remove this. + virtual const std::string& transport_name() const = 0; + + virtual const std::string& content_name() const = 0; + + virtual bool enabled() const = 0; + + // Enables or disables this channel + virtual bool Enable(bool enable) = 0; + + // Used for latency measurements. + virtual sigslot::signal1& SignalFirstPacketReceived() = 0; + + // Channel control + virtual bool SetLocalContent(const MediaContentDescription* content, + webrtc::SdpType type, + std::string* error_desc) = 0; + virtual bool SetRemoteContent(const MediaContentDescription* content, + webrtc::SdpType type, + std::string* error_desc) = 0; + + // Set an RTP level transport. + // Some examples: + // * An RtpTransport without encryption. + // * An SrtpTransport for SDES. + // * A DtlsSrtpTransport for DTLS-SRTP. + virtual bool SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) = 0; + + protected: + virtual ~ChannelInterface() = default; +}; + +} // namespace cricket + +#endif // PC_CHANNELINTERFACE_H_ diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 3fa5ca5ab8..47ecb4e1c1 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1253,7 +1253,7 @@ PeerConnection::AddTrackPlanB( auto new_sender = CreateSender(media_type, track->id(), track, adjusted_stream_ids, {}); if (track->kind() == MediaStreamTrackInterface::kAudioKind) { - new_sender->internal()->SetVoiceMediaChannel(voice_media_channel()); + new_sender->internal()->SetMediaChannel(voice_media_channel()); GetAudioTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_audio_sender_infos_, @@ -1263,7 +1263,7 @@ PeerConnection::AddTrackPlanB( } } else { RTC_DCHECK_EQ(MediaStreamTrackInterface::kVideoKind, track->kind()); - new_sender->internal()->SetVideoMediaChannel(video_media_channel()); + new_sender->internal()->SetMediaChannel(video_media_channel()); GetVideoTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_video_sender_infos_, @@ -1593,14 +1593,14 @@ rtc::scoped_refptr PeerConnection::CreateSender( if (kind == MediaStreamTrackInterface::kAudioKind) { auto* audio_sender = new AudioRtpSender( worker_thread(), rtc::CreateRandomUuid(), stats_.get()); - audio_sender->SetVoiceMediaChannel(voice_media_channel()); + 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(worker_thread(), rtc::CreateRandomUuid()); - video_sender->SetVideoMediaChannel(video_media_channel()); + video_sender->SetMediaChannel(video_media_channel()); new_sender = RtpSenderProxyWithInternal::Create( signaling_thread(), video_sender); GetVideoTransceiver()->internal()->AddSender(new_sender); @@ -2736,11 +2736,11 @@ RTCError PeerConnection::UpdateTransceiverChannel( const cricket::ContentGroup* bundle_group) { RTC_DCHECK(IsUnifiedPlan()); RTC_DCHECK(transceiver); - cricket::BaseChannel* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (content.rejected) { if (channel) { transceiver->internal()->SetChannel(nullptr); - DestroyBaseChannel(channel); + DestroyChannelInterface(channel); } } else { if (!channel) { @@ -3482,7 +3482,7 @@ void PeerConnection::CreateAudioReceiver( // the constructor taking stream IDs instead. auto* audio_receiver = new AudioRtpReceiver( worker_thread(), remote_sender_info.sender_id, streams); - audio_receiver->SetVoiceMediaChannel(voice_media_channel()); + audio_receiver->SetMediaChannel(voice_media_channel()); audio_receiver->SetupMediaChannel(remote_sender_info.first_ssrc); auto receiver = RtpReceiverProxyWithInternal::Create( signaling_thread(), audio_receiver); @@ -3500,7 +3500,7 @@ void PeerConnection::CreateVideoReceiver( // the constructor taking stream IDs instead. auto* video_receiver = new VideoRtpReceiver( worker_thread(), remote_sender_info.sender_id, streams); - video_receiver->SetVideoMediaChannel(video_media_channel()); + video_receiver->SetMediaChannel(video_media_channel()); video_receiver->SetupMediaChannel(remote_sender_info.first_ssrc); auto receiver = RtpReceiverProxyWithInternal::Create( signaling_thread(), video_receiver); @@ -3543,7 +3543,7 @@ void PeerConnection::AddAudioTrack(AudioTrackInterface* track, // Normal case; we've never seen this track before. auto new_sender = CreateSender(cricket::MEDIA_TYPE_AUDIO, track->id(), track, {stream->id()}, {}); - new_sender->internal()->SetVoiceMediaChannel(voice_media_channel()); + new_sender->internal()->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 @@ -3588,7 +3588,7 @@ void PeerConnection::AddVideoTrack(VideoTrackInterface* track, // Normal case; we've never seen this track before. auto new_sender = CreateSender(cricket::MEDIA_TYPE_VIDEO, track->id(), track, {stream->id()}, {}); - new_sender->internal()->SetVideoMediaChannel(video_media_channel()); + new_sender->internal()->SetMediaChannel(video_media_channel()); GetVideoTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_video_sender_infos_, stream->id(), track->id()); @@ -4966,10 +4966,10 @@ void PeerConnection::StopRtcEventLog_w() { } } -cricket::BaseChannel* PeerConnection::GetChannel( +cricket::ChannelInterface* PeerConnection::GetChannel( const std::string& content_name) { for (auto transceiver : transceivers_) { - cricket::BaseChannel* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (channel && channel->content_name() == content_name) { return channel; } @@ -5086,7 +5086,7 @@ RTCError PeerConnection::PushdownMediaDescription( for (auto transceiver : transceivers_) { const ContentInfo* content_info = FindMediaSectionForTransceiver(transceiver, sdesc); - cricket::BaseChannel* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (!channel || !content_info || content_info->rejected) { continue; } @@ -5421,7 +5421,7 @@ std::map PeerConnection::GetTransportNamesByMid() const { std::map transport_names_by_mid; for (auto transceiver : transceivers_) { - cricket::BaseChannel* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (channel) { transport_names_by_mid[channel->content_name()] = channel->transport_name(); @@ -5598,7 +5598,7 @@ void PeerConnection::OnTransportControllerDtlsHandshakeError( void PeerConnection::EnableSending() { for (auto transceiver : transceivers_) { - cricket::BaseChannel* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (channel && !channel->enabled()) { channel->Enable(true); } @@ -6484,7 +6484,7 @@ void PeerConnection::OnSentPacket_w(const rtc::SentPacket& sent_packet) { const std::string PeerConnection::GetTransportName( const std::string& content_name) { - cricket::BaseChannel* channel = GetChannel(content_name); + cricket::ChannelInterface* channel = GetChannel(content_name); if (channel) { return channel->transport_name(); } @@ -6503,17 +6503,17 @@ void PeerConnection::DestroyTransceiverChannel( transceiver) { RTC_DCHECK(transceiver); - cricket::BaseChannel* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (channel) { transceiver->internal()->SetChannel(nullptr); - DestroyBaseChannel(channel); + DestroyChannelInterface(channel); } } void PeerConnection::DestroyDataChannel() { if (rtp_data_channel_) { OnDataChannelDestroyed(); - DestroyBaseChannel(rtp_data_channel_); + DestroyChannelInterface(rtp_data_channel_); rtp_data_channel_ = nullptr; } @@ -6538,7 +6538,8 @@ void PeerConnection::DestroyDataChannel() { } } -void PeerConnection::DestroyBaseChannel(cricket::BaseChannel* channel) { +void PeerConnection::DestroyChannelInterface( + cricket::ChannelInterface* channel) { RTC_DCHECK(channel); switch (channel->media_type()) { case cricket::MEDIA_TYPE_AUDIO: diff --git a/pc/peerconnection.h b/pc/peerconnection.h index fe4d777401..b5ae9d2ae4 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -719,7 +719,7 @@ class PeerConnection : public PeerConnectionInternal, SessionError session_error() const { return session_error_; } const std::string& session_error_desc() const { return session_error_desc_; } - cricket::BaseChannel* GetChannel(const std::string& content_name); + cricket::ChannelInterface* GetChannel(const std::string& content_name); // Get current SSL role used by SCTP's underlying transport. bool GetSctpSslRole(rtc::SSLRole* role); @@ -922,9 +922,9 @@ class PeerConnection : public PeerConnectionInternal, // Destroys the RTP data channel and/or the SCTP data channel and clears it. void DestroyDataChannel(); - // Destroys the given BaseChannel. The channel cannot be accessed after this - // method is called. - void DestroyBaseChannel(cricket::BaseChannel* channel); + // Destroys the given ChannelInterface. + // The channel cannot be accessed after this method is called. + void DestroyChannelInterface(cricket::ChannelInterface* channel); // JsepTransportController::Observer override. // diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index 8fde8751fd..e015abffb8 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -212,7 +212,12 @@ class PeerConnectionIceBaseTest : public ::testing::Test { PeerConnection* pc = static_cast(pc_proxy->internal()); for (auto transceiver : pc->GetTransceiversInternal()) { if (transceiver->media_type() == cricket::MEDIA_TYPE_AUDIO) { - cricket::BaseChannel* channel = transceiver->internal()->channel(); + // TODO(amithi): This test seems to be using a method that should not + // be public |rtp_packet_transport|. Because the test is not mocking + // the channels or transceiver, workaround will be to |static_cast| + // the channel until the method is rewritten. + cricket::BaseChannel* channel = static_cast( + transceiver->internal()->channel()); if (channel) { auto dtls_transport = static_cast( channel->rtp_packet_transport()); diff --git a/pc/rtcstatscollector.cc b/pc/rtcstatscollector.cc index 32e0b65557..871cff9726 100644 --- a/pc/rtcstatscollector.cc +++ b/pc/rtcstatscollector.cc @@ -1426,7 +1426,7 @@ RTCStatsCollector::PrepareTransceiverStatsInfos_s() const { stats.transceiver = transceiver->internal(); stats.media_type = media_type; - cricket::BaseChannel* channel = transceiver->internal()->channel(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (!channel) { // The remaining fields require a BaseChannel. continue; diff --git a/pc/rtpreceiver.cc b/pc/rtpreceiver.cc index acea930be3..1916a73670 100644 --- a/pc/rtpreceiver.cc +++ b/pc/rtpreceiver.cc @@ -268,9 +268,10 @@ void AudioRtpReceiver::SetObserver(RtpReceiverObserverInterface* observer) { } } -void AudioRtpReceiver::SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) { - media_channel_ = voice_media_channel; +void AudioRtpReceiver::SetMediaChannel(cricket::MediaChannel* media_channel) { + RTC_DCHECK(media_channel == nullptr || + media_channel->media_type() == media_type()); + media_channel_ = static_cast(media_channel); } void AudioRtpReceiver::NotifyFirstPacketReceived() { @@ -443,9 +444,10 @@ void VideoRtpReceiver::SetObserver(RtpReceiverObserverInterface* observer) { } } -void VideoRtpReceiver::SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) { - media_channel_ = video_media_channel; +void VideoRtpReceiver::SetMediaChannel(cricket::MediaChannel* media_channel) { + RTC_DCHECK(media_channel == nullptr || + media_channel->media_type() == media_type()); + media_channel_ = static_cast(media_channel); } void VideoRtpReceiver::NotifyFirstPacketReceived() { diff --git a/pc/rtpreceiver.h b/pc/rtpreceiver.h index 85be8c258b..06331a66c4 100644 --- a/pc/rtpreceiver.h +++ b/pc/rtpreceiver.h @@ -34,14 +34,10 @@ class RtpReceiverInternal : public RtpReceiverInterface { virtual void Stop() = 0; // Sets the underlying MediaEngine channel associated with this RtpSender. - // SetVoiceMediaChannel should be used for audio RtpSenders and - // SetVideoMediaChannel should be used for video RtpSenders. Must call the - // appropriate SetXxxMediaChannel(nullptr) before the media channel is - // destroyed. - virtual void SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) = 0; - virtual void SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) = 0; + // A VoiceMediaChannel should be used for audio RtpSenders and + // a VideoMediaChannel should be used for video RtpSenders. + // Must call SetMediaChannel(nullptr) before the media channel is destroyed. + virtual void SetMediaChannel(cricket::MediaChannel* media_channel) = 0; // Configures the RtpReceiver with the underlying media channel, with the // given SSRC as the stream identifier. If |ssrc| is 0, the receiver will @@ -130,13 +126,8 @@ class AudioRtpReceiver : public ObserverInterface, void SetStreams(const std::vector>& streams) override; void SetObserver(RtpReceiverObserverInterface* observer) override; - void SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) override; - void SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) override { - RTC_NOTREACHED(); - } + void SetMediaChannel(cricket::MediaChannel* media_channel) override; std::vector GetSources() const override; int AttachmentId() const override { return attachment_id_; } @@ -217,13 +208,7 @@ class VideoRtpReceiver : public rtc::RefCountedObject { void SetObserver(RtpReceiverObserverInterface* observer) override; - void SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) override { - RTC_NOTREACHED(); - } - - void SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) override; + void SetMediaChannel(cricket::MediaChannel* media_channel) override; int AttachmentId() const override { return attachment_id_; } diff --git a/pc/rtpsender.cc b/pc/rtpsender.cc index abd1748cea..76fdca6e94 100644 --- a/pc/rtpsender.cc +++ b/pc/rtpsender.cc @@ -386,9 +386,10 @@ void AudioRtpSender::Stop() { stopped_ = true; } -void AudioRtpSender::SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) { - media_channel_ = voice_media_channel; +void AudioRtpSender::SetMediaChannel(cricket::MediaChannel* media_channel) { + RTC_DCHECK(media_channel == nullptr || + media_channel->media_type() == media_type()); + media_channel_ = static_cast(media_channel); } void AudioRtpSender::SetAudioSend() { @@ -629,9 +630,10 @@ void VideoRtpSender::Stop() { stopped_ = true; } -void VideoRtpSender::SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) { - media_channel_ = video_media_channel; +void VideoRtpSender::SetMediaChannel(cricket::MediaChannel* media_channel) { + RTC_DCHECK(media_channel == nullptr || + media_channel->media_type() == media_type()); + media_channel_ = static_cast(media_channel); } void VideoRtpSender::SetVideoSend() { diff --git a/pc/rtpsender.h b/pc/rtpsender.h index 2a8289f417..07fae6b954 100644 --- a/pc/rtpsender.h +++ b/pc/rtpsender.h @@ -36,14 +36,10 @@ bool UnimplementedRtpParameterHasValue(const RtpParameters& parameters); class RtpSenderInternal : public RtpSenderInterface { public: // Sets the underlying MediaEngine channel associated with this RtpSender. - // SetVoiceMediaChannel should be used for audio RtpSenders and - // SetVideoMediaChannel should be used for video RtpSenders. Must call the - // appropriate SetXxxMediaChannel(nullptr) before the media channel is - // destroyed. - virtual void SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) = 0; - virtual void SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) = 0; + // A VoiceMediaChannel should be used for audio RtpSenders and + // a VideoMediaChannel should be used for video RtpSenders. + // Must call SetMediaChannel(nullptr) before the media channel is destroyed. + virtual void SetMediaChannel(cricket::MediaChannel* media_channel) = 0; // Used to set the SSRC of the sender, once a local description has been set. // If |ssrc| is 0, this indiates that the sender should disconnect from the @@ -156,13 +152,7 @@ class AudioRtpSender : public DtmfProviderInterface, int AttachmentId() const override { return attachment_id_; } - void SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) override; - - void SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) override { - RTC_NOTREACHED(); - } + void SetMediaChannel(cricket::MediaChannel* media_channel) override; private: // TODO(nisse): Since SSRC == 0 is technically valid, figure out @@ -253,13 +243,7 @@ class VideoRtpSender : public ObserverInterface, void Stop() override; int AttachmentId() const override { return attachment_id_; } - void SetVoiceMediaChannel( - cricket::VoiceMediaChannel* voice_media_channel) override { - RTC_NOTREACHED(); - } - - void SetVideoMediaChannel( - cricket::VideoMediaChannel* video_media_channel) override; + void SetMediaChannel(cricket::MediaChannel* media_channel) override; private: bool can_send_track() const { return track_ && ssrc_; } diff --git a/pc/rtpsenderreceiver_unittest.cc b/pc/rtpsenderreceiver_unittest.cc index fa42615454..b5a513bd6a 100644 --- a/pc/rtpsenderreceiver_unittest.cc +++ b/pc/rtpsenderreceiver_unittest.cc @@ -154,7 +154,7 @@ class RtpSenderReceiverTest : public testing::Test, new AudioRtpSender(worker_thread_, audio_track_->id(), nullptr); ASSERT_TRUE(audio_rtp_sender_->SetTrack(audio_track_)); audio_rtp_sender_->set_stream_ids({local_stream_->id()}); - audio_rtp_sender_->SetVoiceMediaChannel(voice_media_channel_); + audio_rtp_sender_->SetMediaChannel(voice_media_channel_); audio_rtp_sender_->SetSsrc(kAudioSsrc); audio_rtp_sender_->GetOnDestroyedSignal()->connect( this, &RtpSenderReceiverTest::OnAudioSenderDestroyed); @@ -163,7 +163,7 @@ class RtpSenderReceiverTest : public testing::Test, void CreateAudioRtpSenderWithNoTrack() { audio_rtp_sender_ = new AudioRtpSender(worker_thread_, /*id=*/"", nullptr); - audio_rtp_sender_->SetVoiceMediaChannel(voice_media_channel_); + audio_rtp_sender_->SetMediaChannel(voice_media_channel_); } void OnAudioSenderDestroyed() { audio_sender_destroyed_signal_fired_ = true; } @@ -191,13 +191,13 @@ class RtpSenderReceiverTest : public testing::Test, video_rtp_sender_ = new VideoRtpSender(worker_thread_, video_track_->id()); ASSERT_TRUE(video_rtp_sender_->SetTrack(video_track_)); video_rtp_sender_->set_stream_ids({local_stream_->id()}); - video_rtp_sender_->SetVideoMediaChannel(video_media_channel_); + video_rtp_sender_->SetMediaChannel(video_media_channel_); video_rtp_sender_->SetSsrc(ssrc); VerifyVideoChannelInput(ssrc); } void CreateVideoRtpSenderWithNoTrack() { video_rtp_sender_ = new VideoRtpSender(worker_thread_, /*id=*/""); - video_rtp_sender_->SetVideoMediaChannel(video_media_channel_); + video_rtp_sender_->SetMediaChannel(video_media_channel_); } void DestroyAudioRtpSender() { @@ -214,7 +214,7 @@ class RtpSenderReceiverTest : public testing::Test, std::vector> streams = {}) { audio_rtp_receiver_ = new AudioRtpReceiver( rtc::Thread::Current(), kAudioTrackId, std::move(streams)); - audio_rtp_receiver_->SetVoiceMediaChannel(voice_media_channel_); + audio_rtp_receiver_->SetMediaChannel(voice_media_channel_); audio_rtp_receiver_->SetupMediaChannel(kAudioSsrc); audio_track_ = audio_rtp_receiver_->audio_track(); VerifyVoiceChannelOutput(); @@ -224,7 +224,7 @@ class RtpSenderReceiverTest : public testing::Test, std::vector> streams = {}) { video_rtp_receiver_ = new VideoRtpReceiver( rtc::Thread::Current(), kVideoTrackId, std::move(streams)); - video_rtp_receiver_->SetVideoMediaChannel(video_media_channel_); + video_rtp_receiver_->SetMediaChannel(video_media_channel_); video_rtp_receiver_->SetupMediaChannel(kVideoSsrc); video_track_ = video_rtp_receiver_->video_track(); VerifyVideoChannelOutput(); @@ -664,7 +664,7 @@ TEST_F(RtpSenderReceiverTest, AudioSenderInitParametersMovedAfterNegotiation) { cricket::StreamParams stream_params = cricket::CreateSimStreamParams("cname", ssrcs); voice_media_channel_->AddSendStream(stream_params); - audio_rtp_sender_->SetVoiceMediaChannel(voice_media_channel_); + audio_rtp_sender_->SetMediaChannel(voice_media_channel_); audio_rtp_sender_->SetSsrc(1); params = audio_rtp_sender_->GetParameters(); @@ -903,7 +903,7 @@ TEST_F(RtpSenderReceiverTest, VideoSenderInitParametersMovedAfterNegotiation) { cricket::StreamParams stream_params = cricket::CreateSimStreamParams("cname", ssrcs); video_media_channel_->AddSendStream(stream_params); - video_rtp_sender_->SetVideoMediaChannel(video_media_channel_); + video_rtp_sender_->SetMediaChannel(video_media_channel_); video_rtp_sender_->SetSsrc(kVideoSsrcSimulcast); params = video_rtp_sender_->GetParameters(); @@ -938,7 +938,7 @@ TEST_F(RtpSenderReceiverTest, cricket::StreamParams stream_params = cricket::CreateSimStreamParams("cname", ssrcs); video_media_channel_->AddSendStream(stream_params); - video_rtp_sender_->SetVideoMediaChannel(video_media_channel_); + video_rtp_sender_->SetMediaChannel(video_media_channel_); video_rtp_sender_->SetSsrc(kVideoSsrcSimulcast); params = video_rtp_sender_->GetParameters(); @@ -1332,7 +1332,7 @@ TEST_F(RtpSenderReceiverTest, video_rtp_sender_ = new VideoRtpSender(worker_thread_, video_track_->id()); ASSERT_TRUE(video_rtp_sender_->SetTrack(video_track_)); video_rtp_sender_->set_stream_ids({local_stream_->id()}); - video_rtp_sender_->SetVideoMediaChannel(video_media_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 0fe8ea6f50..8b56b8b4f1 100644 --- a/pc/rtptransceiver.cc +++ b/pc/rtptransceiver.cc @@ -38,52 +38,45 @@ RtpTransceiver::~RtpTransceiver() { Stop(); } -void RtpTransceiver::SetChannel(cricket::BaseChannel* channel) { +void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { + // Cannot set a non-null channel on a stopped transceiver. + if (stopped_ && channel) { + return; + } + if (channel) { RTC_DCHECK_EQ(media_type(), channel->media_type()); } if (channel_) { - channel_->SignalFirstPacketReceived.disconnect(this); + channel_->SignalFirstPacketReceived().disconnect(this); } channel_ = channel; if (channel_) { - channel_->SignalFirstPacketReceived.connect( + channel_->SignalFirstPacketReceived().connect( this, &RtpTransceiver::OnFirstPacketReceived); } for (auto sender : senders_) { - if (media_type() == cricket::MEDIA_TYPE_AUDIO) { - auto* voice_channel = static_cast(channel); - sender->internal()->SetVoiceMediaChannel( - voice_channel ? voice_channel->media_channel() : nullptr); - } else { - auto* video_channel = static_cast(channel); - sender->internal()->SetVideoMediaChannel( - video_channel ? video_channel->media_channel() : nullptr); - } + sender->internal()->SetMediaChannel(channel_ ? channel_->media_channel() + : nullptr); } for (auto receiver : receivers_) { - if (!channel) { + if (!channel_) { receiver->internal()->Stop(); } - if (media_type() == cricket::MEDIA_TYPE_AUDIO) { - auto* voice_channel = static_cast(channel); - receiver->internal()->SetVoiceMediaChannel( - voice_channel ? voice_channel->media_channel() : nullptr); - } else { - auto* video_channel = static_cast(channel); - receiver->internal()->SetVideoMediaChannel( - video_channel ? video_channel->media_channel() : nullptr); - } + + receiver->internal()->SetMediaChannel(channel_ ? channel_->media_channel() + : nullptr); } } void RtpTransceiver::AddSender( rtc::scoped_refptr> sender) { + RTC_DCHECK(!stopped_); RTC_DCHECK(!unified_plan_); RTC_DCHECK(sender); RTC_DCHECK_EQ(media_type(), sender->media_type()); @@ -109,6 +102,7 @@ bool RtpTransceiver::RemoveSender(RtpSenderInterface* sender) { void RtpTransceiver::AddReceiver( rtc::scoped_refptr> receiver) { + RTC_DCHECK(!stopped_); RTC_DCHECK(!unified_plan_); RTC_DCHECK(receiver); RTC_DCHECK_EQ(media_type(), receiver->media_type()); @@ -152,7 +146,7 @@ absl::optional RtpTransceiver::mid() const { return mid_; } -void RtpTransceiver::OnFirstPacketReceived(cricket::BaseChannel* channel) { +void RtpTransceiver::OnFirstPacketReceived(cricket::ChannelInterface*) { for (auto receiver : receivers_) { receiver->internal()->NotifyFirstPacketReceived(); } diff --git a/pc/rtptransceiver.h b/pc/rtptransceiver.h index 3e0d433782..07db196edd 100644 --- a/pc/rtptransceiver.h +++ b/pc/rtptransceiver.h @@ -70,11 +70,11 @@ class RtpTransceiver final // Returns the Voice/VideoChannel set for this transceiver. May be null if // the transceiver is not in the currently set local/remote description. - cricket::BaseChannel* channel() const { return channel_; } + cricket::ChannelInterface* channel() const { return channel_; } // Sets the Voice/VideoChannel. The caller must pass in the correct channel // implementation based on the type of the transceiver. - void SetChannel(cricket::BaseChannel* channel); + void SetChannel(cricket::ChannelInterface* channel); // Adds an RtpSender of the appropriate type to be owned by this transceiver. // Must not be null. @@ -177,7 +177,7 @@ class RtpTransceiver final void SetCodecPreferences(rtc::ArrayView codecs) override; private: - void OnFirstPacketReceived(cricket::BaseChannel* channel); + void OnFirstPacketReceived(cricket::ChannelInterface* channel); const bool unified_plan_; const cricket::MediaType media_type_; @@ -196,7 +196,7 @@ class RtpTransceiver final bool created_by_addtrack_ = false; bool has_ever_been_used_to_send_ = false; - cricket::BaseChannel* channel_ = nullptr; + cricket::ChannelInterface* channel_ = nullptr; }; BEGIN_SIGNALING_PROXY_MAP(RtpTransceiver) diff --git a/pc/rtptransceiver_unittest.cc b/pc/rtptransceiver_unittest.cc new file mode 100644 index 0000000000..b57d2123d1 --- /dev/null +++ b/pc/rtptransceiver_unittest.cc @@ -0,0 +1,71 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +// This file contains tests for |RtpTransceiver|. + +#include "pc/rtptransceiver.h" +#include "rtc_base/gunit.h" +#include "test/gmock.h" +#include "test/mock_channelinterface.h" + +using ::testing::Return; +using ::testing::ReturnRef; + +namespace webrtc { + +// Checks that a channel cannot be set on a stopped |RtpTransceiver|. +TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { + RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_AUDIO); + cricket::MockChannelInterface channel1; + sigslot::signal1 signal; + EXPECT_CALL(channel1, media_type()) + .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); + EXPECT_CALL(channel1, SignalFirstPacketReceived()) + .WillRepeatedly(ReturnRef(signal)); + + transceiver.SetChannel(&channel1); + EXPECT_EQ(&channel1, transceiver.channel()); + + // Stop the transceiver. + transceiver.Stop(); + EXPECT_EQ(&channel1, transceiver.channel()); + + cricket::MockChannelInterface channel2; + EXPECT_CALL(channel2, media_type()) + .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); + + // Channel can no longer be set, so this call should be a no-op. + transceiver.SetChannel(&channel2); + EXPECT_EQ(&channel1, transceiver.channel()); +} + +// Checks that a channel can be unset on a stopped |RtpTransceiver| +TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { + RtpTransceiver transceiver(cricket::MediaType::MEDIA_TYPE_VIDEO); + cricket::MockChannelInterface channel; + sigslot::signal1 signal; + EXPECT_CALL(channel, media_type()) + .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_VIDEO)); + EXPECT_CALL(channel, SignalFirstPacketReceived()) + .WillRepeatedly(ReturnRef(signal)); + + transceiver.SetChannel(&channel); + EXPECT_EQ(&channel, transceiver.channel()); + + // Stop the transceiver. + transceiver.Stop(); + EXPECT_EQ(&channel, transceiver.channel()); + + // Set the channel to |nullptr|. + transceiver.SetChannel(nullptr); + EXPECT_EQ(nullptr, transceiver.channel()); +} + +} // namespace webrtc diff --git a/pc/test/mock_channelinterface.h b/pc/test/mock_channelinterface.h new file mode 100644 index 0000000000..4c32a8fb8d --- /dev/null +++ b/pc/test/mock_channelinterface.h @@ -0,0 +1,47 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef PC_TEST_MOCK_CHANNELINTERFACE_H_ +#define PC_TEST_MOCK_CHANNELINTERFACE_H_ + +#include + +#include "pc/channelinterface.h" +#include "test/gmock.h" + +namespace cricket { + +// Mock class for BaseChannel. +// Use this class in unit tests to avoid dependecy on a specific +// implementation of BaseChannel. +class MockChannelInterface : public cricket::ChannelInterface { + public: + MOCK_CONST_METHOD0(media_type, cricket::MediaType()); + MOCK_CONST_METHOD0(media_channel, MediaChannel*()); + MOCK_CONST_METHOD0(transport_name, const std::string&()); + MOCK_CONST_METHOD0(content_name, const std::string&()); + MOCK_CONST_METHOD0(enabled, bool()); + MOCK_METHOD1(Enable, bool(bool)); + MOCK_METHOD0(SignalFirstPacketReceived, + sigslot::signal1&()); + MOCK_METHOD3(SetLocalContent, + bool(const cricket::MediaContentDescription*, + webrtc::SdpType, + std::string*)); + MOCK_METHOD3(SetRemoteContent, + bool(const cricket::MediaContentDescription*, + webrtc::SdpType, + std::string*)); + MOCK_METHOD1(SetRtpTransport, bool(webrtc::RtpTransportInternal*)); +}; + +} // namespace cricket + +#endif // PC_TEST_MOCK_CHANNELINTERFACE_H_ diff --git a/pc/test/mock_rtpreceiverinternal.h b/pc/test/mock_rtpreceiverinternal.h index 850d5a9f8e..84b0d71766 100644 --- a/pc/test/mock_rtpreceiverinternal.h +++ b/pc/test/mock_rtpreceiverinternal.h @@ -41,8 +41,7 @@ class MockRtpReceiverInternal : public RtpReceiverInternal { // RtpReceiverInternal methods. MOCK_METHOD0(Stop, void()); - MOCK_METHOD1(SetVoiceMediaChannel, void(cricket::VoiceMediaChannel*)); - MOCK_METHOD1(SetVideoMediaChannel, void(cricket::VideoMediaChannel*)); + MOCK_METHOD1(SetMediaChannel, void(cricket::MediaChannel*)); MOCK_METHOD1(SetupMediaChannel, void(uint32_t)); MOCK_CONST_METHOD0(ssrc, uint32_t()); MOCK_METHOD0(NotifyFirstPacketReceived, void()); diff --git a/pc/test/mock_rtpsenderinternal.h b/pc/test/mock_rtpsenderinternal.h index 5e14f7ad7d..dd4f69497d 100644 --- a/pc/test/mock_rtpsenderinternal.h +++ b/pc/test/mock_rtpsenderinternal.h @@ -39,8 +39,7 @@ class MockRtpSenderInternal : public RtpSenderInternal { rtc::scoped_refptr()); // RtpSenderInternal methods. - MOCK_METHOD1(SetVoiceMediaChannel, void(cricket::VoiceMediaChannel*)); - MOCK_METHOD1(SetVideoMediaChannel, void(cricket::VideoMediaChannel*)); + MOCK_METHOD1(SetMediaChannel, void(cricket::MediaChannel*)); MOCK_METHOD1(SetSsrc, void(uint32_t)); MOCK_METHOD1(set_stream_ids, void(const std::vector&)); MOCK_METHOD1(set_init_send_encodings,