Changed LogRtpHeader to read the header length from the packet

webrtc/video/rtc_event_log_unittest.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifdef ENABLE_RTC_EVENT_LOG
 
 #include <stdio.h>
 #include <string>
 #include <vector>
 
 #include "testing/gtest/include/gtest/gtest.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"
 #include "webrtc/test/testsupport/gtest_disable.h"
 #include "webrtc/video/rtc_event_log.h"
 
 // Files generated at build-time by the protobuf compiler.
 #ifdef WEBRTC_ANDROID_PLATFORM_BUILD
 #include "external/webrtc/webrtc/video/rtc_event_log.pb.h"
 #else
@@ skipping 216 matching lines @@
 }
 
 void VerifyLogStartEvent(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::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;
   decoder.payload_type = rand();
   decoder.payload_name = (rand() % 2 ? "VP8" : "H264");
   config->decoders.push_back(decoder);
   // Add SSRCs for the stream.
   config->rtp.remote_ssrc = rand();
   config->rtp.local_ssrc = rand();
   // Add extensions and settings for RTCP.
@@ skipping 32 matching lines @@
                                           : 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()));
 }
 
 // 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;
 
   VideoReceiveStream::Config receiver_config;
   VideoSendStream::Config sender_config;
 
   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;
   outgoing_rtcp_packet.reserve(packet_size);
   for (size_t j = 0; j < packet_size; j++) {
     outgoing_rtcp_packet.push_back(rand());
   }
   packet_size = 1000 + rand() % 30;
   incoming_rtcp_packet.reserve(packet_size);
   for (size_t j = 0; j < packet_size; j++) {
@@ skipping 13 matching lines @@
   // to disk.
   {
     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++) {
       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(),
                               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());
+          rtp_packets[i], rtp_packet_sizes[i]);
     }
     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.
   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),
                    (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.
                   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], rtp_header_sizes[i], rtp_packet_sizes[i]);
   }
   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 (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) {
   LogSessionAndReadBack(5, 321);
   LogSessionAndReadBack(8, 3141592653u);
   LogSessionAndReadBack(9, 2718281828u);
 }
 
 }  // namespace webrtc
 
 #endif  // ENABLE_RTC_EVENT_LOG