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}

logging/rtc_event_log/rtc_event_log_parser_new.cc

@@ -584,6 +584,7 @@ void ParsedRtcEventLogNew::StoreParsedEvent(const rtclog::Event& event) {
           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 @@ void ParsedRtcEventLogNew::StoreParsedEvent(const rtclog::Event& event) {
         //             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) {

logging/rtc_event_log/rtc_event_log_unittest_helper.cc

@@ -262,7 +262,8 @@ EventGenerator::NewRtcpPacketOutgoing() {
 }
 
 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 @@ void EventGenerator::RandomizeRtpPacket(
   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<RtcEventRtpPacketIncoming> EventGenerator::NewRtpPacketIncoming(
 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);
+  // In principle,a packet can contain both padding and other payload.
-  RandomizeRtpPacket(packet_size, ssrc, extension_map, &rtp_packet);
+  // 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 @@ bool VerifyLoggedRtpHeader(const RtpPacket& original_header,
       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 @@ bool VerifyLoggedRtpPacketIncoming(
   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 @@ bool VerifyLoggedRtpPacketOutgoing(
   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.
 

logging/rtc_event_log/rtc_event_log_unittest_helper.h

@@ -70,7 +70,8 @@ class EventGenerator {
 
   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);

modules/audio_coding/neteq/tools/neteq_replacement_input.cc

@@ -42,7 +42,15 @@ absl::optional<int64_t> NetEqReplacementInput::NextOutputEventTime() const {
 
 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;
 }

modules/audio_coding/neteq/tools/neteq_test.cc

@@ -95,16 +95,23 @@ NetEqTest::SimulationStepResult NetEqTest::RunToNextGetAudio() {
     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(
+      const size_t payload_data_length =
-          packet_data->header,
+          packet_data->payload.size() - packet_data->header.paddingLength;
-          rtc::ArrayView<const uint8_t>(packet_data->payload),
+      if (payload_data_length != 0) {
-          static_cast<uint32_t>(packet_data->time_ms * sample_rate_hz_ / 1000));
+        int error = neteq_->InsertPacket(
-      if (error != NetEq::kOK && callbacks_.error_callback) {
+            packet_data->header,
-        callbacks_.error_callback->OnInsertPacketError(*packet_data);
+            rtc::ArrayView<const uint8_t>(packet_data->payload),
-      }
+            static_cast<uint32_t>(packet_data->time_ms * sample_rate_hz_ /
-      if (callbacks_.post_insert_packet) {
+                                  1000));
-        callbacks_.post_insert_packet->AfterInsertPacket(*packet_data,
+        if (error != NetEq::kOK && callbacks_.error_callback) {
-                                                         neteq_.get());
+          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 -

modules/rtp_rtcp/source/rtp_utility.cc

@@ -197,7 +197,9 @@ bool RtpHeaderParser::Parse(
   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 @@ bool RtpHeaderParser::Parse(
     }
     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;