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..44c05d7e6093c7a7267b7074e3a07f2d8948c966 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"
@@ -254,6 +255,61 @@ void VerifyLogStartEvent(const rtclog::Event& event) {
   EXPECT_EQ(rtclog::DebugEvent::LOG_START, debug_event.type());
 }
 
+size_t GenerateRtpPacket(uint8_t* packet, size_t packet_size) {
+  Clock* clock = Clock::GetRealTimeClock();
+
+  RTPSender rtp_sender(rand(), // int32_t id

[hlundin-webrtc, 2015/08/04 09:43:50]

Use 0 as id instead. This test is not for testing the RTPSender.

[terelius, 2015/08/12 13:12:40]

Done.

+                       false, // bool audio
+                       clock, // Clock* clock
+                       nullptr,  // Transport*
+                       nullptr,  // RtpAudioFeedback*
+                       nullptr,  // PacedSender*
+                       nullptr,  // BitrateStatisticsObserver*
+                       nullptr,  // FrameCountObserver*
+                       nullptr);  // SendSideDelayObserver*
+
+  std::vector<uint32_t> csrcs;
+  csrcs.push_back(rand());
+  rtp_sender.SetCsrcs(csrcs);
+  rtp_sender.SetSSRC(rand());
+  rtp_sender.SetStartTimestamp(rand(), true);
+  rtp_sender.SetSequenceNumber(rand());
+
+  RTPExtensionType extension_types[] = {
+    RTPExtensionType::kRtpExtensionTransmissionTimeOffset,
+    RTPExtensionType::kRtpExtensionAudioLevel,
+    RTPExtensionType::kRtpExtensionAbsoluteSendTime,
+    RTPExtensionType::kRtpExtensionVideoRotation,
+    RTPExtensionType::kRtpExtensionTransportSequenceNumber,
+  };
+  for (int i=0; i<5; i++) {
+    if (rand()%2) {

[hlundin-webrtc, 2015/08/04 09:43:50]

I don't like this randomized test configuration. Construct the test such that
you can iterate through different configs, e.g., no extensions, one extension,
all extensions...

[terelius, 2015/08/12 13:12:40]

I've rewritten to the tests to allow manual control of which extensions are
used.

+     rtp_sender.RegisterRtpHeaderExtension(extension_types[i], i+2);
+   }
+  }
+
+  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 GenerateVideoReceiveConfig(VideoReceiveStream::Config* config) {
   // Create a map from a payload type to an encoder name.
   VideoReceiveStream::Decoder decoder;
@@ -306,7 +362,9 @@ void GenerateVideoSendConfig(VideoSendStream::Config* config) {
 // 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*> rtp_packets;
+  std::vector<size_t> rtp_packet_sizes;
+  std::vector<size_t> rtp_header_sizes;

[ivoc, 2015/08/06 12:29:44]

I don't see the advantage of this collection of datastructures over the vector
of vectors that used here previously. I would prefer the vector of vectors
because it eliminates any chance at memory leaks.

[terelius, 2015/08/12 13:12:40]

I agree that memory leaks is a concern, but all other code that manipulate
packets represent them as a uint8_t* with a size_t length. Thus, interfacing
with other code becomes easier this way, while most memory errors should be
caught by the asan/msan trybots if not by local testing.

It is certainly possible to use a vector, but I think it will lead to code like 
==================
vector<uint8_t> packet;
packet.assign(0, IP_SIZE);
size_t length = GenerateSomething(packet.data());
packet.resize(length);
DoSomething(packet.data(), packet.size());
==================
which I think is less clear than
==================
uint8_t* packet = new uint8_t[IP_SIZE];
size_t length = GenerateSomething(packet);
DoSomething(packet, length);
==================

[ivoc, 2015/08/14 11:21:53]

I think in your example the vector code could look like this:
====================
vector<uint8_t> packet(IP_SIZE); 
size_t length = GenerateSomething(packet.data());
DoSomething(packet.data(), length);
====================
Which is fairly similar to the non-vector case, with the difference that a
delete is no longer needed.
More specific to this function you could do something like this:
====================
vector<vector<uint8_t>> rtp_packets(rtp_count);
...
for (auto &p: rtp_packets) {
  size_t packet_size = 1000 + rand() % 30;
  p.resize(packet_size);
  ...
}
====================
Which can be compared to the following current code:
====================
std::vector<uint8_t*> rtp_packets;
std::vector<size_t> rtp_packet_sizes;
...
for (size_t i = 0; i < rtp_count; i++) {
  size_t packet_size = 1000 + rand() % 30;
  rtp_packet_sizes.push_back(packet_size);
  rtp_packets.push_back(new uint8_t[packet_size]);
  ...
}
...
for (size_t i = 0; i < rtp_count; i++) {
  delete[] rtp_packets[i];
}
====================
I honestly don't see any downsides of the vector approach, to me it looks
shorter, easier to read and safer.

[terelius, 2015/08/14 18:17:45]

Some functions like BuildRTPHeader writes some bytes and returns the number of
bytes written. If we are going to use the .size() to keep track of the packet
size, then we need to call resize twice. Once before building the header to
allocate enough space and once after to set the actual size.

Your first proposal which does not use resize at all, implicitly requires
storing the actual packet lengths in a second vector. I think this solution is a
bit odd since we would have a bunch of byte vectors each with it's own size()
function and then we would have a second vector for storing the actual size of
each packet. 

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

My random thoughts:

1. Consider using rtc::Buffer instead of std::vector.
2. You are caught between new and old paradigms. No solution is perfect.
3. If you are going for the raw pointers solution, use scoped_ptr to avoid
leaks.


Now, a concrete proposal:

Rewrite GeneateRtpPacket to 

size_t GenerateRtpPacket(rtc::Buffer* packet) {
  // Desired packet length is determined by packet->size();
  ...
  const size_t header_size = rtp_sender.BuildRTPheader(packet->data(),
                                                                             
...);
  ASSERT_LE(header_size, packet->size());
  // Randomize the payload as before.
  return header_size;
}

Use:
std::vector<rtc::Buffer> rtp_packets;
std::vector<size_t> rtp_header_sizes;

But skip:
std::vector<size_t> rtp_packet_sizes;

Use rtc::Buffer in the validation method(s) as well.

[kwiberg-webrtc, 2015/08/17 18:41:57]

+1

   std::vector<uint8_t> incoming_rtcp_packet;
   std::vector<uint8_t> outgoing_rtcp_packet;
 
@@ -316,14 +374,12 @@ 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());
-    }
+    rtp_packet_sizes.push_back(packet_size);
+    rtp_packets.push_back(new uint8_t[packet_size]);
+    size_t header_size = GenerateRtpPacket(rtp_packets[i], packet_size);
+    rtp_header_sizes.push_back(header_size);
   }
   // Create two RTCP packets containing random data.
   size_t packet_size = 1000 + rand() % 30;
