diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 2172bce938..524985c490 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -91,8 +91,8 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number, int64_t arrival_time) { int64_t seq = unwrapper_.Unwrap(sequence_number); - if (window_start_seq_ == -1) { - window_start_seq_ = seq; + if (packet_arrival_times_.lower_bound(window_start_seq_) == + packet_arrival_times_.end()) { // Start new feedback packet, cull old packets. for (auto it = packet_arrival_times_.begin(); it != packet_arrival_times_.end() && it->first < seq && @@ -101,6 +101,10 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number, ++it; packet_arrival_times_.erase(delete_it); } + } + + if (window_start_seq_ == -1) { + window_start_seq_ = sequence_number; } else if (seq < window_start_seq_) { window_start_seq_ = seq; } @@ -114,20 +118,24 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number, bool RemoteEstimatorProxy::BuildFeedbackPacket( rtcp::TransportFeedback* feedback_packet) { - rtc::CritScope cs(&lock_); - if (window_start_seq_ == -1) - return false; - // window_start_seq_ is the first sequence number to include in the current // feedback packet. Some older may still be in the map, in case a reordering // happens and we need to retransmit them. - auto it = packet_arrival_times_.find(window_start_seq_); - RTC_DCHECK(it != packet_arrival_times_.end()); + rtc::CritScope cs(&lock_); + auto it = packet_arrival_times_.lower_bound(window_start_seq_); + if (it == packet_arrival_times_.end()) { + // Feedback for all packets already sent. + return false; + } // TODO(sprang): Measure receive times in microseconds and remove the // conversions below. + const int64_t first_sequence = it->first; feedback_packet->WithMediaSourceSsrc(media_ssrc_); - feedback_packet->WithBase(static_cast(it->first & 0xFFFF), + // Base sequence is the expected next (window_start_seq_). This is known, but + // we might not have actually received it, so the base time shall be the time + // of the first received packet in the feedback. + feedback_packet->WithBase(static_cast(window_start_seq_ & 0xFFFF), it->second * 1000); feedback_packet->WithFeedbackSequenceNumber(feedback_sequence_++); for (; it != packet_arrival_times_.end(); ++it) { @@ -135,19 +143,18 @@ bool RemoteEstimatorProxy::BuildFeedbackPacket( static_cast(it->first & 0xFFFF), it->second * 1000)) { // If we can't even add the first seq to the feedback packet, we won't be // able to build it at all. - RTC_CHECK_NE(window_start_seq_, it->first); + RTC_CHECK_NE(first_sequence, it->first); // Could not add timestamp, feedback packet might be full. Return and // try again with a fresh packet. - window_start_seq_ = it->first; break; } + // Note: Don't erase items from packet_arrival_times_ after sending, in case // they need to be re-sent after a reordering. Removal will be handled // by OnPacketArrival once packets are too old. + window_start_seq_ = it->first + 1; } - if (it == packet_arrival_times_.end()) - window_start_seq_ = -1; return true; } diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index a1264b2ff9..62dd2c3956 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -19,6 +19,7 @@ using ::testing::_; using ::testing::InSequence; using ::testing::Invoke; +using ::testing::Return; namespace webrtc { @@ -107,6 +108,40 @@ TEST_F(RemoteEstimatorProxyTest, DuplicatedPackets) { Process(); } +TEST_F(RemoteEstimatorProxyTest, FeedbackWithMissingStart) { + // First feedback. + IncomingPacket(kBaseSeq, kBaseTimeMs); + IncomingPacket(kBaseSeq + 1, kBaseTimeMs + 1000); + EXPECT_CALL(router_, SendFeedback(_)).Times(1).WillOnce(Return(true)); + Process(); + + // Second feedback starts with a missing packet (DROP kBaseSeq + 2). + IncomingPacket(kBaseSeq + 3, kBaseTimeMs + 3000); + + EXPECT_CALL(router_, SendFeedback(_)) + .Times(1) + .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) { + packet->Build(); + EXPECT_EQ(kBaseSeq + 2, packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, packet->GetMediaSourceSsrc()); + + std::vector status_vec = + packet->GetStatusVector(); + EXPECT_EQ(2u, status_vec.size()); + EXPECT_EQ(rtcp::TransportFeedback::StatusSymbol::kNotReceived, + status_vec[0]); + EXPECT_EQ(rtcp::TransportFeedback::StatusSymbol::kReceivedSmallDelta, + status_vec[1]); + std::vector delta_vec = packet->GetReceiveDeltasUs(); + EXPECT_EQ(1u, delta_vec.size()); + EXPECT_EQ(kBaseTimeMs + 3000, + (packet->GetBaseTimeUs() + delta_vec[0]) / 1000); + return true; + })); + + Process(); +} + TEST_F(RemoteEstimatorProxyTest, SendsFeedbackWithVaryingDeltas) { IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kMaxSmallDeltaMs); 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 5cdaa3aaa4..356263e96f 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc @@ -317,7 +317,11 @@ void TransportFeedback::WithBase(uint16_t base_sequence, RTC_DCHECK_EQ(-1, base_seq_); RTC_DCHECK_NE(-1, ref_timestamp_us); base_seq_ = base_sequence; - last_seq_ = base_sequence; + // last_seq_ is the sequence number of the last packed added _before_ a call + // to WithReceivedPacket(). Since the first sequence to be added is + // base_sequence, we need this to be one lower in order for potential missing + // packets to be populated properly. + last_seq_ = base_sequence - 1; base_time_ = ref_timestamp_us / kBaseScaleFactor; last_timestamp_ = base_time_ * kBaseScaleFactor; } 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 203d70fab1..07f9049c85 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 @@ -359,15 +359,18 @@ TEST(RtcpPacketTest, TransportFeedback_Limits) { // Sequence number wrap above 0x8000. std::unique_ptr packet(new TransportFeedback()); packet->WithBase(0, 0); + EXPECT_TRUE(packet->WithReceivedPacket(0x0, 0)); EXPECT_TRUE(packet->WithReceivedPacket(0x8000, 1000)); packet.reset(new TransportFeedback()); packet->WithBase(0, 0); + EXPECT_TRUE(packet->WithReceivedPacket(0x0, 0)); EXPECT_FALSE(packet->WithReceivedPacket(0x8000 + 1, 1000)); // Packet status count max 0xFFFF. packet.reset(new TransportFeedback()); packet->WithBase(0, 0); + EXPECT_TRUE(packet->WithReceivedPacket(0x0, 0)); EXPECT_TRUE(packet->WithReceivedPacket(0x8000, 1000)); EXPECT_TRUE(packet->WithReceivedPacket(0xFFFF, 2000)); EXPECT_FALSE(packet->WithReceivedPacket(0, 3000));