Update to the neteq_rtpplay utility to support RtcEventLog input files.

webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc

Index: webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc
diff --git a/webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc b/webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc
new file mode 100644
index 0000000000000000000000000000000000000000..13f88cd5000385644972ea2bdd01e7b05ff3c8dc
--- /dev/null
+++ b/webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc
@@ -0,0 +1,115 @@
+/*
+ *  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.
+ */
+
+#include "webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.h"
+
+#include <assert.h>
+#include <string.h>
+
+#include "webrtc/base/checks.h"
+#include "webrtc/modules/audio_coding/neteq/tools/packet.h"
+#include "webrtc/modules/rtp_rtcp/interface/rtp_header_parser.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
+#include "webrtc/video/rtc_event_log.pb.h"
+#endif
+
+namespace webrtc {
+namespace test {
+
+RtcEventLogSource* RtcEventLogSource::Create(const std::string& file_name) {
+  RtcEventLogSource* source = new RtcEventLogSource();
+  CHECK(source->OpenFile(file_name));
+  return source;
+}
+
+RtcEventLogSource::~RtcEventLogSource() {
+}
+
+bool RtcEventLogSource::RegisterRtpHeaderExtension(RTPExtensionType type,
+                                                   uint8_t id) {
+  assert(parser_.get());

[hlundin-webrtc, 2015/08/28 12:06:25]

CHECK instead. This is test/tools code; better to be aggressive about errors.

[ivoc, 2015/09/01 10:03:50]

Done.

+  return parser_->RegisterRtpHeaderExtension(type, id);
+}
+
+Packet* RtcEventLogSource::NextPacket() {
+  for (; rtp_packet_index_ < event_log_->stream_size(); rtp_packet_index_++) {
+    const rtclog::Event& event = event_log_->stream(rtp_packet_index_);
+    if (event.has_type() && event.type() == rtclog::Event::RTP_EVENT) {

[hlundin-webrtc, 2015/08/28 12:06:25]

This is a very long harangue of nested if statements. I'm thinking that it may
be better to do a helper function like this:

const rtclog::RtpPacket* GetRtpPacket(const rtclog::Event& event) {
  // All the ifs needed to return either a pointer to a valid rtclog::RtpPacket,
or nullptr.
  // Also, this can be implemented in a "break early on failure" manner.
  if (!event.has_type()) return nullptr;
  if (event.type() != rtclog::Event::RTP_EVENT) return nullptr;
  ...
}

Then you could just do 
const rtclog::RtpPacket* rtp_packet =
GetRtpPacket(event_log_->stream(rtp_packet_index_));
if (rtp_packet) {
  // Do the stuff...
}

WDYT?

[ivoc, 2015/09/01 10:03:50]

Great idea, I refactored the code like you suggested and it is much clearer.

+      if (event.has_timestamp_us() && event.has_rtp_packet()) {
+        const rtclog::RtpPacket& rtp_packet = event.rtp_packet();
+        if (rtp_packet.has_type() && rtp_packet.type() == rtclog::AUDIO &&
+            rtp_packet.has_incoming() && rtp_packet.incoming() == true &&

[hlundin-webrtc, 2015/08/28 12:06:25]

You can omit "== true".

[ivoc, 2015/09/01 10:03:50]

Done.

+            rtp_packet.has_packet_length() && rtp_packet.packet_length() > 0 &&
+            rtp_packet.has_header() && rtp_packet.header().size() > 0 &&
+            rtp_packet.packet_length() >= rtp_packet.header().size()) {
+          // Increase the index to avoid rechecking this event the next time
+          // the function is called.
+          rtp_packet_index_++;

[hlundin-webrtc, 2015/08/28 12:06:25]

The double increment (in the for statement and here) is awkward. Can you get
around it by using a while loop instead?

[ivoc, 2015/09/01 10:03:50]

Done.

+          uint8_t* packet_data = new uint8_t[rtp_packet.header().size()];
+          memcpy(packet_data, rtp_packet.header().data(),

[minyue-webrtc, 2015/08/28 16:54:17]

Do you need payload data for packet_data, and is rtp_packet.header().data() only
a header?

[ivoc, 2015/09/01 10:03:50]

Right now the rtp packet message in the protobuf only has a field for the
header, there is no place intended for payloads or full packages. So
rtp_packet.header().data() should indeed only contain the header. I renamed the
variable to make this more clear.

[minyue-webrtc, 2015/09/01 11:48:35]

Yes, that makes it clearer.

Now I see why I had a doubt here. I thought that the Packet must be a full
packet and payload should be faked here (maybe advisable, but no action needed
in this CL).

+                 rtp_packet.header().size());
+          rtc::scoped_ptr<Packet> packet(

[minyue-webrtc, 2015/08/28 14:50:21]

what is the life time of packet?

I'd like the method be 
void RtcEventLogSource::NextPacket(Packet* packet) and let the caller hold the
|packet|, since it is not obvious that the caller has to release the memory.

[ivoc, 2015/09/01 10:03:50]

I agree that this is not super obvious, but the reason I made it like this is
because this method is on the PacketSource interface (which this class and a few
others implements). If I want to change it, I should change it everywhere. Do
you think I should?

[minyue-webrtc, 2015/09/01 11:48:35]

No bother. maybe no need of scoped_ptr around packet

+              new Packet(packet_data, rtp_packet.header().size(),
+                         rtp_packet.packet_length(),
+                         event.timestamp_us() / 1000, *parser_.get()));
+          if (!packet->valid_header()) {

[hlundin-webrtc, 2015/08/28 12:06:25]

Are we expecting the logger to write invalid headers? Does it verify them before
writing, or is it just dumping them verbatim?

If we are not expecting invalid headers, simply CHECK(packet->valid_header()).
Otherwise, remove the assert, and print a warning.

[ivoc, 2015/09/01 10:03:50]

The header is currently not checked while writing, they are just dumped exactly
as they come in. I made your suggested changes.

+            assert(false);
+            return nullptr;
+          }
+          if (filter_.test(packet->header().payloadType) ||
+              (use_ssrc_filter_ && packet->header().ssrc != ssrc_)) {
+            // This payload type should be filtered out. Continue to the next

[hlundin-webrtc, 2015/08/28 12:06:25]

s/payload type/packet/

[ivoc, 2015/09/01 10:03:50]

I replaced this, but I also restructured this code a bit, so I made some more
changes.

+            // packet.
+            continue;
+          }
+          return packet.release();
+        }
+      }
+    }
+  }
+  return nullptr;
+}
+
+int RtcEventLogSource::NextAudioOutputEventMs() {
+  for (; audio_output_index_ < event_log_->stream_size();
+       audio_output_index_++) {
+    const rtclog::Event& event = event_log_->stream(audio_output_index_);
+    if (event.has_type() && event.type() == rtclog::Event::DEBUG_EVENT) {

[hlundin-webrtc, 2015/08/28 12:06:25]

Same comment as above about a helper function.

[ivoc, 2015/09/01 10:03:50]

I restructured in a similar way.

+      if (event.has_timestamp_us() && event.has_debug_event()) {
+        const rtclog::DebugEvent& debug_event = event.debug_event();
+        if (debug_event.has_type() &&
+            debug_event.type() == rtclog::DebugEvent::AUDIO_PLAYOUT) {
+          // Increase the index to avoid rechecking this event the next time
+          // the function is called.
+          audio_output_index_++;

[hlundin-webrtc, 2015/08/28 12:06:25]

Same awkwardness here.

[ivoc, 2015/09/01 10:03:50]

Same fix applied.

+          return event.timestamp_us() / 1000;

[hlundin-webrtc, 2015/08/28 12:06:25]

What is the type of timestamp_us? If it is not int, it would be better to return
the original type to avoid implicit casts.

[ivoc, 2015/09/01 10:03:50]

The value is int64_t, so I changed the return type as well as the variables
holding timestamps in neteq_rtpplay.cc. 

+        }
+      }
+    }
+  }
+  return -1;
+}
+
+RtcEventLogSource::RtcEventLogSource()
+    : PacketSource(), parser_(RtpHeaderParser::Create()) {
+}
+
+bool RtcEventLogSource::OpenFile(const std::string& file_name) {
+  event_log_.reset(new rtclog::EventStream());
+  return RtcEventLog::ParseRtcEventLog(file_name, event_log_.get());
+}
+
+}  // namespace test
+}  // namespace webrtc