Changed LogRtpHeader to read the header length from the packet

webrtc/video/rtc_event_log_unittest.cc

Index: webrtc/video/rtc_event_log_unittest.cc
diff --git a/webrtc/video/rtc_event_log_unittest.cc b/webrtc/video/rtc_event_log_unittest.cc
index 0c18e750cc79cbd5938890211b8b64d43ee45135..036ac7c61354bcbed3977e93d048d936aa94e2e6 100644
--- a/webrtc/video/rtc_event_log_unittest.cc
+++ b/webrtc/video/rtc_event_log_unittest.cc
@@ -18,6 +18,7 @@
 #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"
@@ -246,6 +247,14 @@ void VerifyRtcpEvent(const rtclog::Event& event,
   }
 }
 
+void VerifyPlayoutEvent(const rtclog::Event& event) {
+  ASSERT_TRUE(IsValidBasicEvent(event));

[hlundin-webrtc, 2015/08/13 14:09:52]

If any of the ASSERT_*s trigger, only this subfunction will exit, and the rest
of the current test will be executed anyway. This may have some unforeseen
consequences. 

You might have already thought about this, but I wanted to point it out anyway.
See link below for discussions and ways to avoid it.
https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai...

[terelius, 2015/08/14 17:48:26]

Yes, it is intentional. Even if we fail to read one event in the event stream,
we might still be able to read the rest. This makes it easier to distinguish
whether it is a particular event type that we fail to parse, or whether it is
something that affects all events. (The downside is that we might flood the
terminal with error messages, so there is no obvious solution here.)

Thanks for the link though. I'll save it for future reference.

[hlundin-webrtc, 2015/08/17 13:47:07]

Acknowledged.

+  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 +263,80 @@ void VerifyLogStartEvent(const rtclog::Event& event) {
   EXPECT_EQ(rtclog::DebugEvent::LOG_START, debug_event.type());
 }
 
-void GenerateVideoReceiveConfig(VideoReceiveStream::Config* config) {
+/*
+ * LSB of extension_bitvector indicates presence of TransmissionTimeOffset,

[hlundin-webrtc, 2015/08/13 14:09:52]

Please, add constant bitmasks representing each bit:
static const uint32_t kTransmissionTimeOffsetBit = 1;
static const uint32_t kAudioLevelBit = 2;
... and so on.
Then use these when you construct your test cases.

[terelius, 2015/08/14 17:48:27]

I changed the approach to avoid repeating almost the same code for each of the 5
possible extensions. There is now an array of extension names and a
corresponding array of extension types. The bitmask for element i in these
vectors is 1<<i, but as it is only needed in very few places I have not added
names for each bitmask.

Do you still think that having named bitmasks would be useful?

+ * next higher bit indicates AudioLevel, then AbsoluteSendTime, then
+ * VideoRotation and finally TransportSequenceNumber.
+ */
+size_t GenerateRtpPacket(uint32_t extensions_bitvector,
+                         uint32_t csrcs_count,
+                         uint8_t* packet,
+                         size_t packet_size) {
+  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());
+
+  if (extensions_bitvector & 1)

[hlundin-webrtc, 2015/08/13 14:09:52]

Use the bitmasks here as well.

[terelius, 2015/08/14 17:48:26]

Rewritten.

+    rtp_sender.RegisterRtpHeaderExtension(
+        RTPExtensionType::kRtpExtensionTransmissionTimeOffset, 1);
+  if (extensions_bitvector & 2)
+    rtp_sender.RegisterRtpHeaderExtension(
+        RTPExtensionType::kRtpExtensionAudioLevel, 2);
+  if (extensions_bitvector & 4)
+    rtp_sender.RegisterRtpHeaderExtension(
+        RTPExtensionType::kRtpExtensionAbsoluteSendTime, 3);
+  if (extensions_bitvector & 8)
+    rtp_sender.RegisterRtpHeaderExtension(
+        RTPExtensionType::kRtpExtensionVideoRotation, 4);
+  if (extensions_bitvector & 16)
+    rtp_sender.RegisterRtpHeaderExtension(
+        RTPExtensionType::kRtpExtensionTransportSequenceNumber, 5);
+
+  int8_t payload_type = rand() % 128;
+  bool marker_bit = rand() & 0x01;
+  uint32_t capture_timestamp = rand();
+  int64_t capture_time_ms = rand();
+  bool timestamp_provided = rand() & 0x01;
+  bool inc_sequence_number = rand() & 0x01;
+
+  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();
@@ -274,16 +356,32 @@ void GenerateVideoReceiveConfig(VideoReceiveStream::Config* config) {
   rtx_pair.ssrc = rand();
   rtx_pair.payload_type = rand();
   config->rtp.rtx.insert(std::make_pair(rand(), rtx_pair));
-  // Add two random header extensions.
-  const char* extension_name = rand() % 2 ? RtpExtension::kTOffset
-                                          : RtpExtension::kVideoRotation;
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
-  extension_name = rand() % 2 ? RtpExtension::kAudioLevel
-                              : RtpExtension::kAbsSendTime;
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  // Add header extensions.
+  const char* extension_name;
+  if (extensions_bitvector & 1) {

[hlundin-webrtc, 2015/08/13 14:09:52]

... and here.

[terelius, 2015/08/14 17:48:26]

Rewritten.

+    extension_name = RtpExtension::kTOffset;

[hlundin-webrtc, 2015/08/13 14:09:52]

Can't you use RtpExtension::kTOffset directly in the call to RtpExtension? Then
you can delete extension_name, and save 25% of the lines here. Same thing
happens again later in the code.

[terelius, 2015/08/14 17:48:26]

I could, but that would make the lines too long. The automatic formatting would
then undo the 25% reduction of code we achieved by removing extension_name.

The new code is much shorter anyway.

+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 2) {
+    extension_name = RtpExtension::kAudioLevel;
+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 4) {
+    extension_name = RtpExtension::kAbsSendTime;
+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 8) {
+    extension_name = RtpExtension::kVideoRotation;
+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 16) {
+    extension_name = RtpExtension::kTransportSequenceNumber;  // TODO
+    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 +392,45 @@ 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.
-  const char* extension_name = rand() % 2 ? RtpExtension::kTOffset
-                                          : RtpExtension::kVideoRotation;
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
-  extension_name = rand() % 2 ? RtpExtension::kAudioLevel
-                              : RtpExtension::kAbsSendTime;
-  config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  // Add header extensions.
+  const char* extension_name;
+  if (extensions_bitvector & 1) {

[hlundin-webrtc, 2015/08/13 14:09:52]

... and bitmasks here, too.

[terelius, 2015/08/14 17:48:26]

Rewritten.

+    extension_name = RtpExtension::kTOffset;
+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 2) {
+    extension_name = RtpExtension::kAudioLevel;
+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 4) {
+    extension_name = RtpExtension::kAbsSendTime;
+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 8) {
+    extension_name = RtpExtension::kVideoRotation;
+    config->rtp.extensions.push_back(RtpExtension(extension_name, rand()));
+  }
+  if (extensions_bitvector & 16) {
+    extension_name = RtpExtension::kTransportSequenceNumber;
+    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) {
-  std::vector<std::vector<uint8_t>> rtp_packets;
-  std::vector<uint8_t> incoming_rtcp_packet;
-  std::vector<uint8_t> outgoing_rtcp_packet;
+void LogSessionAndReadBack(size_t rtp_count,
+                           size_t rtcp_count,
+                           size_t debug_count,
+                           uint32_t extensions_bitvector,
+                           uint32_t csrcs_count,
+                           unsigned random_seed) {
+  assert(rtcp_count <= rtp_count);

[hlundin-webrtc, 2015/08/13 14:09:52]

Use ASSERT_LE instead, to avoid crashing the entire test binary.

[terelius, 2015/08/14 17:48:26]

Done.

+  assert(debug_count <= rtp_count);
+  std::vector<uint8_t*> rtp_packets;
+  std::vector<size_t> rtp_packet_sizes;
+  std::vector<size_t> rtp_header_sizes;
+  std::vector<uint8_t*> rtcp_packets;
+  std::vector<size_t> rtcp_packet_sizes;
 
   VideoReceiveStream::Config receiver_config;
   VideoSendStream::Config sender_config;
@@ -316,29 +438,25 @@ 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;
-    rtp_packets.push_back(std::vector<uint8_t>());
-    rtp_packets[i].reserve(packet_size);
-    for (size_t j = 0; j < packet_size; j++) {
-      rtp_packets[i].push_back(rand());
-    }
-  }
-  // Create two RTCP packets containing random data.
-  size_t packet_size = 1000 + rand() % 30;
-  outgoing_rtcp_packet.reserve(packet_size);
-  for (size_t j = 0; j < packet_size; j++) {
-    outgoing_rtcp_packet.push_back(rand());
+    rtp_packet_sizes.push_back(packet_size);
+    rtp_packets.push_back(new uint8_t[packet_size]);
+    size_t header_size = GenerateRtpPacket(extensions_bitvector, csrcs_count,
+                                           rtp_packets[i], packet_size);
+    rtp_header_sizes.push_back(header_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 rtcp_count RTCP packets containing random data.
+  for (size_t i = 0; i < rtcp_count; i++) {
+    size_t packet_size = 1000 + rand() % 30;
+    rtcp_packet_sizes.push_back(packet_size);
+    rtcp_packets.push_back(new uint8_t[packet_size]);
+    GenerateRtcpPacket(rtcp_packets[i], packet_size);
   }
   // Create configurations for the video streams.
-  GenerateVideoReceiveConfig(&receiver_config);
-  GenerateVideoSendConfig(&sender_config);
+  GenerateVideoReceiveConfig(extensions_bitvector, &receiver_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 +470,95 @@ 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;
-    for (; i < rtp_count / 2; i++) {
+    size_t rtcp_index = 1, debug_index = 1;
+    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], rtp_packet_sizes[i - 1]);
+      if (i * rtcp_count >= rtcp_index * rtp_count) {
+        log_dumper->LogRtcpPacket(
+            rtcp_index % 2 == 0,  // Even packets incoming
+            rtcp_index % 3 == 0 ? MediaType::AUDIO : MediaType::VIDEO,
+            rtcp_packets[rtcp_index - 1], rtcp_packet_sizes[rtcp_index - 1]);
+        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;
-  for (; i < rtp_count / 2; i++) {
-    VerifyRtpEvent(parsed_stream.stream(config_count + i),
+  size_t event_index = config_count, rtcp_index = 1, debug_index = 1;
+  for (size_t i = 1; i <= rtp_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].size());
-  }
-  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());
+                   rtp_packets[i - 1], rtp_header_sizes[i - 1],
+                   rtp_packet_sizes[i - 1]);
+    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],
+                      rtcp_packet_sizes[rtcp_index - 1]);
+      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(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());
+
+  // Free memory
+  for (auto packet : rtp_packets) {
+    delete[] packet;
+  }
+  for (auto packet : rtcp_packets) {
+    delete[] packet;
+  }
 }
 
 TEST(RtcEventLogTest, LogSessionAndReadBack) {
-  LogSessionAndReadBack(5, 321);
-  LogSessionAndReadBack(8, 3141592653u);
-  LogSessionAndReadBack(9, 2718281828u);
+  LogSessionAndReadBack(5, 2, 0, 0, 0, 321);

[hlundin-webrtc, 2015/08/13 14:09:52]

Use the bitmask constants here too.

[terelius, 2015/08/14 17:48:26]

This is the one place where named bitmasks might be more convenient. Advice?

[hlundin-webrtc, 2015/08/17 13:47:07]

I suggest you introduce an array of bitmasks, the same size as the other const
arrays you already added at the top of the file. Then you can index that with
the same index as the other arrays, and use it both when constructing and
parsing the combined bit-patterns.

[terelius, 2015/08/18 08:20:51]

That would not simplify the code here. The issue is that I want to select two
specific extensions, for which I know the names but not the indices. Even if I
added an array of bitmasks, I would still have to iterate over the array to find
the correct mask.

+  LogSessionAndReadBack(8, 2, 0, 4 + 16, 0, 3141592653u);
+  LogSessionAndReadBack(9, 2, 3, 1 + 2 + 4 + 8 + 16, 2, 2718281828u);
+
+  for (unsigned extensions = 0; extensions < 32; extensions++) {

[hlundin-webrtc, 2015/08/13 14:09:52]

size_t, since extensions is used as input argument to
LogSessionAndReadBack(size_t ...)

[terelius, 2015/08/14 17:48:26]

Sorry, I don't understand. Can you elaborate?

[hlundin-webrtc, 2015/08/17 13:47:07]

I was nagging about you using unsigned as the type. Then you are passing it as
argument to LogSessionAndReadBack(..., uint32_t extensions_bitvector, ...).
Better to decide on using one type.

[terelius, 2015/08/18 08:20:51]

Oh, so you meant unsigned vs uint32_t. I got confused by the comment about
size_t, which isn't used at either place.

Changed to uint32_t.

+    for (unsigned csrcs_count = 0; csrcs_count < 3; csrcs_count++) {

[hlundin-webrtc, 2015/08/13 14:09:52]

size_t

[terelius, 2015/08/14 17:48:27]

I don't understand. Can you elaborate?

[hlundin-webrtc, 2015/08/17 13:47:07]

Same as above.

[terelius, 2015/08/18 08:20:51]

Same as above.

+      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