From ff27da5ca1e74f4e16f5e1a5b5e9e8b8e85fa140 Mon Sep 17 00:00:00 2001 From: Saurav Das Date: Fri, 20 Sep 2019 11:05:30 -0700 Subject: [PATCH] Add/remove receive streams with SSRC 0 from media channels This enables creation and removal of receive streams with SSRC 0. Several related methods, for example SetOutputVolume, still use 0 as a special value. Bug: webrtc:8694 Change-Id: I341e6bd6c981c9838997510d8d712ad2948f6460 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/152780 Reviewed-by: Niels Moller Reviewed-by: Steve Anton Commit-Queue: Saurav Das Cr-Commit-Position: refs/heads/master@{#29398} --- media/base/fake_media_engine.h | 2 ++ media/base/media_channel.h | 2 ++ media/base/rtp_data_engine.cc | 3 +++ media/base/rtp_data_engine.h | 1 + media/engine/webrtc_video_engine.cc | 13 ++++++------ media/engine/webrtc_video_engine.h | 1 + media/engine/webrtc_video_engine_unittest.cc | 4 ++-- media/engine/webrtc_voice_engine.cc | 17 ++++++--------- media/engine/webrtc_voice_engine.h | 1 + media/engine/webrtc_voice_engine_unittest.cc | 8 +++---- pc/channel.cc | 22 ++++++++++++++++---- pc/channel.h | 1 + 12 files changed, 47 insertions(+), 28 deletions(-) diff --git a/media/base/fake_media_engine.h b/media/base/fake_media_engine.h index 28af02a5fd..ac303e6b5a 100644 --- a/media/base/fake_media_engine.h +++ b/media/base/fake_media_engine.h @@ -117,6 +117,8 @@ class RtpHelper : public Base { } return RemoveStreamBySsrc(&send_streams_, ssrc); } + virtual void ResetUnsignaledRecvStream() {} + virtual bool AddRecvStream(const StreamParams& sp) { if (absl::c_linear_search(receive_streams_, sp)) { return false; diff --git a/media/base/media_channel.h b/media/base/media_channel.h index d6dfe7025c..8f6b04b512 100644 --- a/media/base/media_channel.h +++ b/media/base/media_channel.h @@ -224,6 +224,8 @@ class MediaChannel : public sigslot::has_slots<> { // ssrc must be the first SSRC of the media stream if the stream uses // multiple SSRCs. virtual bool RemoveRecvStream(uint32_t ssrc) = 0; + // Resets any cached StreamParams for an unsignaled RecvStream. + virtual void ResetUnsignaledRecvStream() = 0; // Returns the absoulte sendtime extension id value from media channel. virtual int GetRtpSendTimeExtnId() const; // Set the frame encryptor to use on all outgoing frames. This is optional. diff --git a/media/base/rtp_data_engine.cc b/media/base/rtp_data_engine.cc index 33c87cbf1a..6161085a7c 100644 --- a/media/base/rtp_data_engine.cc +++ b/media/base/rtp_data_engine.cc @@ -194,6 +194,9 @@ bool RtpDataMediaChannel::RemoveRecvStream(uint32_t ssrc) { return true; } +// Not implemented. +void RtpDataMediaChannel::ResetUnsignaledRecvStream() {} + void RtpDataMediaChannel::OnPacketReceived(rtc::CopyOnWriteBuffer packet, int64_t /* packet_time_us */) { RtpHeader header; diff --git a/media/base/rtp_data_engine.h b/media/base/rtp_data_engine.h index 60d5f55cf4..e5f071d5a9 100644 --- a/media/base/rtp_data_engine.h +++ b/media/base/rtp_data_engine.h @@ -72,6 +72,7 @@ class RtpDataMediaChannel : public DataMediaChannel { virtual bool RemoveSendStream(uint32_t ssrc); virtual bool AddRecvStream(const StreamParams& sp); virtual bool RemoveRecvStream(uint32_t ssrc); + virtual void ResetUnsignaledRecvStream(); virtual bool SetSend(bool send) { sending_ = send; return true; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 10154d12fc..96a426d88d 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -1325,7 +1325,6 @@ bool WebRtcVideoChannel::AddRecvStream(const StreamParams& sp, return false; uint32_t ssrc = sp.first_ssrc(); - RTC_DCHECK(ssrc != 0); // TODO(pbos): Is this ever valid? // Remove running stream if this was a default stream. const auto& prev_stream = receive_streams_.find(ssrc); @@ -1417,12 +1416,6 @@ void WebRtcVideoChannel::ConfigureReceiverRtp( bool WebRtcVideoChannel::RemoveRecvStream(uint32_t ssrc) { RTC_DCHECK_RUN_ON(&thread_checker_); RTC_LOG(LS_INFO) << "RemoveRecvStream: " << ssrc; - if (ssrc == 0) { - // This indicates that we need to remove the unsignaled stream parameters - // that are cached. - unsignaled_stream_params_ = StreamParams(); - return true; - } std::map::iterator stream = receive_streams_.find(ssrc); @@ -1436,6 +1429,12 @@ bool WebRtcVideoChannel::RemoveRecvStream(uint32_t ssrc) { return true; } +void WebRtcVideoChannel::ResetUnsignaledRecvStream() { + RTC_DCHECK_RUN_ON(&thread_checker_); + RTC_LOG(LS_INFO) << "ResetUnsignaledRecvStream."; + unsignaled_stream_params_ = StreamParams(); +} + bool WebRtcVideoChannel::SetSink( uint32_t ssrc, rtc::VideoSinkInterface* sink) { diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index 88956e9283..b989e22f88 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -156,6 +156,7 @@ class WebRtcVideoChannel : public VideoMediaChannel, bool AddRecvStream(const StreamParams& sp) override; bool AddRecvStream(const StreamParams& sp, bool default_stream); bool RemoveRecvStream(uint32_t ssrc) override; + void ResetUnsignaledRecvStream() override; bool SetSink(uint32_t ssrc, rtc::VideoSinkInterface* sink) override; void FillBitrateInfo(BandwidthEstimationInfo* bwe_info) override; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index ca2d1a2344..1ed3dc3f17 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -5533,9 +5533,9 @@ TEST_F(WebRtcVideoChannelTest, RecvUnsignaledSsrcWithSignaledStreamId) { EXPECT_EQ(kSyncLabel, fake_call_->GetVideoReceiveStreams()[0]->GetConfig().sync_group); - // Removing the unsignaled stream should clear the cache. This time when + // Reset the unsignaled stream to clear the cache. This time when // a default video receive stream is created it won't have a sync_group. - ASSERT_TRUE(channel_->RemoveRecvStream(0)); + channel_->ResetUnsignaledRecvStream(); ASSERT_TRUE(channel_->RemoveRecvStream(kIncomingUnsignalledSsrc)); EXPECT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()); diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 195ed5b408..bef9d23840 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -1841,10 +1841,6 @@ bool WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) { } const uint32_t ssrc = sp.first_ssrc(); - if (ssrc == 0) { - RTC_DLOG(LS_WARNING) << "AddRecvStream with ssrc==0 is not supported."; - return false; - } // If this stream was previously received unsignaled, we promote it, possibly // recreating the AudioReceiveStream, if stream ids have changed. @@ -1880,13 +1876,6 @@ bool WebRtcVoiceMediaChannel::RemoveRecvStream(uint32_t ssrc) { RTC_DCHECK(worker_thread_checker_.IsCurrent()); RTC_LOG(LS_INFO) << "RemoveRecvStream: " << ssrc; - if (ssrc == 0) { - // This indicates that we need to remove the unsignaled stream parameters - // that are cached. - unsignaled_stream_params_ = StreamParams(); - return true; - } - const auto it = recv_streams_.find(ssrc); if (it == recv_streams_.end()) { RTC_LOG(LS_WARNING) << "Try to remove stream with ssrc " << ssrc @@ -1902,6 +1891,12 @@ bool WebRtcVoiceMediaChannel::RemoveRecvStream(uint32_t ssrc) { return true; } +void WebRtcVoiceMediaChannel::ResetUnsignaledRecvStream() { + RTC_DCHECK(worker_thread_checker_.IsCurrent()); + RTC_LOG(LS_INFO) << "ResetUnsignaledRecvStream."; + unsignaled_stream_params_ = StreamParams(); +} + bool WebRtcVoiceMediaChannel::SetLocalSource(uint32_t ssrc, AudioSource* source) { auto it = send_streams_.find(ssrc); diff --git a/media/engine/webrtc_voice_engine.h b/media/engine/webrtc_voice_engine.h index 8990b21fb7..8067ef064e 100644 --- a/media/engine/webrtc_voice_engine.h +++ b/media/engine/webrtc_voice_engine.h @@ -166,6 +166,7 @@ class WebRtcVoiceMediaChannel final : public VoiceMediaChannel, bool RemoveSendStream(uint32_t ssrc) override; bool AddRecvStream(const StreamParams& sp) override; bool RemoveRecvStream(uint32_t ssrc) override; + void ResetUnsignaledRecvStream() override; // E2EE Frame API // Set a frame decryptor to a particular ssrc that will intercept all diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index c0f0f2eba0..8fac2a1f92 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -2590,9 +2590,9 @@ TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaledSsrcWithSignaledStreamId) { GetRecvStream(kSsrc1).VerifyLastPacket(kPcmuFrame, sizeof(kPcmuFrame))); EXPECT_EQ(kSyncLabel, GetRecvStream(kSsrc1).GetConfig().sync_group); - // Removing the unsignaled stream clears the cached parameters. If a new + // Remset the unsignaled stream to clear the cached parameters. If a new // default unsignaled receive stream is created it will not have a sync group. - channel_->RemoveRecvStream(0); + channel_->ResetUnsignaledRecvStream(); channel_->RemoveRecvStream(kSsrc1); DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame)); @@ -2763,9 +2763,9 @@ TEST_F(WebRtcVoiceEngineTestFake, StreamCleanup) { EXPECT_EQ(0u, call_.GetAudioReceiveStreams().size()); } -TEST_F(WebRtcVoiceEngineTestFake, TestAddRecvStreamFailWithZeroSsrc) { +TEST_F(WebRtcVoiceEngineTestFake, TestAddRecvStreamSuccessWithZeroSsrc) { EXPECT_TRUE(SetupSendStream()); - EXPECT_FALSE(AddRecvStream(0)); + EXPECT_TRUE(AddRecvStream(0)); } TEST_F(WebRtcVoiceEngineTestFake, TestAddRecvStreamFailWithSameSsrc) { diff --git a/pc/channel.cc b/pc/channel.cc index bcc3d161a3..83927750e5 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -580,6 +580,11 @@ bool BaseChannel::RemoveRecvStream_w(uint32_t ssrc) { return media_channel()->RemoveRecvStream(ssrc); } +void BaseChannel::ResetUnsignaledRecvStream_w() { + RTC_DCHECK(worker_thread() == rtc::Thread::Current()); + media_channel()->ResetUnsignaledRecvStream(); +} + bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, SdpType type, std::string* error_desc) { @@ -666,8 +671,11 @@ bool BaseChannel::UpdateRemoteStreams_w( for (const StreamParams& old_stream : remote_streams_) { // If we no longer have an unsignaled stream, we would like to remove // the unsignaled stream params that are cached. - if ((!old_stream.has_ssrcs() && !HasStreamWithNoSsrcs(streams)) || - !GetStreamBySsrc(streams, old_stream.first_ssrc())) { + if (!old_stream.has_ssrcs() && !HasStreamWithNoSsrcs(streams)) { + ResetUnsignaledRecvStream_w(); + RTC_LOG(LS_INFO) << "Reset unsignaled remote stream."; + } else if (old_stream.has_ssrcs() && + !GetStreamBySsrc(streams, old_stream.first_ssrc())) { if (RemoveRecvStream_w(old_stream.first_ssrc())) { RTC_LOG(LS_INFO) << "Remove remote ssrc: " << old_stream.first_ssrc(); } else { @@ -688,10 +696,16 @@ bool BaseChannel::UpdateRemoteStreams_w( if ((!new_stream.has_ssrcs() && !HasStreamWithNoSsrcs(remote_streams_)) || !GetStreamBySsrc(remote_streams_, new_stream.first_ssrc())) { if (AddRecvStream_w(new_stream)) { - RTC_LOG(LS_INFO) << "Add remote ssrc: " << new_stream.first_ssrc(); + RTC_LOG(LS_INFO) << "Add remote ssrc: " + << (new_stream.has_ssrcs() + ? std::to_string(new_stream.first_ssrc()) + : "unsignaled"); } else { rtc::StringBuilder desc; - desc << "Failed to add remote stream ssrc: " << new_stream.first_ssrc(); + desc << "Failed to add remote stream ssrc: " + << (new_stream.has_ssrcs() + ? std::to_string(new_stream.first_ssrc()) + : "unsignaled"); SafeSetError(desc.str(), error_desc); ret = false; } diff --git a/pc/channel.h b/pc/channel.h index 3b76776842..62fcaa25d6 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -233,6 +233,7 @@ class BaseChannel : public ChannelInterface, bool AddRecvStream_w(const StreamParams& sp); bool RemoveRecvStream_w(uint32_t ssrc); + void ResetUnsignaledRecvStream_w(); bool AddSendStream_w(const StreamParams& sp); bool RemoveSendStream_w(uint32_t ssrc);