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}
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 fba4547..4ad4956 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 @@
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 76b51df..ceb911d 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 @@
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<rtcp::RawPacket> serialized_packet = feedback.Build();
+ EXPECT_TRUE(serialized_packet.get() != nullptr);
+ rtc::scoped_ptr<TransportFeedback> 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 81ee72f..cdbc47d 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 @@
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 @@
kCumulativeLoss, kJitter));
}
+TEST_F(RtcpReceiverTest, ReceivesTransportFeedback) {
+ const uint32_t kSenderSsrc = 0x10203;
+ const uint32_t kSourceSsrc = 0x123456;
+
+ std::set<uint32_t> 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<rtcp::RawPacket> 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<uint32_t> 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<rtcp::RawPacket> 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<uint16_t>::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 6c1deb4..d2b8043 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc
@@ -1235,13 +1235,13 @@
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;
