Changed LogRtpHeader to read the header length from the packet instead of requiring an extra argument.

BUG=

Review URL: https://codereview.webrtc.org/1257163003

Cr-Commit-Position: refs/heads/master@{#9856}

webrtc/video/rtc_event_log.cc

@@ -16,6 +16,7 @@
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/call.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/system_wrappers/interface/clock.h"
 #include "webrtc/system_wrappers/interface/file_wrapper.h"
 
@@ -44,8 +45,7 @@ class RtcEventLogImpl final : public RtcEventLog {
   void LogRtpHeader(bool incoming,
                     MediaType media_type,
                     const uint8_t* header,
-                    size_t header_length,
+                    size_t packet_length) override {}
-                    size_t total_length) override {}
   void LogRtcpPacket(bool incoming,
                      MediaType media_type,
                      const uint8_t* packet,
@@ -67,8 +67,7 @@ class RtcEventLogImpl final : public RtcEventLog {
   void LogRtpHeader(bool incoming,
                     MediaType media_type,
                     const uint8_t* header,
-                    size_t header_length,
+                    size_t packet_length) override;
-                    size_t total_length) override;
   void LogRtcpPacket(bool incoming,
                      MediaType media_type,
                      const uint8_t* packet,
@@ -284,13 +283,26 @@ void RtcEventLogImpl::LogVideoSendStreamConfig(
   HandleEvent(&event);
 }
 
-// TODO(terelius): It is more convenient and less error prone to parse the
-// header length from the packet instead of relying on the caller to provide it.
 void RtcEventLogImpl::LogRtpHeader(bool incoming,
                                    MediaType media_type,
                                    const uint8_t* header,
-                                   size_t header_length,
+                                   size_t packet_length) {
-                                   size_t total_length) {
+  // Read header length (in bytes) from packet data.
+  if (packet_length < 12u) {
+    return;  // Don't read outside the packet.
+  }
+  const bool x = (header[0] & 0x10) != 0;
+  const uint8_t cc = header[0] & 0x0f;
+  size_t header_length = 12u + cc * 4u;
+
+  if (x) {
+    if (packet_length < 12u + cc * 4u + 4u) {
+      return;  // Don't read outside the packet.
+    }
+    size_t x_len = ByteReader<uint16_t>::ReadBigEndian(header + 14 + cc * 4);
+    header_length += (x_len + 1) * 4;
+  }
+
   rtc::CritScope lock(&crit_);
   rtclog::Event rtp_event;
   const int64_t timestamp = clock_->TimeInMicroseconds();
@@ -298,7 +310,7 @@ void RtcEventLogImpl::LogRtpHeader(bool incoming,
   rtp_event.set_type(rtclog::Event::RTP_EVENT);
   rtp_event.mutable_rtp_packet()->set_incoming(incoming);
   rtp_event.mutable_rtp_packet()->set_type(ConvertMediaType(media_type));
-  rtp_event.mutable_rtp_packet()->set_packet_length(total_length);
+  rtp_event.mutable_rtp_packet()->set_packet_length(packet_length);
   rtp_event.mutable_rtp_packet()->set_header(header, header_length);
   HandleEvent(&rtp_event);
 }

webrtc/video/rtc_event_log.h

@@ -54,12 +54,12 @@ class RtcEventLog {
   virtual void LogVideoSendStreamConfig(
       const webrtc::VideoSendStream::Config& config) = 0;
 
-  // Logs the header of an incoming or outgoing RTP packet.
+  // Logs the header of an incoming or outgoing RTP packet. packet_length
+  // is the total length of the packet, including both header and payload.
   virtual void LogRtpHeader(bool incoming,
                             MediaType media_type,
                             const uint8_t* header,
-                            size_t header_length,
+                            size_t packet_length) = 0;
-                            size_t total_length) = 0;
 
   // Logs an incoming or outgoing RTCP packet.
   virtual void LogRtcpPacket(bool incoming,

webrtc/video/rtc_event_log_unittest.cc

@@ -15,9 +15,11 @@
 #include <vector>
 
 #include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/base/buffer.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/scoped_ptr.h"
 #include "webrtc/call.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_sender.h"
 #include "webrtc/system_wrappers/interface/clock.h"
 #include "webrtc/test/test_suite.h"
 #include "webrtc/test/testsupport/fileutils.h"
@@ -33,6 +35,23 @@
 
 namespace webrtc {
 
+namespace {
+
+const RTPExtensionType kExtensionTypes[] = {
+    RTPExtensionType::kRtpExtensionTransmissionTimeOffset,
+    RTPExtensionType::kRtpExtensionAudioLevel,
+    RTPExtensionType::kRtpExtensionAbsoluteSendTime,
+    RTPExtensionType::kRtpExtensionVideoRotation,
+    RTPExtensionType::kRtpExtensionTransportSequenceNumber};
+const char* kExtensionNames[] = {RtpExtension::kTOffset,
+                                 RtpExtension::kAudioLevel,
+                                 RtpExtension::kAbsSendTime,
+                                 RtpExtension::kVideoRotation,
+                                 RtpExtension::kTransportSequenceNumber};
+const size_t kNumExtensions = 5;
+
+}  // namepsace
+
 // TODO(terelius): Place this definition with other parsing functions?
 MediaType GetRuntimeMediaType(rtclog::MediaType media_type) {
   switch (media_type) {
@@ -246,6 +265,14 @@ void VerifyRtcpEvent(const rtclog::Event& event,
   }
 }
 
+void VerifyPlayoutEvent(const rtclog::Event& event) {
+  ASSERT_TRUE(IsValidBasicEvent(event));
+  ASSERT_EQ(rtclog::Event::DEBUG_EVENT, event.type());
+  const rtclog::DebugEvent& debug_event = event.debug_event();
+  ASSERT_TRUE(debug_event.has_type());
+  EXPECT_EQ(rtclog::DebugEvent::AUDIO_PLAYOUT, debug_event.type());
+}
+
 void VerifyLogStartEvent(const rtclog::Event& event) {
   ASSERT_TRUE(IsValidBasicEvent(event));
   ASSERT_EQ(rtclog::Event::DEBUG_EVENT, event.type());
@@ -254,7 +281,71 @@ void VerifyLogStartEvent(const rtclog::Event& event) {
   EXPECT_EQ(rtclog::DebugEvent::LOG_START, debug_event.type());
 }
 
-void GenerateVideoReceiveConfig(VideoReceiveStream::Config* config) {
+/*
+ * Bit number i of extension_bitvector is set to indicate the
+ * presence of extension number i from kExtensionTypes / kExtensionNames.
+ * The least significant bit extension_bitvector has number 0.
+ */
+size_t GenerateRtpPacket(uint32_t extensions_bitvector,
+                         uint32_t csrcs_count,
+                         uint8_t* packet,
+                         size_t packet_size) {
+  CHECK_GE(packet_size, 16 + 4 * csrcs_count + 4 * kNumExtensions);
+  Clock* clock = Clock::GetRealTimeClock();
+
+  RTPSender rtp_sender(0,         // int32_t id
+                       false,     // bool audio
+                       clock,     // Clock* clock
+                       nullptr,   // Transport*
+                       nullptr,   // RtpAudioFeedback*
+                       nullptr,   // PacedSender*
+                       nullptr,   // PacketRouter*
+                       nullptr,   // SendTimeObserver*
+                       nullptr,   // BitrateStatisticsObserver*
+                       nullptr,   // FrameCountObserver*
+                       nullptr);  // SendSideDelayObserver*
+
+  std::vector<uint32_t> csrcs;
+  for (unsigned i = 0; i < csrcs_count; i++) {
+    csrcs.push_back(rand());
+  }
+  rtp_sender.SetCsrcs(csrcs);
+  rtp_sender.SetSSRC(rand());
+  rtp_sender.SetStartTimestamp(rand(), true);
+  rtp_sender.SetSequenceNumber(rand());
+
+  for (unsigned i = 0; i < kNumExtensions; i++) {
+    if (extensions_bitvector & (1u << i)) {
+      rtp_sender.RegisterRtpHeaderExtension(kExtensionTypes[i], i + 1);
+    }
+  }
+
+  int8_t payload_type = rand() % 128;
+  bool marker_bit = (rand() % 2 == 1);
+  uint32_t capture_timestamp = rand();
+  int64_t capture_time_ms = rand();
+  bool timestamp_provided = (rand() % 2 == 1);
+  bool inc_sequence_number = (rand() % 2 == 1);
+
+  size_t header_size = rtp_sender.BuildRTPheader(
+      packet, payload_type, marker_bit, capture_timestamp, capture_time_ms,
+      timestamp_provided, inc_sequence_number);
+
+  for (size_t i = header_size; i < packet_size; i++) {
+    packet[i] = rand();
+  }
+
+  return header_size;
+}
+
+void GenerateRtcpPacket(uint8_t* packet, size_t packet_size) {
+  for (size_t i = 0; i < packet_size; i++) {
+    packet[i] = rand();
+  }
+}
+
+void GenerateVideoReceiveConfig(uint32_t extensions_bitvector,
+                                VideoReceiveStream::Config* config) {
   // Create a map from a payload type to an encoder name.
   VideoReceiveStream::Decoder decoder;
   decoder.payload_type = rand();
@@ -266,24 +357,24 @@ void GenerateVideoReceiveConfig(VideoReceiveStream::Config* config) {
   // Add extensions and settings for RTCP.
   config->rtp.rtcp_mode = rand() % 2 ? newapi::kRtcpCompound
                                      : newapi::kRtcpReducedSize;
-  config->rtp.rtcp_xr.receiver_reference_time_report =
+  config->rtp.rtcp_xr.receiver_reference_time_report = (rand() % 2 == 1);
-      static_cast<bool>(rand() % 2);
+  config->rtp.remb = (rand() % 2 == 1);
-  config->rtp.remb = static_cast<bool>(rand() % 2);
   // Add a map from a payload type to a new ssrc and a new payload type for RTX.
   VideoReceiveStream::Config::Rtp::Rtx rtx_pair;
   rtx_pair.ssrc = rand();
   rtx_pair.payload_type = rand();
   config->rtp.rtx.insert(std::make_pair(rand(), rtx_pair));
-  // Add two random header extensions.
+  // Add header extensions.
-  const char* extension_name = rand() % 2 ? RtpExtension::kTOffset
+  for (unsigned i = 0; i < kNumExtensions; i++) {
-                                          : RtpExtension::kVideoRotation;
+    if (extensions_bitvector & (1u << i)) {
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+      config->rtp.extensions.push_back(
-  extension_name = rand() % 2 ? RtpExtension::kAudioLevel
+          RtpExtension(kExtensionNames[i], rand()));
-                              : RtpExtension::kAbsSendTime;
+    }
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
 }
 
-void GenerateVideoSendConfig(VideoSendStream::Config* config) {
+void GenerateVideoSendConfig(uint32_t extensions_bitvector,
+                             VideoSendStream::Config* config) {
   // Create a map from a payload type to an encoder name.
   config->encoder_settings.payload_type = rand();
   config->encoder_settings.payload_name = (rand() % 2 ? "VP8" : "H264");
@@ -294,21 +385,28 @@ void GenerateVideoSendConfig(VideoSendStream::Config* config) {
   config->rtp.rtx.payload_type = rand();
   // Add a CNAME.
   config->rtp.c_name = "some.user@some.host";
-  // Add two random header extensions.
+  // Add header extensions.
-  const char* extension_name = rand() % 2 ? RtpExtension::kTOffset
+  for (unsigned i = 0; i < kNumExtensions; i++) {
-                                          : RtpExtension::kVideoRotation;
+    if (extensions_bitvector & (1u << i)) {
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+      config->rtp.extensions.push_back(
-  extension_name = rand() % 2 ? RtpExtension::kAudioLevel
+          RtpExtension(kExtensionNames[i], rand()));
-                              : RtpExtension::kAbsSendTime;
+    }
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
 }
 
 // Test for the RtcEventLog class. Dumps some RTP packets to disk, then reads
 // them back to see if they match.
-void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
+void LogSessionAndReadBack(size_t rtp_count,
-  std::vector<std::vector<uint8_t>> rtp_packets;
+                           size_t rtcp_count,
-  std::vector<uint8_t> incoming_rtcp_packet;
+                           size_t debug_count,
-  std::vector<uint8_t> outgoing_rtcp_packet;
+                           uint32_t extensions_bitvector,
+                           uint32_t csrcs_count,
+                           unsigned random_seed) {
+  ASSERT_LE(rtcp_count, rtp_count);
+  ASSERT_LE(debug_count, rtp_count);
+  std::vector<rtc::Buffer> rtp_packets;
+  std::vector<rtc::Buffer> rtcp_packets;
+  std::vector<size_t> rtp_header_sizes;
 
   VideoReceiveStream::Config receiver_config(nullptr);
   VideoSendStream::Config sender_config(nullptr);
@@ -316,29 +414,23 @@ void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
   srand(random_seed);
 
   // Create rtp_count RTP packets containing random data.
-  const size_t rtp_header_size = 20;
   for (size_t i = 0; i < rtp_count; i++) {
-    size_t packet_size = 1000 + rand() % 30;
+    size_t packet_size = 1000 + rand() % 64;
-    rtp_packets.push_back(std::vector<uint8_t>());
+    rtp_packets.push_back(rtc::Buffer(packet_size));
-    rtp_packets[i].reserve(packet_size);
+    size_t header_size = GenerateRtpPacket(extensions_bitvector, csrcs_count,
-    for (size_t j = 0; j < packet_size; j++) {
+                                           rtp_packets[i].data(), packet_size);
-      rtp_packets[i].push_back(rand());
+    rtp_header_sizes.push_back(header_size);
-    }
   }
-  // Create two RTCP packets containing random data.
+  // Create rtcp_count RTCP packets containing random data.
-  size_t packet_size = 1000 + rand() % 30;
+  for (size_t i = 0; i < rtcp_count; i++) {
-  outgoing_rtcp_packet.reserve(packet_size);
+    size_t packet_size = 1000 + rand() % 64;
-  for (size_t j = 0; j < packet_size; j++) {
+    rtcp_packets.push_back(rtc::Buffer(packet_size));
-    outgoing_rtcp_packet.push_back(rand());
+    GenerateRtcpPacket(rtcp_packets[i].data(), packet_size);
-  }
-  packet_size = 1000 + rand() % 30;
-  incoming_rtcp_packet.reserve(packet_size);
-  for (size_t j = 0; j < packet_size; j++) {
-    incoming_rtcp_packet.push_back(rand());
   }
   // Create configurations for the video streams.
-  GenerateVideoReceiveConfig(&receiver_config);
+  GenerateVideoReceiveConfig(extensions_bitvector, &receiver_config);
-  GenerateVideoSendConfig(&sender_config);
+  GenerateVideoSendConfig(extensions_bitvector, &sender_config);
+  const int config_count = 2;
 
   // Find the name of the current test, in order to use it as a temporary
   // filename.
@@ -352,76 +444,102 @@ void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
     rtc::scoped_ptr<RtcEventLog> log_dumper(RtcEventLog::Create());
     log_dumper->LogVideoReceiveStreamConfig(receiver_config);
     log_dumper->LogVideoSendStreamConfig(sender_config);
-    size_t i = 0;
+    size_t rtcp_index = 1, debug_index = 1;
-    for (; i < rtp_count / 2; i++) {
+    for (size_t i = 1; i <= rtp_count; i++) {
       log_dumper->LogRtpHeader(
           (i % 2 == 0),  // Every second packet is incoming.
           (i % 3 == 0) ? MediaType::AUDIO : MediaType::VIDEO,
-          rtp_packets[i].data(), rtp_header_size, rtp_packets[i].size());
+          rtp_packets[i - 1].data(), rtp_packets[i - 1].size());
+      if (i * rtcp_count >= rtcp_index * rtp_count) {
+        log_dumper->LogRtcpPacket(
+            rtcp_index % 2 == 0,  // Every second packet is incoming
+            rtcp_index % 3 == 0 ? MediaType::AUDIO : MediaType::VIDEO,
+            rtcp_packets[rtcp_index - 1].data(),
+            rtcp_packets[rtcp_index - 1].size());
+        rtcp_index++;
+      }
+      if (i * debug_count >= debug_index * rtp_count) {
+        log_dumper->LogDebugEvent(RtcEventLog::DebugEvent::kAudioPlayout);
+        debug_index++;
+      }
+      if (i == rtp_count / 2) {
+        log_dumper->StartLogging(temp_filename, 10000000);
+      }
     }
-    log_dumper->LogRtcpPacket(false, MediaType::AUDIO,
-                              outgoing_rtcp_packet.data(),
-                              outgoing_rtcp_packet.size());
-    log_dumper->StartLogging(temp_filename, 10000000);
-    for (; i < rtp_count; i++) {
-      log_dumper->LogRtpHeader(
-          (i % 2 == 0),  // Every second packet is incoming,
-          (i % 3 == 0) ? MediaType::AUDIO : MediaType::VIDEO,
-          rtp_packets[i].data(), rtp_header_size, rtp_packets[i].size());
-    }
-    log_dumper->LogRtcpPacket(true, MediaType::VIDEO,
-                              incoming_rtcp_packet.data(),
-                              incoming_rtcp_packet.size());
   }
 
-  const int config_count = 2;
-  const int rtcp_count = 2;
-  const int debug_count = 1;  // Only LogStart event,
-  const int event_count = config_count + debug_count + rtcp_count + rtp_count;
-
   // Read the generated file from disk.
   rtclog::EventStream parsed_stream;
 
   ASSERT_TRUE(RtcEventLog::ParseRtcEventLog(temp_filename, &parsed_stream));
 
   // Verify the result.
+  const int event_count =
+      config_count + debug_count + rtcp_count + rtp_count + 1;
   EXPECT_EQ(event_count, parsed_stream.stream_size());
   VerifyReceiveStreamConfig(parsed_stream.stream(0), receiver_config);
   VerifySendStreamConfig(parsed_stream.stream(1), sender_config);
-  size_t i = 0;
+  size_t event_index = config_count, rtcp_index = 1, debug_index = 1;
-  for (; i < rtp_count / 2; i++) {
+  for (size_t i = 1; i <= rtp_count; i++) {
-    VerifyRtpEvent(parsed_stream.stream(config_count + i),
+    VerifyRtpEvent(parsed_stream.stream(event_index),
                    (i % 2 == 0),  // Every second packet is incoming.
                    (i % 3 == 0) ? MediaType::AUDIO : MediaType::VIDEO,
-                   rtp_packets[i].data(), rtp_header_size,
+                   rtp_packets[i - 1].data(), rtp_header_sizes[i - 1],
-                   rtp_packets[i].size());
+                   rtp_packets[i - 1].size());
+    event_index++;
+    if (i * rtcp_count >= rtcp_index * rtp_count) {
+      VerifyRtcpEvent(parsed_stream.stream(event_index),
+                      rtcp_index % 2 == 0,  // Every second packet is incoming.
+                      rtcp_index % 3 == 0 ? MediaType::AUDIO : MediaType::VIDEO,
+                      rtcp_packets[rtcp_index - 1].data(),
+                      rtcp_packets[rtcp_index - 1].size());
+      event_index++;
+      rtcp_index++;
+    }
+    if (i * debug_count >= debug_index * rtp_count) {
+      VerifyPlayoutEvent(parsed_stream.stream(event_index));
+      event_index++;
+      debug_index++;
+    }
+    if (i == rtp_count / 2) {
+      VerifyLogStartEvent(parsed_stream.stream(event_index));
+      event_index++;
+    }
   }
-  VerifyRtcpEvent(parsed_stream.stream(config_count + rtp_count / 2),
-                  false,  // Outgoing RTCP packet.
-                  MediaType::AUDIO, outgoing_rtcp_packet.data(),
-                  outgoing_rtcp_packet.size());
-
-  VerifyLogStartEvent(parsed_stream.stream(1 + config_count + rtp_count / 2));
-  for (; i < rtp_count; i++) {
-    VerifyRtpEvent(parsed_stream.stream(2 + config_count + i),
-                   (i % 2 == 0),  // Every second packet is incoming.
-                   (i % 3 == 0) ? MediaType::AUDIO : MediaType::VIDEO,
-                   rtp_packets[i].data(), rtp_header_size,
-                   rtp_packets[i].size());
-  }
-  VerifyRtcpEvent(parsed_stream.stream(2 + config_count + rtp_count),
-                  true,  // Incoming RTCP packet.
-                  MediaType::VIDEO, incoming_rtcp_packet.data(),
-                  incoming_rtcp_packet.size());
 
   // Clean up temporary file - can be pretty slow.
   remove(temp_filename.c_str());
 }
 
 TEST(RtcEventLogTest, LogSessionAndReadBack) {
-  LogSessionAndReadBack(5, 321);
+  // Log 5 RTP, 2 RTCP, and 0 playout events with no header extensions or CSRCS.
-  LogSessionAndReadBack(8, 3141592653u);
+  LogSessionAndReadBack(5, 2, 0, 0, 0, 321);
-  LogSessionAndReadBack(9, 2718281828u);
+
+  // Enable AbsSendTime and TransportSequenceNumbers
+  uint32_t extensions = 0;
+  for (uint32_t i = 0; i < kNumExtensions; i++) {
+    if (kExtensionTypes[i] == RTPExtensionType::kRtpExtensionAbsoluteSendTime ||
+        kExtensionTypes[i] ==
+            RTPExtensionType::kRtpExtensionTransportSequenceNumber) {
+      extensions |= 1u << i;
+    }
+  }
+  LogSessionAndReadBack(8, 2, 0, extensions, 0, 3141592653u);
+
+  extensions = (1u << kNumExtensions) - 1;  // Enable all header extensions
+  LogSessionAndReadBack(9, 2, 3, extensions, 2, 2718281828u);
+
+  // Try all combinations of header extensions and up to 2 CSRCS.
+  for (extensions = 0; extensions < (1u << kNumExtensions); extensions++) {
+    for (uint32_t csrcs_count = 0; csrcs_count < 3; csrcs_count++) {
+      LogSessionAndReadBack(5 + extensions,   // Number of RTP packets.
+                            2 + csrcs_count,  // Number of RTCP packets.
+                            3 + csrcs_count,  // Number of playout events
+                            extensions,       // Bit vector choosing extensions
+                            csrcs_count,      // Number of contributing sources
+                            rand());
+    }
+  }
 }
 
 }  // namespace webrtc