From 49f9cdba02248d216dfc875dc0ab3c5ae187bc42 Mon Sep 17 00:00:00 2001 From: sprang Date: Thu, 1 Oct 2015 03:06:57 -0700 Subject: [PATCH] Fix bug where rtcp::TransportFeedback may generate incorrect messages. In particular, if 14 short deltas were inserted (2 * capacity of status vector chunk with 2bit items) followed by a large delta, that status item would be dropped. BUG= Review URL: https://codereview.webrtc.org/1367193002 Cr-Commit-Position: refs/heads/master@{#10132} --- .../source/rtcp_packet/transport_feedback.cc | 2 +- .../transport_feedback_unittest.cc | 24 +++++++ .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 69 ++++++++++++++++++- .../modules/rtp_rtcp/source/rtcp_utility.cc | 4 +- 4 files changed, 94 insertions(+), 5 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc index fba4547862..4ad49561b8 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc @@ -447,7 +447,7 @@ bool TransportFeedback::Encode(StatusSymbol symbol) { return true; } else { // New symbol does not match what's already in symbol_vec. - if (first_symbol_cardinality_ > capacity) { + if (first_symbol_cardinality_ >= capacity) { // Symbols in symbol_vec can only be RLE-encoded. Emit the RLE-chunk // and re-add input. symbol_vec is then guaranteed to have room for the // symbol, so recursion cannot continue. diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc index 76b51df0ef..ceb911d308 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc @@ -454,5 +454,29 @@ TEST(RtcpPacketTest, TransportFeedback_Padding) { EXPECT_EQ(kExpectedSizeWords * 4, packet->Length()); // Padding not included. } +TEST(RtcpPacketTest, TransportFeedback_CorrectlySplitsVectorChunks) { + const int kOneBitVectorCapacity = 14; + const int64_t kLargeTimeDelta = + TransportFeedback::kDeltaScaleFactor * (1 << 8); + + // Test that a number of small deltas followed by a large delta results in a + // correct split into multiple chunks, as needed. + + for (int deltas = 0; deltas <= kOneBitVectorCapacity + 1; ++deltas) { + TransportFeedback feedback; + feedback.WithBase(0, 0); + for (int i = 0; i < deltas; ++i) + feedback.WithReceivedPacket(i, i * 1000); + feedback.WithReceivedPacket(deltas, deltas * 1000 + kLargeTimeDelta); + + rtc::scoped_ptr serialized_packet = feedback.Build(); + EXPECT_TRUE(serialized_packet.get() != nullptr); + rtc::scoped_ptr deserialized_packet = + TransportFeedback::ParseFrom(serialized_packet->Buffer(), + serialized_packet->Length()); + EXPECT_TRUE(deserialized_packet.get() != nullptr); + } +} + } // namespace } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 81ee72fb1f..cdbc47d2d7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -19,11 +19,13 @@ #include "webrtc/common_types.h" #include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_observer.h" #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h" +#include "webrtc/modules/rtp_rtcp/source/byte_io.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_receiver.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_sender.h" #include "webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h" #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" +#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" namespace webrtc { @@ -117,9 +119,10 @@ class RtcpReceiverTest : public ::testing::Test { rtcp_packet_info_.ntp_frac = rtcpPacketInformation.ntp_frac; rtcp_packet_info_.rtp_timestamp = rtcpPacketInformation.rtp_timestamp; rtcp_packet_info_.xr_dlrr_item = rtcpPacketInformation.xr_dlrr_item; - if (rtcpPacketInformation.VoIPMetric) { + if (rtcpPacketInformation.VoIPMetric) rtcp_packet_info_.AddVoIPMetric(rtcpPacketInformation.VoIPMetric); - } + rtcp_packet_info_.transport_feedback_.reset( + rtcpPacketInformation.transport_feedback_.release()); return 0; } @@ -1025,6 +1028,68 @@ TEST_F(RtcpReceiverTest, Callbacks) { kCumulativeLoss, kJitter)); } +TEST_F(RtcpReceiverTest, ReceivesTransportFeedback) { + const uint32_t kSenderSsrc = 0x10203; + const uint32_t kSourceSsrc = 0x123456; + + std::set ssrcs; + ssrcs.insert(kSourceSsrc); + rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs); + + rtcp::TransportFeedback packet; + packet.WithMediaSourceSsrc(kSourceSsrc); + packet.WithPacketSenderSsrc(kSenderSsrc); + packet.WithBase(1, 1000); + packet.WithReceivedPacket(1, 1000); + + rtc::scoped_ptr built_packet = packet.Build(); + ASSERT_TRUE(built_packet.get() != nullptr); + + EXPECT_EQ(0, + InjectRtcpPacket(built_packet->Buffer(), built_packet->Length())); + + EXPECT_NE(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpTransportFeedback); + EXPECT_TRUE(rtcp_packet_info_.transport_feedback_.get() != nullptr); +} + +TEST_F(RtcpReceiverTest, HandlesInvalidTransportFeedback) { + const uint32_t kSenderSsrc = 0x10203; + const uint32_t kSourceSsrc = 0x123456; + + std::set ssrcs; + ssrcs.insert(kSourceSsrc); + rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs); + + // Send a compound packet with a TransportFeedback followed by something else. + rtcp::TransportFeedback packet; + packet.WithMediaSourceSsrc(kSourceSsrc); + packet.WithPacketSenderSsrc(kSenderSsrc); + packet.WithBase(1, 1000); + packet.WithReceivedPacket(1, 1000); + + static uint32_t kBitrateBps = 50000; + rtcp::Remb remb; + remb.From(kSourceSsrc); + remb.WithBitrateBps(kBitrateBps); + packet.Append(&remb); + + rtc::scoped_ptr built_packet = packet.Build(); + ASSERT_TRUE(built_packet.get() != nullptr); + + // Modify the TransportFeedback packet so that it is invalid. + const size_t kStatusCountOffset = 14; + ByteWriter::WriteBigEndian( + &built_packet->MutableBuffer()[kStatusCountOffset], 42); + + EXPECT_EQ(0, + InjectRtcpPacket(built_packet->Buffer(), built_packet->Length())); + + // Transport feedback should be ignored, but next packet should work. + EXPECT_EQ(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpTransportFeedback); + EXPECT_NE(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpRemb); + EXPECT_EQ(kBitrateBps, rtcp_packet_info_.receiverEstimatedMaxBitrate); +} + } // Anonymous namespace } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc index 6c1deb467e..d2b80438cc 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc @@ -1235,13 +1235,13 @@ bool RTCPUtility::RTCPParserV2::ParseFBCommon(const RtcpCommonHeader& header) { return true; } case 15: { - _packetType = RTCPPacketTypes::kTransportFeedback; rtcp_packet_ = rtcp::TransportFeedback::ParseFrom(_ptrRTCPData - 12, length); // Since we parse the whole packet here, keep the TopLevel state and // just end the current block. + EndCurrentBlock(); if (rtcp_packet_.get()) { - EndCurrentBlock(); + _packetType = RTCPPacketTypes::kTransportFeedback; return true; } break;