New rtc dump analyzing tool in Python

tools/py_event_log_analyzer/pb_parse.py

+#  Copyright (c) 2016 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.
+
+"""Parses protobuf RTC dumps.
+"""
+
+from __future__ import division
+
+import os
+import struct
+import sys
+import enum
+
+
+# makes sure to import correct protobuf
+webrtc_root = "../../"
+sys.path.append(os.path.join(webrtc_root, "out/Debug/pyproto/webrtc/call/"))
+sys.path.append(os.path.join(webrtc_root, "out/Release/pyproto/webrtc/call/"))
+sys.path.append(os.path.join(webrtc_root, "third_party/protobuf/python/"))

[kwiberg-webrtc, 2016/05/24 12:09:51]

sys.path += [os.path.join(webrtc_root, path) for path in
             ["out/Debug/pyproto/webrtc/call/",
              "out/Release/pyproto/webrtc/call/",
              "third_party/protobuf/python/"]]

?

[aleloi, 2016/05/24 15:54:41]

The point is to make it import the correct generated protobuf file and library.
When WebRTC is compiled, rtc_event_log_pb2 is placed in out/???/pyproto/... The
protobuf library lies in third_party/protobuf.

Instead of asking the user to set PYTHONPATH so it contains these things, I
edited it before including the library. There is more about this in
https://codereview.webrtc.org/1999113002/#msg6

This is not a good solution, but I couldn't find any other.

[kwiberg-webrtc, 2016/05/25 12:42:41]

Sorry, I didn't mean to ask why you did this; I just tried to suggest a less
repetitive way of saying the same thing (and incidentally failed to remove the
then-useless local webrtc_root).

[aleloi2, 2016/05/30 14:57:55]

Thank you! This is better. Ignore the latest comment then; I misunderstood.

+import rtc_event_log_pb2 as rtc_pb
+
+
+DIR = enum.Enum("Direction", "incoming outgoing")

[ivoc, 2016/05/24 11:50:18]

I would prefer a less fancy solution that does not add a dependency to the code.

[kwiberg-webrtc, 2016/05/24 12:09:51]

Why is this in all caps?

[aleloi, 2016/05/24 15:54:40]

Got convinced by Ivo and removed. Directions are not used at all, so I removed
it.

+
+
+class DataPoint(object):
+  """Simple container class for RTP events.
+  """
+
+  def __init__(self, rtp_header_str, packet_size,
+               arrival_timestamp_us, direction):
+    """Builds a data point by parsing an RTP header.
+
+    As defined in RFC 3550 section 5.1.
+    """
+    self.size = packet_size
+    self.arrival_timestamp_ms = arrival_timestamp_us / 1000
+    unpacked = struct.unpack_from("!HHII", rtp_header_str, 0)
+    first2header_bytes, self.seq_no, self.timestamp, self.ssrc = unpacked

[ivoc, 2016/05/24 11:50:18]

The temporary 'unpacked' variable can be removed here. Please rename seq_no to
sequence_number.

[kwiberg-webrtc, 2016/05/24 12:09:51]

I find it's good for readability to surround tuples by parentheses.

[aleloi, 2016/05/24 15:54:40]

Acknowledged.

+
+    def bit_unpack(bit_offsets, number):
+      """unpacks a big-endian number into blocks of bits."""

[kwiberg-webrtc, 2016/05/24 12:09:51]

This doesn't sound right. Python integers are always host-endian;
struct.unpack_from has done endian conversion for you.

[aleloi, 2016/05/24 15:54:40]

Yes, thank you. Anyway, I removed it completely by your and Ivo's suggestions.

+      res = []
+      for offset in reversed(bit_offsets):
+        mask = ((1<<offset) - 1)
+        res.append(number & mask)
+        number >>= offset
+      return reversed(res)
+
+    # fields V, P, X, CC, and M of the header are ignored
+    _, _, _, _, _, self.pt = bit_unpack([2, 1, 1, 4, 1, 7], first2header_bytes)

[ivoc, 2016/05/24 11:50:18]

I think something like this would be simpler (I didn't check it, but I think it
should work): self.pt = int(bin(first2header_bytes)[-7:], 2)
Also, rename pt to payload_type.

[kwiberg-webrtc, 2016/05/24 12:09:51]

It's probably much easier to just shift and mask manually, especially since you
only need one of the bit fields:

  self.pt = first2header_bytes & 0x7f

(Note: I'm not entirely sure that I got that right. Double check!)

The ctypes module can do bit fields as well as endianness conversion and naming
the fields, if you want to make the parsing fancier in the future:
https://docs.python.org/3/library/ctypes.html#structure-union-alignment-and-b...,
https://docs.python.org/3/library/ctypes.html#bit-fields-in-structures-and-un...

[aleloi, 2016/05/24 15:54:40]

Thank you! Changed to "& 0b01111111". I think it's a little more readable. Your
hex arithmetic was correct.

+    self.direction = direction

[ivoc, 2016/05/24 11:50:18]

Move to line 42 please.

[aleloi, 2016/05/24 15:54:40]

completely removed because not used.

+
+
+def parse_protobuf(file_path):
+  """Parses rtc event log from protobuf file.
+
+  Args:
+       file_path: path to protobuf file of RTC event stream
+
+  Returns:
+    all RtpPacket events from the event stream as a list of DataPoint:s

[ivoc, 2016/05/24 11:50:18]

Remove the : please.

[aleloi, 2016/05/24 15:54:40]

Done.

+  """
+  event_stream = rtc_pb.EventStream()
+  with open(file_path, "rb") as f:
+    event_stream.ParseFromString(f.read())
+  result = []
+
+  for event in event_stream.stream:
+    if event.HasField("rtp_packet"):
+      rtp_packet = event.rtp_packet
+      header = rtp_packet.header
+      direction = DIR.incoming if rtp_packet.incoming else DIR.outgoing
+      result.append(DataPoint(header, rtp_packet.packet_length,
+                              event.timestamp_us,
+                              direction))

[kwiberg-webrtc, 2016/05/24 12:09:51]

Consider using a comprehension:

  return [DataPoint(event.rtp_packet.header,
                    event.rtp_packet.packet_length,
                    event.timestamp_us,
                    DIR.incoming if event.rtp_packet.incoming else DIR.outgoing)
          for event in event_stream.stream
          if event.HasField("rtp_packet")]

I'm assuming I don't have to preach the virtues of functional programming to
you. :-)

[aleloi, 2016/05/24 15:54:40]

Thank you, that's nicer!

+
+  return result