diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc index 81eda9e5ce..ee8b93a6d7 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc @@ -262,10 +262,14 @@ void TransportFeedback::LastChunk::DecodeRunLength(uint16_t chunk, } TransportFeedback::TransportFeedback() + : TransportFeedback(/*include_timestamps=*/true) {} + +TransportFeedback::TransportFeedback(bool include_timestamps) : base_seq_no_(0), num_seq_no_(0), base_time_ticks_(0), feedback_seq_(0), + include_timestamps_(include_timestamps), last_timestamp_us_(0), size_bytes_(kTransportFeedbackHeaderSizeBytes) {} @@ -276,6 +280,7 @@ TransportFeedback::TransportFeedback(TransportFeedback&& other) num_seq_no_(other.num_seq_no_), base_time_ticks_(other.base_time_ticks_), feedback_seq_(other.feedback_seq_), + include_timestamps_(other.include_timestamps_), last_timestamp_us_(other.last_timestamp_us_), packets_(std::move(other.packets_)), encoded_chunks_(std::move(other.encoded_chunks_)), @@ -301,19 +306,25 @@ void TransportFeedback::SetFeedbackSequenceNumber(uint8_t feedback_sequence) { bool TransportFeedback::AddReceivedPacket(uint16_t sequence_number, int64_t timestamp_us) { - // Convert to ticks and round. - int64_t delta_full = (timestamp_us - last_timestamp_us_) % kTimeWrapPeriodUs; - if (delta_full > kTimeWrapPeriodUs / 2) - delta_full -= kTimeWrapPeriodUs; - delta_full += - delta_full < 0 ? -(kDeltaScaleFactor / 2) : kDeltaScaleFactor / 2; - delta_full /= kDeltaScaleFactor; + // Set delta to zero if timestamps are not included, this will simplify the + // encoding process. + int16_t delta = 0; + if (include_timestamps_) { + // Convert to ticks and round. + int64_t delta_full = + (timestamp_us - last_timestamp_us_) % kTimeWrapPeriodUs; + if (delta_full > kTimeWrapPeriodUs / 2) + delta_full -= kTimeWrapPeriodUs; + delta_full += + delta_full < 0 ? -(kDeltaScaleFactor / 2) : kDeltaScaleFactor / 2; + delta_full /= kDeltaScaleFactor; - int16_t delta = static_cast(delta_full); - // If larger than 16bit signed, we can't represent it - need new fb packet. - if (delta != delta_full) { - RTC_LOG(LS_WARNING) << "Delta value too large ( >= 2^16 ticks )"; - return false; + delta = static_cast(delta_full); + // If larger than 16bit signed, we can't represent it - need new fb packet. + if (delta != delta_full) { + RTC_LOG(LS_WARNING) << "Delta value too large ( >= 2^16 ticks )"; + return false; + } } uint16_t next_seq_no = base_seq_no_ + num_seq_no_; @@ -332,7 +343,9 @@ bool TransportFeedback::AddReceivedPacket(uint16_t sequence_number, packets_.emplace_back(sequence_number, delta); last_timestamp_us_ += delta * kDeltaScaleFactor; - size_bytes_ += delta_size; + if (include_timestamps_) { + size_bytes_ += delta_size; + } return true; } @@ -400,38 +413,57 @@ bool TransportFeedback::Parse(const CommonHeader& packet) { num_seq_no_ = status_count; uint16_t seq_no = base_seq_no_; + size_t recv_delta_size = 0; for (size_t delta_size : delta_sizes) { - if (index + delta_size > end_index) { - RTC_LOG(LS_WARNING) << "Buffer overflow while parsing packet."; - Clear(); - return false; - } - switch (delta_size) { - case 0: - break; - case 1: { - int16_t delta = payload[index]; - packets_.emplace_back(seq_no, delta); - last_timestamp_us_ += delta * kDeltaScaleFactor; - index += delta_size; - break; - } - case 2: { - int16_t delta = ByteReader::ReadBigEndian(&payload[index]); - packets_.emplace_back(seq_no, delta); - last_timestamp_us_ += delta * kDeltaScaleFactor; - index += delta_size; - break; - } - case 3: + recv_delta_size += delta_size; + } + + // Determine if timestamps, that is, recv_delta are included in the packet. + if (end_index >= index + recv_delta_size) { + for (size_t delta_size : delta_sizes) { + if (index + delta_size > end_index) { + RTC_LOG(LS_WARNING) << "Buffer overflow while parsing packet."; Clear(); - RTC_LOG(LS_WARNING) << "Invalid delta_size for seq_no " << seq_no; return false; - default: - RTC_NOTREACHED(); - break; + } + switch (delta_size) { + case 0: + break; + case 1: { + int16_t delta = payload[index]; + packets_.emplace_back(seq_no, delta); + last_timestamp_us_ += delta * kDeltaScaleFactor; + index += delta_size; + break; + } + case 2: { + int16_t delta = ByteReader::ReadBigEndian(&payload[index]); + packets_.emplace_back(seq_no, delta); + last_timestamp_us_ += delta * kDeltaScaleFactor; + index += delta_size; + break; + } + case 3: + Clear(); + RTC_LOG(LS_WARNING) << "Invalid delta_size for seq_no " << seq_no; + + return false; + default: + RTC_NOTREACHED(); + break; + } + ++seq_no; + } + } else { + // The packet does not contain receive deltas. + include_timestamps_ = false; + for (size_t delta_size : delta_sizes) { + // Use delta sizes to detect if packet was received. + if (delta_size > 0) { + packets_.emplace_back(seq_no, 0); + } + ++seq_no; } - ++seq_no; } size_bytes_ = RtcpPacket::kHeaderLength + index; RTC_DCHECK_LE(index, end_index); @@ -495,7 +527,9 @@ bool TransportFeedback::IsConsistent() const { timestamp_us += packet_it->delta_us(); ++packet_it; } - packet_size += delta_size; + if (include_timestamps_) { + packet_size += delta_size; + } ++seq_no; } if (packet_it != packets_.end()) { @@ -566,13 +600,15 @@ bool TransportFeedback::Create(uint8_t* packet, *position += 2; } - for (const auto& received_packet : packets_) { - int16_t delta = received_packet.delta_ticks(); - if (delta >= 0 && delta <= 0xFF) { - packet[(*position)++] = delta; - } else { - ByteWriter::WriteBigEndian(&packet[*position], delta); - *position += 2; + if (include_timestamps_) { + for (const auto& received_packet : packets_) { + int16_t delta = received_packet.delta_ticks(); + if (delta >= 0 && delta <= 0xFF) { + packet[(*position)++] = delta; + } else { + ByteWriter::WriteBigEndian(&packet[*position], delta); + *position += 2; + } } } diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h index b7bed9d21a..174ef6bcb5 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h @@ -45,6 +45,10 @@ class TransportFeedback : public Rtpfb { static constexpr size_t kMaxReportedPackets = 0xffff; TransportFeedback(); + explicit TransportFeedback( + bool include_timestamps); // If |include_timestamps| is set to false, the + // created packet will not contain the receive + // delta block. TransportFeedback(const TransportFeedback&); TransportFeedback(TransportFeedback&&); @@ -65,6 +69,9 @@ class TransportFeedback : public Rtpfb { // Get the reference time in microseconds, including any precision loss. int64_t GetBaseTimeUs() const; + // Does the feedback packet contain timestamp information? + bool IncludeTimestamps() const { return include_timestamps_; } + bool Parse(const CommonHeader& packet); static std::unique_ptr ParseFrom(const uint8_t* buffer, size_t length); @@ -141,6 +148,7 @@ class TransportFeedback : public Rtpfb { uint16_t num_seq_no_; int32_t base_time_ticks_; uint8_t feedback_seq_; + bool include_timestamps_; int64_t last_timestamp_us_; std::vector packets_; diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc index 43eeeb9b36..0bb2d475f5 100644 --- a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc @@ -33,9 +33,11 @@ static const int64_t kDeltaLimit = 0xFF * TransportFeedback::kDeltaScaleFactor; class FeedbackTester { public: - FeedbackTester() + FeedbackTester() : FeedbackTester(true) {} + explicit FeedbackTester(bool include_timestamps) : expected_size_(kAnySize), - default_delta_(TransportFeedback::kDeltaScaleFactor * 4) {} + default_delta_(TransportFeedback::kDeltaScaleFactor * 4), + include_timestamps_(include_timestamps) {} void WithExpectedSize(size_t expected_size) { expected_size_ = expected_size; @@ -46,16 +48,16 @@ class FeedbackTester { void WithInput(const uint16_t received_seq[], const int64_t received_ts[], uint16_t length) { - std::unique_ptr temp_deltas; + std::unique_ptr temp_timestamps; if (received_ts == nullptr) { - temp_deltas.reset(new int64_t[length]); - GenerateDeltas(received_seq, length, temp_deltas.get()); - received_ts = temp_deltas.get(); + temp_timestamps.reset(new int64_t[length]); + GenerateReceiveTimestamps(received_seq, length, temp_timestamps.get()); + received_ts = temp_timestamps.get(); } expected_seq_.clear(); expected_deltas_.clear(); - feedback_.reset(new TransportFeedback()); + feedback_.reset(new TransportFeedback(include_timestamps_)); feedback_->SetBase(received_seq[0], received_ts[0]); ASSERT_TRUE(feedback_->IsConsistent()); @@ -81,8 +83,9 @@ class FeedbackTester { VerifyInternal(); feedback_ = TransportFeedback::ParseFrom(serialized_.data(), serialized_.size()); - ASSERT_TRUE(feedback_->IsConsistent()); ASSERT_NE(nullptr, feedback_); + ASSERT_TRUE(feedback_->IsConsistent()); + EXPECT_EQ(include_timestamps_, feedback_->IncludeTimestamps()); VerifyInternal(); } @@ -104,12 +107,14 @@ class FeedbackTester { actual_deltas_us.push_back(packet.delta_us()); } EXPECT_THAT(actual_seq_nos, ElementsAreArray(expected_seq_)); - EXPECT_THAT(actual_deltas_us, ElementsAreArray(expected_deltas_)); + if (include_timestamps_) { + EXPECT_THAT(actual_deltas_us, ElementsAreArray(expected_deltas_)); + } } - void GenerateDeltas(const uint16_t seq[], - const size_t length, - int64_t* deltas) { + void GenerateReceiveTimestamps(const uint16_t seq[], + const size_t length, + int64_t* timestamps) { uint16_t last_seq = seq[0]; int64_t offset = 0; @@ -118,7 +123,7 @@ class FeedbackTester { offset += 0x10000 * default_delta_; last_seq = seq[i]; - deltas[i] = offset + (last_seq * default_delta_); + timestamps[i] = offset + (last_seq * default_delta_); } } @@ -128,8 +133,17 @@ class FeedbackTester { int64_t default_delta_; std::unique_ptr feedback_; rtc::Buffer serialized_; + bool include_timestamps_; }; +// The following tests use FeedbackTester that simulates received packets as +// specified by the parameters |received_seq[]| and |received_ts[]| (optional). +// The following is verified in these tests: +// - Expected size of serialized packet. +// - Expected sequence numbers and receive deltas. +// - Sequence numbers and receive deltas are persistent after serialization +// followed by parsing. +// - The internal state of a feedback packet is consistent. TEST(RtcpPacketTest, TransportFeedbackOneBitVector) { const uint16_t kReceived[] = {1, 2, 7, 8, 9, 10, 13}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); @@ -142,6 +156,17 @@ TEST(RtcpPacketTest, TransportFeedbackOneBitVector) { test.VerifyPacket(); } +TEST(RtcpPacketTest, TransportFeedbackOneBitVectorNoRecvDelta) { + const uint16_t kReceived[] = {1, 2, 7, 8, 9, 10, 13}; + const size_t kLength = sizeof(kReceived) / sizeof(uint16_t); + const size_t kExpectedSizeBytes = kHeaderSize + kStatusChunkSize; + + FeedbackTester test(/*include_timestamps=*/false); + test.WithExpectedSize(kExpectedSizeBytes); + test.WithInput(kReceived, nullptr, kLength); + test.VerifyPacket(); +} + TEST(RtcpPacketTest, TransportFeedbackFullOneBitVector) { const uint16_t kReceived[] = {1, 2, 7, 8, 9, 10, 13, 14}; const size_t kLength = sizeof(kReceived) / sizeof(uint16_t);