From caba2d2a370cb6b5e67c881ecfa57fdac7411de8 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Wed, 14 May 2014 13:57:12 +0000 Subject: [PATCH] Add DeliveryStatus enum to DeliverPacket(). Allows signalling why packet delivery failed. Especially enables signaling that delivery fails because the incoming packet had an unknown SSRC. This allows an application to react and create receivers for the new streams. R=mflodman@webrtc.org BUG=3228 Review URL: https://webrtc-codereview.appspot.com/12289005 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6150 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/call.h | 9 ++++- webrtc/test/fake_network_pipe_unittest.cc | 2 +- webrtc/video/call.cc | 41 +++++++++++++---------- webrtc/video/call_perf_tests.cc | 8 +++-- webrtc/video/call_tests.cc | 11 +++--- webrtc/video/full_stack.cc | 3 +- webrtc/video/rampup_tests.cc | 5 +-- webrtc/video/video_send_stream_tests.cc | 9 ++--- 8 files changed, 55 insertions(+), 33 deletions(-) diff --git a/webrtc/call.h b/webrtc/call.h index 932770d4d9..53b17edadb 100644 --- a/webrtc/call.h +++ b/webrtc/call.h @@ -25,7 +25,14 @@ const char* Version(); class PacketReceiver { public: - virtual bool DeliverPacket(const uint8_t* packet, size_t length) = 0; + enum DeliveryStatus { + DELIVERY_OK, + DELIVERY_UNKNOWN_SSRC, + DELIVERY_PACKET_ERROR, + }; + + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) = 0; protected: virtual ~PacketReceiver() {} diff --git a/webrtc/test/fake_network_pipe_unittest.cc b/webrtc/test/fake_network_pipe_unittest.cc index 6655fa1658..0399688f62 100644 --- a/webrtc/test/fake_network_pipe_unittest.cc +++ b/webrtc/test/fake_network_pipe_unittest.cc @@ -33,7 +33,7 @@ class MockReceiver : public PacketReceiver { delete [] data; } - MOCK_METHOD2(DeliverPacket, bool(const uint8_t*, size_t)); + MOCK_METHOD2(DeliverPacket, DeliveryStatus(const uint8_t*, size_t)); }; class FakeNetworkPipeTest : public ::testing::Test { diff --git a/webrtc/video/call.cc b/webrtc/video/call.cc index a3276540b1..6adb0fccfd 100644 --- a/webrtc/video/call.cc +++ b/webrtc/video/call.cc @@ -86,13 +86,14 @@ class Call : public webrtc::Call, public PacketReceiver { virtual uint32_t SendBitrateEstimate() OVERRIDE; virtual uint32_t ReceiveBitrateEstimate() OVERRIDE; - virtual bool DeliverPacket(const uint8_t* packet, size_t length) OVERRIDE; + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) OVERRIDE; private: - bool DeliverRtcp(const uint8_t* packet, size_t length); - bool DeliverRtp(const RTPHeader& header, - const uint8_t* packet, - size_t length); + DeliveryStatus DeliverRtcp(const uint8_t* packet, size_t length); + DeliveryStatus DeliverRtp(const RTPHeader& header, + const uint8_t* packet, + size_t length); Call::Config config_; @@ -278,9 +279,12 @@ uint32_t Call::ReceiveBitrateEstimate() { return 0; } -bool Call::DeliverRtcp(const uint8_t* packet, size_t length) { +Call::PacketReceiver::DeliveryStatus Call::DeliverRtcp(const uint8_t* packet, + size_t length) { // TODO(pbos): Figure out what channel needs it actually. // Do NOT broadcast! Also make sure it's a valid packet. + // Return DELIVERY_UNKNOWN_SSRC if it can be determined that + // there's no receiver of the packet. bool rtcp_delivered = false; { ReadLockScoped read_lock(*receive_lock_); @@ -303,30 +307,33 @@ bool Call::DeliverRtcp(const uint8_t* packet, size_t length) { rtcp_delivered = true; } } - return rtcp_delivered; + return rtcp_delivered ? DELIVERY_OK : DELIVERY_PACKET_ERROR; } -bool Call::DeliverRtp(const RTPHeader& header, - const uint8_t* packet, - size_t length) { +Call::PacketReceiver::DeliveryStatus Call::DeliverRtp(const RTPHeader& header, + const uint8_t* packet, + size_t length) { ReadLockScoped read_lock(*receive_lock_); std::map::iterator it = receive_ssrcs_.find(header.ssrc); - if (it == receive_ssrcs_.end()) { - // TODO(pbos): Log some warning, SSRC without receiver. - return false; - } - return it->second->DeliverRtp(static_cast(packet), length); + + if (it == receive_ssrcs_.end()) + return DELIVERY_UNKNOWN_SSRC; + + return it->second->DeliverRtp(static_cast(packet), length) + ? DELIVERY_OK + : DELIVERY_PACKET_ERROR; } -bool Call::DeliverPacket(const uint8_t* packet, size_t length) { +Call::PacketReceiver::DeliveryStatus Call::DeliverPacket(const uint8_t* packet, + size_t length) { // TODO(pbos): ExtensionMap if there are extensions. if (RtpHeaderParser::IsRtcp(packet, static_cast(length))) return DeliverRtcp(packet, length); RTPHeader rtp_header; if (!rtp_header_parser_->Parse(packet, static_cast(length), &rtp_header)) - return false; + return DELIVERY_PACKET_ERROR; return DeliverRtp(rtp_header, packet, length); } diff --git a/webrtc/video/call_perf_tests.cc b/webrtc/video/call_perf_tests.cc index 9830dd1e2f..0076ce2f86 100644 --- a/webrtc/video/call_perf_tests.cc +++ b/webrtc/video/call_perf_tests.cc @@ -258,7 +258,8 @@ TEST_F(CallPerfTest, PlaysOutAudioAndVideoInSync) { : channel_(channel), voe_network_(voe_network), parser_(RtpHeaderParser::Create()) {} - virtual bool DeliverPacket(const uint8_t* packet, size_t length) { + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) OVERRIDE { int ret; if (parser_->IsRtcp(packet, static_cast(length))) { ret = voe_network_->ReceivedRTCPPacket( @@ -267,7 +268,7 @@ TEST_F(CallPerfTest, PlaysOutAudioAndVideoInSync) { ret = voe_network_->ReceivedRTPPacket( channel_, packet, static_cast(length), PacketTime()); } - return ret == 0; + return ret == 0 ? DELIVERY_OK : DELIVERY_PACKET_ERROR; } private: @@ -589,7 +590,8 @@ void CallPerfTest::TestMinTransmitBitrate(bool pad_to_min_bitrate) { } private: - virtual bool DeliverPacket(const uint8_t* packet, size_t length) OVERRIDE { + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) OVERRIDE { VideoSendStream::Stats stats = send_stream_->GetStats(); if (stats.substreams.size() > 0) { assert(stats.substreams.size() == 1); diff --git a/webrtc/video/call_tests.cc b/webrtc/video/call_tests.cc index 617b944f2b..b305246c18 100644 --- a/webrtc/video/call_tests.cc +++ b/webrtc/video/call_tests.cc @@ -840,7 +840,7 @@ TEST_F(CallTest, DISABLED_ReceivesPliAndRecoversWithoutNack) { ReceivesPliAndRecovers(0); } -TEST_F(CallTest, SurvivesIncomingRtpPacketsToDestroyedReceiveStream) { +TEST_F(CallTest, UnknownRtpPacketGivesUnknownSsrcReturnCode) { class PacketInputObserver : public PacketReceiver { public: explicit PacketInputObserver(PacketReceiver* receiver) @@ -851,13 +851,16 @@ TEST_F(CallTest, SurvivesIncomingRtpPacketsToDestroyedReceiveStream) { } private: - virtual bool DeliverPacket(const uint8_t* packet, size_t length) { + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) OVERRIDE { if (RtpHeaderParser::IsRtcp(packet, static_cast(length))) { return receiver_->DeliverPacket(packet, length); } else { - EXPECT_FALSE(receiver_->DeliverPacket(packet, length)); + DeliveryStatus delivery_status = + receiver_->DeliverPacket(packet, length); + EXPECT_EQ(DELIVERY_UNKNOWN_SSRC, delivery_status); delivered_packet_->Set(); - return false; + return delivery_status; } } diff --git a/webrtc/video/full_stack.cc b/webrtc/video/full_stack.cc index dbd234b8ed..cb0ba55a38 100644 --- a/webrtc/video/full_stack.cc +++ b/webrtc/video/full_stack.cc @@ -111,7 +111,8 @@ class VideoAnalyzer : public PacketReceiver, virtual void SetReceiver(PacketReceiver* receiver) { receiver_ = receiver; } - virtual bool DeliverPacket(const uint8_t* packet, size_t length) OVERRIDE { + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) OVERRIDE { scoped_ptr parser(RtpHeaderParser::Create()); RTPHeader header; parser->Parse(packet, static_cast(length), &header); diff --git a/webrtc/video/rampup_tests.cc b/webrtc/video/rampup_tests.cc index 32a299820d..c86963d033 100644 --- a/webrtc/video/rampup_tests.cc +++ b/webrtc/video/rampup_tests.cc @@ -268,7 +268,8 @@ class LowRateStreamObserver : public test::DirectTransport, return test::DirectTransport::SendRtp(data, length); } - virtual bool DeliverPacket(const uint8_t* packet, size_t length) OVERRIDE { + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) OVERRIDE { CriticalSectionScoped lock(crit_.get()); RTPHeader header; EXPECT_TRUE(rtp_parser_->Parse(packet, static_cast(length), &header)); @@ -279,7 +280,7 @@ class LowRateStreamObserver : public test::DirectTransport, remote_bitrate_estimator_->Process(); } suspended_in_stats_ = send_stream_->GetStats().suspended; - return true; + return DELIVERY_OK; } virtual bool SendRtcp(const uint8_t* packet, size_t length) OVERRIDE { diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index 13ac01cf58..666286650b 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -1160,13 +1160,14 @@ TEST_F(VideoSendStreamTest, MinTransmitBitrateRespectsRemb) { } private: - virtual bool DeliverPacket(const uint8_t* packet, size_t length) { + virtual DeliveryStatus DeliverPacket(const uint8_t* packet, + size_t length) OVERRIDE { if (RtpHeaderParser::IsRtcp(packet, static_cast(length))) - return true; + return DELIVERY_OK; RTPHeader header; if (!parser_->Parse(packet, static_cast(length), &header)) - return true; + return DELIVERY_PACKET_ERROR; assert(send_stream_ != NULL); VideoSendStream::Stats stats = send_stream_->GetStats(); if (!stats.substreams.empty()) { @@ -1188,7 +1189,7 @@ TEST_F(VideoSendStreamTest, MinTransmitBitrateRespectsRemb) { observation_complete_->Set(); } } - return true; + return DELIVERY_OK; } scoped_ptr rtp_rtcp_;