From a2a6fe66a39797ea61a04d80ce3afc494d850bfc Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Fri, 6 Mar 2015 15:35:19 +0000 Subject: [PATCH] Reconfigure default streams on AddRecvStream. Makes sure RTX can be used for streams that have received early media before being properly configured. BUG=1788 R=mflodman@webrtc.org Review URL: https://webrtc-codereview.appspot.com/46499004 Cr-Commit-Position: refs/heads/master@{#8634} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8634 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/media/webrtc/webrtcvideoengine2.cc | 41 ++++++++++++---- talk/media/webrtc/webrtcvideoengine2.h | 9 +++- .../webrtc/webrtcvideoengine2_unittest.cc | 48 ++++++++++++++++++- .../webrtc/webrtcvideoengine2_unittest.h | 3 +- 4 files changed, 86 insertions(+), 15 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index e6f59d12e..23c8a9d22 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -276,7 +276,7 @@ DefaultUnsignalledSsrcHandler::DefaultUnsignalledSsrcHandler() : default_recv_ssrc_(0), default_renderer_(NULL) {} UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc( - VideoMediaChannel* channel, + WebRtcVideoChannel2* channel, uint32_t ssrc) { if (default_recv_ssrc_ != 0) { // Already one default stream. LOG(LS_WARNING) << "Unknown SSRC, but default receive stream already set."; @@ -286,7 +286,7 @@ UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc( StreamParams sp; sp.ssrcs.push_back(ssrc); LOG(LS_INFO) << "Creating default receive stream for SSRC=" << ssrc << "."; - if (!channel->AddRecvStream(sp)) { + if (!channel->AddRecvStream(sp, true)) { LOG(LS_WARNING) << "Could not create default receive stream."; } @@ -801,7 +801,7 @@ bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) { // ssrc. rtc::CritScope stream_lock(&stream_crit_); if (send_streams_.find(ssrc) != send_streams_.end()) { - LOG(LS_ERROR) << "Send stream with ssrc '" << ssrc << "' already exists."; + LOG(LS_ERROR) << "Send stream with SSRC '" << ssrc << "' already exists."; return false; } @@ -875,6 +875,11 @@ bool WebRtcVideoChannel2::RemoveSendStream(uint32 ssrc) { } bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) { + return AddRecvStream(sp, false); +} + +bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp, + bool default_stream) { LOG(LS_INFO) << "AddRecvStream: " << sp.ToString(); assert(sp.ssrcs.size() > 0); @@ -883,9 +888,17 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) { // TODO(pbos): Check if any of the SSRCs overlap. rtc::CritScope stream_lock(&stream_crit_); - if (receive_streams_.find(ssrc) != receive_streams_.end()) { - LOG(LS_ERROR) << "Receive stream for SSRC " << ssrc << "already exists."; - return false; + { + auto it = receive_streams_.find(ssrc); + if (it != receive_streams_.end()) { + if (default_stream || !it->second->IsDefaultStream()) { + LOG(LS_ERROR) << "Receive stream for SSRC '" << ssrc + << "' already exists."; + return false; + } + delete it->second; + receive_streams_.erase(it); + } } webrtc::VideoReceiveStream::Config config; @@ -902,8 +915,9 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) { static_cast(voice_channel_)->voe_channel(); } - receive_streams_[ssrc] = new WebRtcVideoReceiveStream( - call_.get(), external_decoder_factory_, config, recv_codecs_); + receive_streams_[ssrc] = + new WebRtcVideoReceiveStream(call_.get(), external_decoder_factory_, + default_stream, config, recv_codecs_); return true; } @@ -1098,8 +1112,9 @@ void WebRtcVideoChannel2::OnPacketReceived( return; } - // TODO(pbos): Make sure that the unsignalled SSRC uses the video payload. - // Also figure out whether RTX needs to be handled. + // TODO(pbos): Ignore unsignalled packets that don't use the video payload + // (prevent creating default receivers for RTX configured as if it would + // receive media payloads on those SSRCs). switch (unsignalled_ssrc_handler_->OnUnsignalledSsrc(this, ssrc)) { case UnsignalledSsrcHandler::kDropPacket: return; @@ -1857,10 +1872,12 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::RecreateWebRtcStream() { WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( webrtc::Call* call, WebRtcVideoDecoderFactory* external_decoder_factory, + bool default_stream, const webrtc::VideoReceiveStream::Config& config, const std::vector& recv_codecs) : call_(call), stream_(NULL), + default_stream_(default_stream), config_(config), external_decoder_factory_(external_decoder_factory), renderer_(NULL), @@ -2004,6 +2021,10 @@ bool WebRtcVideoChannel2::WebRtcVideoReceiveStream::IsTextureSupported() const { return true; } +bool WebRtcVideoChannel2::WebRtcVideoReceiveStream::IsDefaultStream() const { + return default_stream_; +} + void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRenderer( cricket::VideoRenderer* renderer) { rtc::CritScope crit(&renderer_lock_); diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index b5d69c68d..5dc1b044f 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -81,7 +81,7 @@ class UnsignalledSsrcHandler { kDropPacket, kDeliverPacket, }; - virtual Action OnUnsignalledSsrc(VideoMediaChannel* engine, + virtual Action OnUnsignalledSsrc(WebRtcVideoChannel2* channel, uint32_t ssrc) = 0; }; @@ -89,7 +89,8 @@ class UnsignalledSsrcHandler { class DefaultUnsignalledSsrcHandler : public UnsignalledSsrcHandler { public: DefaultUnsignalledSsrcHandler(); - Action OnUnsignalledSsrc(VideoMediaChannel* engine, uint32_t ssrc) override; + Action OnUnsignalledSsrc(WebRtcVideoChannel2* channel, + uint32_t ssrc) override; VideoRenderer* GetDefaultRenderer() const; void SetDefaultRenderer(VideoMediaChannel* channel, VideoRenderer* renderer); @@ -197,6 +198,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, bool AddSendStream(const StreamParams& sp) override; bool RemoveSendStream(uint32 ssrc) override; bool AddRecvStream(const StreamParams& sp) override; + bool AddRecvStream(const StreamParams& sp, bool default_stream); bool RemoveRecvStream(uint32 ssrc) override; bool SetRenderer(uint32 ssrc, VideoRenderer* renderer) override; bool GetStats(VideoMediaInfo* info) override; @@ -387,6 +389,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, WebRtcVideoReceiveStream( webrtc::Call*, WebRtcVideoDecoderFactory* external_decoder_factory, + bool default_stream, const webrtc::VideoReceiveStream::Config& config, const std::vector& recv_codecs); ~WebRtcVideoReceiveStream(); @@ -397,6 +400,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, void RenderFrame(const webrtc::I420VideoFrame& frame, int time_to_render_ms) override; bool IsTextureSupported() const override; + bool IsDefaultStream() const; void SetRenderer(cricket::VideoRenderer* renderer); cricket::VideoRenderer* GetRenderer(); @@ -425,6 +429,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, webrtc::Call* const call_; webrtc::VideoReceiveStream* stream_; + const bool default_stream_; webrtc::VideoReceiveStream::Config config_; WebRtcVideoDecoderFactory* const external_decoder_factory_; diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index b29fc5151..4cf0462b0 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -333,8 +333,21 @@ void FakeCall::DestroyVideoReceiveStream( } webrtc::PacketReceiver* FakeCall::Receiver() { - // TODO(pbos): Fix this. - return NULL; + return this; +} + +FakeCall::DeliveryStatus FakeCall::DeliverPacket(const uint8_t* packet, + size_t length) { + CHECK(length >= 12); + uint32_t ssrc; + if (!GetRtpSsrc(packet, length, &ssrc)) + return DELIVERY_PACKET_ERROR; + + for (auto& receiver: video_receive_streams_) { + if (receiver->GetConfig().rtp.remote_ssrc == ssrc) + return DELIVERY_OK; + } + return DELIVERY_UNKNOWN_SSRC; } void FakeCall::SetStats(const webrtc::Call::Stats& stats) { @@ -2349,6 +2362,37 @@ TEST_F(WebRtcVideoChannel2Test, TranslatesSenderBitrateStatsCorrectly) { << "Bandwidth stats should take all streams into account."; } +TEST_F(WebRtcVideoChannel2Test, DefaultReceiveStreamReconfiguresToUseRtx) { + EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs())); + + const std::vector ssrcs = MAKE_VECTOR(kSsrcs1); + const std::vector rtx_ssrcs = MAKE_VECTOR(kRtxSsrcs1); + + ASSERT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()); + const size_t kDataLength = 12; + uint8_t data[kDataLength]; + memset(data, 0, sizeof(data)); + rtc::SetBE32(&data[8], ssrcs[0]); + rtc::Buffer packet(data, kDataLength); + rtc::PacketTime packet_time; + channel_->OnPacketReceived(&packet, packet_time); + + ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()) + << "No default receive stream created."; + FakeVideoReceiveStream* recv_stream = fake_call_->GetVideoReceiveStreams()[0]; + EXPECT_EQ(0u, recv_stream->GetConfig().rtp.rtx.size()) + << "Default receive stream should not have configured RTX"; + + EXPECT_TRUE(channel_->AddRecvStream( + cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs))); + ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()) + << "AddRecvStream should've reconfigured, not added a new receiver."; + recv_stream = fake_call_->GetVideoReceiveStreams()[0]; + ASSERT_EQ(1u, recv_stream->GetConfig().rtp.rtx.size()); + EXPECT_EQ(rtx_ssrcs[0], + recv_stream->GetConfig().rtp.rtx.begin()->second.ssrc); +} + class WebRtcVideoEngine2SimulcastTest : public testing::Test { public: WebRtcVideoEngine2SimulcastTest() diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.h b/talk/media/webrtc/webrtcvideoengine2_unittest.h index 770353540..7032dbe92 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.h +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.h @@ -99,7 +99,7 @@ class FakeVideoReceiveStream : public webrtc::VideoReceiveStream { webrtc::VideoReceiveStream::Stats stats_; }; -class FakeCall : public webrtc::Call { +class FakeCall : public webrtc::Call, public webrtc::PacketReceiver { public: FakeCall(const webrtc::Call::Config& config); ~FakeCall(); @@ -135,6 +135,7 @@ class FakeCall : public webrtc::Call { void DestroyVideoReceiveStream( webrtc::VideoReceiveStream* receive_stream) override; webrtc::PacketReceiver* Receiver() override; + DeliveryStatus DeliverPacket(const uint8_t* packet, size_t length) override; webrtc::Call::Stats GetStats() const override;