Avoid wrong parsing of padding length and its use in NetEq simulation.
Bug: b/113648474, webrtc:9730
Change-Id: Ieff7ab8697f5c8742548897a9b452a20b0bd2e7c
Reviewed-on: https://webrtc-review.googlesource.com/98461
Commit-Queue: Minyue Li <minyue@webrtc.org>
Reviewed-by: Henrik Lundin <henrik.lundin@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24703}diff --git a/logging/rtc_event_log/rtc_event_log_parser_new.cc b/logging/rtc_event_log/rtc_event_log_parser_new.cc
index bb5b1b2..9fbf0d0 100644
--- a/logging/rtc_event_log/rtc_event_log_parser_new.cc
+++ b/logging/rtc_event_log/rtc_event_log_parser_new.cc
@@ -584,6 +584,7 @@
event, &direction, header, &header_length, &total_length, nullptr);
RtpUtility::RtpHeaderParser rtp_parser(header, header_length);
RTPHeader parsed_header;
+
if (extension_map != nullptr) {
rtp_parser.Parse(&parsed_header, extension_map);
} else {
@@ -595,6 +596,15 @@
// Tracking bug: webrtc:6399
rtp_parser.Parse(&parsed_header, &default_extension_map_);
}
+
+ // Since we give the parser only a header, there is no way for it to know
+ // the padding length. The best solution would be to log the padding
+ // length in RTC event log. In absence of it, we assume the RTP packet to
+ // contain only padding, if the padding bit is set.
+ // TODO(webrtc:9730): Use a generic way to obtain padding length.
+ if ((header[0] & 0x20) != 0)
+ parsed_header.paddingLength = total_length - header_length;
+
RTC_CHECK(event.has_timestamp_us());
uint64_t timestamp_us = event.timestamp_us();
if (direction == kIncomingPacket) {
diff --git a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc
index 273539c..7a6573f 100644
--- a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc
+++ b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc
@@ -262,7 +262,8 @@
}
void EventGenerator::RandomizeRtpPacket(
- size_t packet_size,
+ size_t payload_size,
+ size_t padding_size,
uint32_t ssrc,
const RtpHeaderExtensionMap& extension_map,
RtpPacket* rtp_packet) {
@@ -291,27 +292,40 @@
if (extension_map.IsRegistered(TransportSequenceNumber::kId))
rtp_packet->SetExtension<TransportSequenceNumber>(prng_.Rand<uint16_t>());
- RTC_DCHECK_GE(packet_size, rtp_packet->headers_size());
- size_t payload_size = packet_size - rtp_packet->headers_size();
RTC_CHECK_LE(rtp_packet->headers_size() + payload_size, IP_PACKET_SIZE);
+
uint8_t* payload = rtp_packet->AllocatePayload(payload_size);
RTC_DCHECK(payload != nullptr);
for (size_t i = 0; i < payload_size; i++) {
payload[i] = prng_.Rand<uint8_t>();
}
+ RTC_CHECK(rtp_packet->SetPadding(padding_size, &prng_));
}
std::unique_ptr<RtcEventRtpPacketIncoming> EventGenerator::NewRtpPacketIncoming(
uint32_t ssrc,
const RtpHeaderExtensionMap& extension_map) {
+ constexpr size_t kMaxPaddingLength = 224;
+ const bool padding = prng_.Rand(0, 9) == 0; // Let padding be 10% probable.
+ const size_t padding_size = !padding ? 0u : prng_.Rand(0u, kMaxPaddingLength);
+
// 12 bytes RTP header, 4 bytes for 0xBEDE + alignment, 4 bytes per CSRC.
constexpr size_t kMaxHeaderSize =
16 + 4 * kMaxCsrcs + kMaxExtensionSizeBytes * kMaxNumExtensions;
- size_t packet_size =
- prng_.Rand(kMaxHeaderSize, static_cast<uint32_t>(IP_PACKET_SIZE - 1));
+
+ // In principle, a packet can contain both padding and other payload.
+ // Currently, RTC eventlog encoder-parser can only maintain padding length if
+ // packet is full padding.
+ // TODO(webrtc:9730): Remove the deterministic logic for padding_size > 0.
+ size_t payload_size =
+ padding_size > 0 ? 0
+ : prng_.Rand(0u, static_cast<uint32_t>(IP_PACKET_SIZE -
+ 1 - padding_size -
+ kMaxHeaderSize));
RtpPacketReceived rtp_packet(&extension_map);
- RandomizeRtpPacket(packet_size, ssrc, extension_map, &rtp_packet);
+ RandomizeRtpPacket(payload_size, padding_size, ssrc, extension_map,
+ &rtp_packet);
return absl::make_unique<RtcEventRtpPacketIncoming>(rtp_packet);
}
@@ -319,14 +333,28 @@
std::unique_ptr<RtcEventRtpPacketOutgoing> EventGenerator::NewRtpPacketOutgoing(
uint32_t ssrc,
const RtpHeaderExtensionMap& extension_map) {
+ constexpr size_t kMaxPaddingLength = 224;
+ const bool padding = prng_.Rand(0, 9) == 0; // Let padding be 10% probable.
+ const size_t padding_size = !padding ? 0u : prng_.Rand(0u, kMaxPaddingLength);
+
// 12 bytes RTP header, 4 bytes for 0xBEDE + alignment, 4 bytes per CSRC.
constexpr size_t kMaxHeaderSize =
16 + 4 * kMaxCsrcs + kMaxExtensionSizeBytes * kMaxNumExtensions;
- size_t packet_size =
- prng_.Rand(kMaxHeaderSize, static_cast<uint32_t>(IP_PACKET_SIZE - 1));
- RtpPacketToSend rtp_packet(&extension_map, packet_size);
- RandomizeRtpPacket(packet_size, ssrc, extension_map, &rtp_packet);
+ // In principle,a packet can contain both padding and other payload.
+ // Currently, RTC eventlog encoder-parser can only maintain padding length if
+ // packet is full padding.
+ // TODO(webrtc:9730): Remove the deterministic logic for padding_size > 0.
+ size_t payload_size =
+ padding_size > 0 ? 0
+ : prng_.Rand(0u, static_cast<uint32_t>(IP_PACKET_SIZE -
+ 1 - padding_size -
+ kMaxHeaderSize));
+
+ RtpPacketToSend rtp_packet(&extension_map,
+ kMaxHeaderSize + payload_size + padding_size);
+ RandomizeRtpPacket(payload_size, padding_size, ssrc, extension_map,
+ &rtp_packet);
int probe_cluster_id = prng_.Rand(0, 100000);
return absl::make_unique<RtcEventRtpPacketOutgoing>(rtp_packet,
@@ -619,8 +647,6 @@
return false;
}
- if (original_header.padding_size() != logged_header.paddingLength)
- return false;
if (original_header.headers_size() != logged_header.headerLength)
return false;
@@ -698,6 +724,16 @@
if (original_event.packet_length_ != logged_event.rtp.total_length)
return false;
+ if ((original_event.header_.data()[0] & 0x20) != 0 && // has padding
+ original_event.packet_length_ - original_event.header_.headers_size() !=
+ logged_event.rtp.header.paddingLength) {
+ // Currently, RTC eventlog encoder-parser can only maintain padding length
+ // if packet is full padding.
+ // TODO(webrtc:9730): Change the condition to something like
+ // original_event.padding_length_ != logged_event.rtp.header.paddingLength.
+ return false;
+ }
+
if (!VerifyLoggedRtpHeader(original_event.header_, logged_event.rtp.header))
return false;
@@ -716,6 +752,16 @@
if (original_event.packet_length_ != logged_event.rtp.total_length)
return false;
+ if ((original_event.header_.data()[0] & 0x20) != 0 && // has padding
+ original_event.packet_length_ - original_event.header_.headers_size() !=
+ logged_event.rtp.header.paddingLength) {
+ // Currently, RTC eventlog encoder-parser can only maintain padding length
+ // if packet is full padding.
+ // TODO(webrtc:9730): Change the condition to something like
+ // original_event.padding_length_ != logged_event.rtp.header.paddingLength.
+ return false;
+ }
+
// TODO(terelius): Probe cluster ID isn't parsed, used or tested. Unless
// someone has a strong reason to keep it, it'll be removed.
diff --git a/logging/rtc_event_log/rtc_event_log_unittest_helper.h b/logging/rtc_event_log/rtc_event_log_unittest_helper.h
index d67469c..f4d1876 100644
--- a/logging/rtc_event_log/rtc_event_log_unittest_helper.h
+++ b/logging/rtc_event_log/rtc_event_log_unittest_helper.h
@@ -70,7 +70,8 @@
std::unique_ptr<RtcEventRtcpPacketOutgoing> NewRtcpPacketOutgoing();
- void RandomizeRtpPacket(size_t packet_size,
+ void RandomizeRtpPacket(size_t payload_size,
+ size_t padding_size,
uint32_t ssrc,
const RtpHeaderExtensionMap& extension_map,
RtpPacket* rtp_packet);
diff --git a/modules/audio_coding/neteq/tools/neteq_replacement_input.cc b/modules/audio_coding/neteq/tools/neteq_replacement_input.cc
index 362eb89..ffd114a 100644
--- a/modules/audio_coding/neteq/tools/neteq_replacement_input.cc
+++ b/modules/audio_coding/neteq/tools/neteq_replacement_input.cc
@@ -42,7 +42,15 @@
std::unique_ptr<NetEqInput::PacketData> NetEqReplacementInput::PopPacket() {
std::unique_ptr<PacketData> to_return = std::move(packet_);
- packet_ = source_->PopPacket();
+ while (true) {
+ packet_ = source_->PopPacket();
+ if (!packet_)
+ break;
+ if (packet_->payload.size() > packet_->header.paddingLength) {
+ // Not padding only. Good to go. Skip this packet otherwise.
+ break;
+ }
+ }
ReplacePacket();
return to_return;
}
diff --git a/modules/audio_coding/neteq/tools/neteq_test.cc b/modules/audio_coding/neteq/tools/neteq_test.cc
index ff60048..e51ee47 100644
--- a/modules/audio_coding/neteq/tools/neteq_test.cc
+++ b/modules/audio_coding/neteq/tools/neteq_test.cc
@@ -95,16 +95,23 @@
if (input_->NextPacketTime() && time_now_ms >= *input_->NextPacketTime()) {
std::unique_ptr<NetEqInput::PacketData> packet_data = input_->PopPacket();
RTC_CHECK(packet_data);
- int error = neteq_->InsertPacket(
- packet_data->header,
- rtc::ArrayView<const uint8_t>(packet_data->payload),
- static_cast<uint32_t>(packet_data->time_ms * sample_rate_hz_ / 1000));
- if (error != NetEq::kOK && callbacks_.error_callback) {
- callbacks_.error_callback->OnInsertPacketError(*packet_data);
- }
- if (callbacks_.post_insert_packet) {
- callbacks_.post_insert_packet->AfterInsertPacket(*packet_data,
- neteq_.get());
+ const size_t payload_data_length =
+ packet_data->payload.size() - packet_data->header.paddingLength;
+ if (payload_data_length != 0) {
+ int error = neteq_->InsertPacket(
+ packet_data->header,
+ rtc::ArrayView<const uint8_t>(packet_data->payload),
+ static_cast<uint32_t>(packet_data->time_ms * sample_rate_hz_ /
+ 1000));
+ if (error != NetEq::kOK && callbacks_.error_callback) {
+ callbacks_.error_callback->OnInsertPacketError(*packet_data);
+ }
+ if (callbacks_.post_insert_packet) {
+ callbacks_.post_insert_packet->AfterInsertPacket(*packet_data,
+ neteq_.get());
+ }
+ } else {
+ neteq_->InsertEmptyPacket(packet_data->header);
}
if (last_packet_time_ms_) {
current_state_.packet_iat_ms.push_back(time_now_ms -
diff --git a/modules/rtp_rtcp/source/rtp_utility.cc b/modules/rtp_rtcp/source/rtp_utility.cc
index 5f5b3dc0..228572f 100644
--- a/modules/rtp_rtcp/source/rtp_utility.cc
+++ b/modules/rtp_rtcp/source/rtp_utility.cc
@@ -197,7 +197,9 @@
header->timestamp = RTPTimestamp;
header->ssrc = SSRC;
header->numCSRCs = CC;
- header->paddingLength = P ? *(_ptrRTPDataEnd - 1) : 0;
+ if (!P) {
+ header->paddingLength = 0;
+ }
for (uint8_t i = 0; i < CC; ++i) {
uint32_t CSRC = ByteReader<uint32_t>::ReadBigEndian(ptr);
@@ -276,6 +278,21 @@
}
header->headerLength += XLen;
}
+ if (header->headerLength > static_cast<size_t>(length))
+ return false;
+
+ if (P) {
+ // Packet has padding.
+ if (header->headerLength != static_cast<size_t>(length)) {
+ // Packet is not header only. We can parse padding length now.
+ header->paddingLength = *(_ptrRTPDataEnd - 1);
+ } else {
+ RTC_LOG(LS_WARNING) << "Cannot parse padding length.";
+ // Packet is header only. We have no clue of the padding length.
+ return false;
+ }
+ }
+
if (header->headerLength + header->paddingLength >
static_cast<size_t>(length))
return false;
