Make recv_deltas optional in TransportFeedback packets
Bug: webrtc:10263
Change-Id: I49c4a4710a5c34a62b53080e708c310a8484831b
Reviewed-on: https://webrtc-review.googlesource.com/c/122543
Commit-Queue: Johannes Kron <kron@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26687}diff --git a/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc b/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc
index 81eda9e..ee8b93a 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 @@
}
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 @@
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 @@
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<int16_t>(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<int16_t>(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 @@
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 @@
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<int16_t>::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<int16_t>::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;
}
- ++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;
+ }
}
size_bytes_ = RtcpPacket::kHeaderLength + index;
RTC_DCHECK_LE(index, end_index);
@@ -495,7 +527,9 @@
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 @@
*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<int16_t>::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<int16_t>::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 b7bed9d..174ef6b 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 @@
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 @@
// 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<TransportFeedback> ParseFrom(const uint8_t* buffer,
size_t length);
@@ -141,6 +148,7 @@
uint16_t num_seq_no_;
int32_t base_time_ticks_;
uint8_t feedback_seq_;
+ bool include_timestamps_;
int64_t last_timestamp_us_;
std::vector<ReceivedPacket> 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 43eeeb9..0bb2d47 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 @@
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 @@
void WithInput(const uint16_t received_seq[],
const int64_t received_ts[],
uint16_t length) {
- std::unique_ptr<int64_t[]> temp_deltas;
+ std::unique_ptr<int64_t[]> 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 @@
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 @@
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 @@
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 @@
int64_t default_delta_;
std::unique_ptr<TransportFeedback> 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.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);