@@ -357,7 +413,7 @@ void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
       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], rtp_packet_sizes[i]);
     }
     log_dumper->LogRtcpPacket(false, MediaType::AUDIO,
                               outgoing_rtcp_packet.data(),
@@ -367,7 +423,7 @@ void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
       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], rtp_packet_sizes[i]);
     }
     log_dumper->LogRtcpPacket(true, MediaType::VIDEO,
                               incoming_rtcp_packet.data(),
@@ -393,8 +449,7 @@ void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
     VerifyRtpEvent(parsed_stream.stream(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], rtp_header_sizes[i], rtp_packet_sizes[i]);
   }
   VerifyRtcpEvent(parsed_stream.stream(config_count + rtp_count / 2),
                   false,  // Outgoing RTCP packet.
@@ -406,8 +461,7 @@ void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
     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], rtp_header_sizes[i], rtp_packet_sizes[i]);
   }
   VerifyRtcpEvent(parsed_stream.stream(2 + config_count + rtp_count),
                   true,  // Incoming RTCP packet.
@@ -416,6 +470,11 @@ void LogSessionAndReadBack(size_t rtp_count, unsigned random_seed) {
 
   // Clean up temporary file - can be pretty slow.
   remove(temp_filename.c_str());
+
+  // Free memory
+  for (size_t i = 0; i < rtp_count; i++) {

[hlundin-webrtc, 2015/08/04 09:43:50]

for (auto packet : rtp_packets) {
  delete[] packet;
}

[terelius, 2015/08/12 13:12:40]

Done.

+    delete[] rtp_packets[i];
+  }
 }
 
 TEST(RtcEventLogTest, LogSessionAndReadBack) {