New parser for event log. Manually parse the outermost EventStream

webrtc/call/rtc_event_log_parser.h

+/*
+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.

[ivoc, 2016/04/01 08:27:59]

Should be 2016 I guess?

[terelius, 2016/04/19 17:01:45]

I created the file in 2015 when I started on the previous version of the parser.
So it depends whether we consider this a new implementation, or a refinement of
the old one. :)

[ivoc, 2016/04/25 08:08:12]

Ok, I'll leave it up to you :)

+ *
+ *  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.
+ */
+#ifndef WEBRTC_CALL_RTC_EVENT_LOG_PARSER_H_
+#define WEBRTC_CALL_RTC_EVENT_LOG_PARSER_H_
+
+#include <string>
+#include <vector>
+
+#include "webrtc/call/rtc_event_log.h"
+#include "webrtc/video_receive_stream.h"
+#include "webrtc/video_send_stream.h"
+
+// Files generated at build-time by the protobuf compiler.
+#ifdef WEBRTC_ANDROID_PLATFORM_BUILD
+#include "external/webrtc/webrtc/call/rtc_event_log.pb.h"
+#else
+#include "webrtc/call/rtc_event_log.pb.h"
+#endif
+
+namespace webrtc {
+
+enum class MediaType;
+
+class ParsedRtcEventLog {
+  friend class RtcEventLogTestHelper;
+
+ public:
+  enum EventType {

[ivoc, 2016/04/01 08:27:59]

Would it be possible to reuse the enum from the generated protobuf code? maybe
with a typedef? It would be nice if it is updated automatically when new event
types are added.

[terelius, 2016/04/19 17:01:45]

It is non-trivial. The enum is actually named Event_EventType and typedef'ed to
Event::EventType. This is fine, but the second part of the problem is that the
literal names in Event_EventType are Event_EventType_UNKNOWN_EVENT and so on.
The Event class in turn declare static constants such as
static const EventType UNKNOWN_EVENT = Event_EventType_UNKNOWN_EVENT;
So regardless of whether we do typedef Event::EventType EventType or typedef
Event_EventType EventType we won't get nice names for the enum literals.

[ivoc, 2016/04/25 08:08:12]

Acknowledged.

+    UNKNOWN_EVENT = 0,
+    LOG_START = 1,
+    LOG_END = 2,
+    RTP_EVENT = 3,
+    RTCP_EVENT = 4,
+    AUDIO_PLAYOUT_EVENT = 5,
+    BWE_PACKET_LOSS_EVENT = 6,
+    BWE_PACKET_DELAY_EVENT = 7,
+    VIDEO_RECEIVER_CONFIG_EVENT = 8,
+    VIDEO_SENDER_CONFIG_EVENT = 9,
+    AUDIO_RECEIVER_CONFIG_EVENT = 10,
+    AUDIO_SENDER_CONFIG_EVENT = 11
+  };
+
+  // Reads an RtcEventLog file and returns true if parsing was successful.
+  bool ParseFile(const std::string& file_name);

[ivoc, 2016/04/01 08:27:59]

I guess it doesn't make a big difference either way (and I think this way is
fine), but this could also be done in the constructor.

[terelius, 2016/04/19 17:01:45]

Yeah, I though about that. I don't have any strong opinion either, but one could
conceivably have a program that repeatedly asks the user for a file and analyze
it. If parsing is done in the constructor, then one would have to construct new
objects on the heap instead of just parsing the next file. On the other hand,
passing the file name into the constructor would save one line of code.

[ivoc, 2016/04/25 08:08:12]

Ok, sounds reasonable.

+
+  // Returns the number of events in an EventStream.
+  size_t GetNumberOfEvents() const;
+
+  // Reads the arrival timestamp (in microseconds) from a rtclog::Event.
+  int64_t GetTimestamp(size_t i) const;

[ivoc, 2016/04/01 08:27:59]

I would prefer a more descriptive name instead of 'i', maybe something like
event_index (same for functions below)?

[terelius, 2016/04/19 17:01:45]

Done.

+
+  // Reads the event type of the rtclog::Event at index i.
+  EventType GetEventType(size_t i) const;
+
+  // Reads the header, direction, media type, header length and packet length
+  // from the RTP event at index i. The values are stored in the corresponding
+  // output parameters. If some value is irrelevant, then that output parameter

[ivoc, 2016/04/01 08:27:59]

I think this could be rephrased a bit more clearly, maybe something like "If an
output parameter is not needed, it can be set to nullptr.". Same applies to
comment below.

[terelius, 2016/04/19 17:01:45]

Done.

+  // can be set to NULL.
+  // NB: The header must have space for at least IP_PACKET_SIZE bytes.
+  void GetRtpHeader(size_t i,
+                    PacketDirection* incoming,
+                    MediaType* media_type,
+                    uint8_t* header,
+                    size_t* header_length,
+                    size_t* total_length) const;
+
+  // Reads packet, direction, media type and packet length from the RTCP event
+  // at index i. The values are stored in the corresponding output parameters.
+  // If some value is irrelevant, then that output parameter can be set to NULL.
+  // NB: The packet must have space for at least IP_PACKET_SIZE bytes.
+  void GetRtcpPacket(size_t i,
+                     PacketDirection* incoming,
+                     MediaType* media_type,
+                     uint8_t* packet,
+                     size_t* length) const;
+
+  // Reads a config event to a (non-NULL) VideoReceiveStream::Config struct.
+  // Only the fields that are stored in the protobuf will be written.
+  void GetVideoReceiveConfig(size_t i,
+                             VideoReceiveStream::Config* config) const;
+
+  // Reads a config event to a (non-NULL) VideoSendStream::Config struct.
+  // Only the fields that are stored in the protobuf will be written.
+  void GetVideoSendConfig(size_t i, VideoSendStream::Config* config) const;
+
+  // Reads the SSRC from the audio playout event at index i. The SSRC is stored
+  // in the output parameter ssrc. To only check that the event is well formed,
+  // the output parameter can be set to NULL.
+  void GetAudioPlayout(size_t i, uint32_t* ssrc) const;
+
+  // Reads packet, direction, media type and packet length from the RTCP event
+  // at index i. The values are stored in the corresponding output parameters.
+  // If some value is irrelevant, then that output parameter can be set to NULL.
+  // NB: The packet must have space for at least IP_PACKET_SIZE bytes.
+  void GetBwePacketLossEvent(size_t i,
+                             int32_t* bitrate,
+                             uint8_t* fraction_loss,
+                             int32_t* total_packets) const;
+
+ private:
+  std::vector<rtclog::Event> stream_;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_CALL_RTC_EVENT_LOG_PARSER_H_