From 0dc57eabf6471084df5b9123eb1271636d8f99f3 Mon Sep 17 00:00:00 2001 From: brandtr Date: Mon, 29 May 2017 23:33:31 -0700 Subject: [PATCH] Avoid toggling default receive streams in WebRtcVideoChannel2. This CL removes |default_recv_ssrc_| from DefaultUnsignalledSsrcHandler and replaces it with calls to a new member function WebRtcVideoChannel2::GetDefaultReceiveStreamSsrc. The latter checks the |default_stream_| member on the WebRtcVideoChannel2::WebRtcVideoReceiveStreams to know which stream is the current default stream. This change removes duplicate state and fixes an issue where incoming unsignaled SSRCs would compete for being the default receive stream. BUG=webrtc:7725 Review-Url: https://codereview.webrtc.org/2906893002 Cr-Commit-Position: refs/heads/master@{#18314} --- webrtc/media/engine/webrtcvideoengine2.cc | 35 ++++++++++--- webrtc/media/engine/webrtcvideoengine2.h | 12 ++--- .../engine/webrtcvideoengine2_unittest.cc | 49 +++++++++++++++++++ 3 files changed, 82 insertions(+), 14 deletions(-) diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 4980153490..abb35843c0 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -428,14 +428,18 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::ConfigureVideoEncoderSettings( } DefaultUnsignalledSsrcHandler::DefaultUnsignalledSsrcHandler() - : default_recv_ssrc_(0), default_sink_(NULL) {} + : default_sink_(nullptr) {} UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc( WebRtcVideoChannel2* channel, uint32_t ssrc) { - if (default_recv_ssrc_ != 0) { // Already one default stream, so replace it. - channel->RemoveRecvStream(default_recv_ssrc_); - default_recv_ssrc_ = 0; + rtc::Optional default_recv_ssrc = + channel->GetDefaultReceiveStreamSsrc(); + + if (default_recv_ssrc) { + LOG(LS_INFO) << "Destroying old default receive stream for SSRC=" << ssrc + << "."; + channel->RemoveRecvStream(*default_recv_ssrc); } StreamParams sp; @@ -446,7 +450,6 @@ UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc( } channel->SetSink(ssrc, default_sink_); - default_recv_ssrc_ = ssrc; return kDeliverPacket; } @@ -456,11 +459,13 @@ DefaultUnsignalledSsrcHandler::GetDefaultSink() const { } void DefaultUnsignalledSsrcHandler::SetDefaultSink( - VideoMediaChannel* channel, + WebRtcVideoChannel2* channel, rtc::VideoSinkInterface* sink) { default_sink_ = sink; - if (default_recv_ssrc_ != 0) { - channel->SetSink(default_recv_ssrc_, default_sink_); + rtc::Optional default_recv_ssrc = + channel->GetDefaultReceiveStreamSsrc(); + if (default_recv_ssrc) { + channel->SetSink(*default_recv_ssrc, default_sink_); } } @@ -1321,6 +1326,8 @@ bool WebRtcVideoChannel2::SetSink( LOG(LS_INFO) << "SetSink: ssrc:" << ssrc << " " << (sink ? "(ptr)" : "nullptr"); if (ssrc == 0) { + // Do not hold |stream_crit_| here, since SetDefaultSink will call + // WebRtcVideoChannel2::GetDefaultReceiveStreamSsrc(). default_unsignalled_ssrc_handler_.SetDefaultSink(this, sink); return true; } @@ -1529,6 +1536,18 @@ void WebRtcVideoChannel2::SetInterface(NetworkInterface* iface) { kVideoRtpBufferSize); } +rtc::Optional WebRtcVideoChannel2::GetDefaultReceiveStreamSsrc() { + rtc::CritScope stream_lock(&stream_crit_); + rtc::Optional ssrc; + for (auto it = receive_streams_.begin(); it != receive_streams_.end(); ++it) { + if (it->second->IsDefaultStream()) { + ssrc.emplace(it->first); + break; + } + } + return ssrc; +} + bool WebRtcVideoChannel2::SendRtp(const uint8_t* data, size_t len, const webrtc::PacketOptions& options) { diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index 2c7d36ccb5..c40014a39e 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -22,13 +22,14 @@ #include "webrtc/base/asyncinvoker.h" #include "webrtc/base/criticalsection.h" #include "webrtc/base/networkroute.h" +#include "webrtc/base/optional.h" #include "webrtc/base/thread_annotations.h" #include "webrtc/base/thread_checker.h" -#include "webrtc/media/base/videosinkinterface.h" -#include "webrtc/media/base/videosourceinterface.h" #include "webrtc/call/call.h" #include "webrtc/call/flexfec_receive_stream.h" #include "webrtc/media/base/mediaengine.h" +#include "webrtc/media/base/videosinkinterface.h" +#include "webrtc/media/base/videosourceinterface.h" #include "webrtc/media/engine/webrtcvideodecoderfactory.h" #include "webrtc/media/engine/webrtcvideoencoderfactory.h" #include "webrtc/video_receive_stream.h" @@ -81,15 +82,12 @@ class DefaultUnsignalledSsrcHandler : public UnsignalledSsrcHandler { uint32_t ssrc) override; rtc::VideoSinkInterface* GetDefaultSink() const; - void SetDefaultSink(VideoMediaChannel* channel, + void SetDefaultSink(WebRtcVideoChannel2* channel, rtc::VideoSinkInterface* sink); - uint32_t default_recv_ssrc() const { return default_recv_ssrc_; } - virtual ~DefaultUnsignalledSsrcHandler() = default; private: - uint32_t default_recv_ssrc_; rtc::VideoSinkInterface* default_sink_; }; @@ -177,6 +175,8 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { // Implemented for VideoMediaChannelTest. bool sending() const { return sending_; } + rtc::Optional GetDefaultReceiveStreamSsrc(); + // AdaptReason is used for expressing why a WebRtcVideoSendStream request // a lower input frame size than the currently configured camera input frame // size. There can be more than one reason OR:ed together. diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 46664c4d94..43dbbc6d18 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -3969,6 +3969,55 @@ TEST_F(WebRtcVideoChannel2Test, ReceiveDifferentUnsignaledSsrc) { #endif } +// This test verifies that when a new default stream is created for a new +// unsignaled SSRC, the new stream does not overwrite any old stream that had +// been the default receive stream before being properly signaled. +TEST_F(WebRtcVideoChannel2Test, + NewUnsignaledStreamDoesNotDestroyPreviouslyUnsignaledStream) { + cricket::VideoRecvParameters parameters; + parameters.codecs.push_back(GetEngineCodec("VP8")); + ASSERT_TRUE(channel_->SetRecvParameters(parameters)); + + // No streams signaled and no packets received, so we should not have any + // stream objects created yet. + EXPECT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()); + + // Receive packet on an unsignaled SSRC. + uint8_t data[kMinRtpPacketLen]; + cricket::RtpHeader rtp_header; + rtp_header.payload_type = GetEngineCodec("VP8").id; + rtp_header.seq_num = rtp_header.timestamp = 0; + rtp_header.ssrc = kSsrcs3[0]; + cricket::SetRtpHeader(data, sizeof(data), rtp_header); + rtc::CopyOnWriteBuffer packet(data, sizeof(data)); + rtc::PacketTime packet_time; + channel_->OnPacketReceived(&packet, packet_time); + // Default receive stream should be created. + ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()); + FakeVideoReceiveStream* recv_stream0 = + fake_call_->GetVideoReceiveStreams()[0]; + EXPECT_EQ(kSsrcs3[0], recv_stream0->GetConfig().rtp.remote_ssrc); + + // Signal the SSRC. + EXPECT_TRUE( + channel_->AddRecvStream(cricket::StreamParams::CreateLegacy(kSsrcs3[0]))); + ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()); + recv_stream0 = fake_call_->GetVideoReceiveStreams()[0]; + EXPECT_EQ(kSsrcs3[0], recv_stream0->GetConfig().rtp.remote_ssrc); + + // Receive packet on a different unsignaled SSRC. + rtp_header.ssrc = kSsrcs3[1]; + cricket::SetRtpHeader(data, sizeof(data), rtp_header); + packet.SetData(data, sizeof(data)); + channel_->OnPacketReceived(&packet, packet_time); + // New default receive stream should be created, but old stream should remain. + ASSERT_EQ(2u, fake_call_->GetVideoReceiveStreams().size()); + EXPECT_EQ(recv_stream0, fake_call_->GetVideoReceiveStreams()[0]); + FakeVideoReceiveStream* recv_stream1 = + fake_call_->GetVideoReceiveStreams()[1]; + EXPECT_EQ(kSsrcs3[1], recv_stream1->GetConfig().rtp.remote_ssrc); +} + TEST_F(WebRtcVideoChannel2Test, CanSentMaxBitrateForExistingStream) { AddSendStream();