New rtc dump analyzing tool in Python

tools/py_event_log_analyzer/rtp_analyzer.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.
+
+"""Displays statistics and plots graphs from RTC protobuf dump."""
+
+from __future__ import division
+from __future__ import print_function
+
+import sys
+import builtins
+import matplotlib.pyplot as plt
+import misc
+import numpy
+import pb_parse
+
+
+class RTPStatistics(object):
+  """Acts as namespace for RTP statistics.

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

Modules and packages are Python's closest analogs to C++'s namespaces. This is a
class, with member variables, __init__, and all that; calling it a namespace
seems confusing.

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

Changed. Still not really satisfied with the comment.

+  """
+
+  BANDWIDTH_SMOOTHING_WINDOW_SIZE = 10
+
+  def __init__(self, data_points):
+    """Initializes data_points and does computations.

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

I suggest merging this with the comment below, so: Initializes data_points and
calculates statistics for ...

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

This sounds misleading. __init__ doesn't do anything to data_points, does it?

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

Hopefully a little better now.

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

Done.

+
+    Calculates statistics for number of packages and size of packages

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

package -> packet

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

Done.

+    by SSRC.
+
+    Args:
+        data_points: list of pb_parse.DataPoint:s on which statistics are

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

Remove the : in DataPoint:s please.

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

Done.

+            calculated.
+
+    """
+
+    # currently does nothing, because parse_protobuf() only returns RTP packages

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

Please start comment with a capital and end with a .

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

Removed completely.

+    no_rtcp_packages = [x for x in data_points if not 72 <= x.pt <= 76]

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

package -> packet, aren't these just the RTP packets? If so, rename to
rtp_packets. It also seems redundant to me to split into RTP/RTCP packets here
when they are already kind of separately stored in the event log. 

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

"packages" -> "packets" (twice)

Also, I don't understand this variable name. "no_xxx" sounds like it ought to be
a bool. Should it be non_rtcp_packets? num_rtcp_packets? Something else?

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

removed, because RTCP were filtered in protobuf already.

+    num_rtcp = len(data_points) - len(no_rtcp_packages)
+    if num_rtcp > 0:
+      print("Removing {} RTCP packets".format(num_rtcp))
+    else:
+      print("No RTCP packets present")
+    data_points = no_rtcp_packages

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

Please don't reuse variable names lightly. It makes the code harder to read.

+
+    self.data_points = data_points
+    self.ssrc_frequencies = misc.percent_table([x.ssrc for x in
+                                                self.data_points])
+    self.ssrc_size_table = misc.ssrc_size_table(self.data_points)
+    self.bandwidth_kbps = None
+    self.smooth_bw_kbps = None
+
+  def print_ssrc_info(self, ssrc_id, ssrc):
+    """Prints packet and size statistics for given SSRC.

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

Explain the other argument.

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

Done.

+
+    Raises:
+         Exception: when different payload types are present in data
+           for same SSRC
+    """
+    filtered_ssrc = [x for x in self.data_points if x.ssrc == ssrc]
+    payloads = misc.percent_table([x.pt for x in filtered_ssrc])
+    sizes = misc.percent_table([x.size for x in filtered_ssrc])
+
+    if len(payloads) == 1:
+      payload_info = "payload type {}".format(*list(payloads))
+    else:
+      raise Exception(
+          "This tool cannot yet handle changes in codec sample rate")
+    print("{} 0X{:X} {}, {:.2f}% packets, {:.2f}% data".format(
+        ssrc_id, ssrc, payload_info, self.ssrc_frequencies[ssrc]*100,
+        self.ssrc_size_table[ssrc]*100))
+    print("  package sizes:")
+    size_hists = misc.hists(sizes, 5)
+    print("\n".join([
+        "  {} - {}: {:.2f}%".format(size_interval[0], size_interval[1],
+                                    size_hists[size_interval]*100)
+        for size_interval in sorted(size_hists)
+    ]))
+
+  def choose_ssrc(self):
+    """Queries user for SSRC."""
+    ssrc_frequencies_lst = list(enumerate(self.ssrc_frequencies))
+
+    assert self.ssrc_frequencies

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

This should be at the top of the function.

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

Was not really needed, because constructor initializes ssrc_frequencies

+    if len(self.ssrc_frequencies) == 1:
+      chosen_ssrc = self.ssrc_frequencies[0][-1]
+      self.print_ssrc_info("", chosen_ssrc)
+      return chosen_ssrc
+
+    for i, ssrc in enumerate(self.ssrc_frequencies):
+      self.print_ssrc_info(i, ssrc)
+    chosen_index = None
+    while chosen_index is None:
+      chosen_index = int(builtins.input("choose one> "))
+      if 0 <= chosen_index < len(ssrc_frequencies_lst):
+        chosen_ssrc = ssrc_frequencies_lst[chosen_index][-1]
+      else:
+        print("Invalid index!")
+        chosen_index = None
+    return chosen_ssrc

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

Hmm. Wouldn't it be simpler to do something like

  while True:
      ....
      if ...:
          return ssrc_frequencies_lst[chosen_index][-1]
      else:
          ...

That lets you eliminate one local variable and reduce the scope of another.

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

Done.

+
+  def filter_ssrc(self, chosen_ssrc):
+    """Filters and wraps data points.
+
+    Removes data points with `ssrc != chosen_ssrc`. Unwraps sequence
+    numbers and time stamps for the chosen selection.
+    """
+    self.data_points = [x for x in self.data_points if x.ssrc ==
+                        chosen_ssrc]
+    data_points_seq_no_unwrap = misc.unwrap([x.seq_no for x in
+                                             self.data_points],
+                                            2**16-1)  # 65535

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

This comment is probably not that useful. You've picked a self-documenting
format for the number in the code.

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

Done.

+    for i, seq_no_unwrap_value in enumerate(data_points_seq_no_unwrap):
+      self.data_points[i].seq_no = seq_no_unwrap_value
+
+    data_points_time_stamp_unwrap = enumerate(
+        misc.unwrap([x.timestamp for x in self.data_points],
+                    2**32-1))  # 4294967295

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

Remove this comment too.

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

Done.

+    for i, timestamp_unwrap_value in data_points_time_stamp_unwrap:
+      self.data_points[i].timestamp = timestamp_unwrap_value

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

You've placed enumerate outside the loop expression in this loop; this is
inconsistent with the previous loop.

Relatedly, consider writing these loops like this instead:

  for (data_point, timestamp) in zip(self.data_points,
                                     unwrapped_timestamps):
      data_point.timestamp = unwrapped_timestamp

Indexes are very expressive, so by avoiding them you reduce the number of
possible bugs your code can have.

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

Done.

+
+  def print_seq_no_statistics(self):

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

rename to print_sequence_number_statistics

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

Done.

+    sortseq_no = sorted(x.seq_no for x in self.data_points)
+    print("Missing sequence numbers: {} out of {}".format(
+        sortseq_no[-1] - sortseq_no[0] + 1 - len(set(sortseq_no)),
+        len(set(sortseq_no))

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

You can get the min and max elements without sorting the sequence numbers.

Also, you convert to a set twice.

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

Done.

+    ))
+    print("Duplicated packets: {}".format(sortseq_no.count(0)))

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

How does this work? Doesn't this just count the number of packets with sequence
number 0? (And in a very inefficient way, too, considering sortseq_no is
sorted.)

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

Yes, that was wrong. Fixed now!

+    print("Reordered packets: {}".format(
+        misc.count_reordered([x.seq_no for x in  self.data_points])))
+
+  def print_frequency_duration_statistics(self):
+    """Estimates frequency and prints related statistics.
+
+    Guesses the most probable frequency by looking at changes in
+    timestamps (RFC 3550 section 5.1), calculates clock drifts and
+    sending time of packets. Updates `self.data_points` with changes
+    in delay and send time.
+
+    """
+    delta_timestamp = (self.data_points[-1].timestamp -
+                       self.data_points[0].timestamp)
+    delta_arr_timestamp = float((self.data_points[-1].arrival_timestamp_ms -
+                                 self.data_points[0].arrival_timestamp_ms))
+    fs_est = delta_timestamp / delta_arr_timestamp
+
+    fs_vec = [8, 16, 32, 48, 90]  # TODO(aleloi) 90 is a hack for video
+    fs = None
+    for f in fs_vec:
+      if abs((fs_est - f)/float(f)) < 0.05:

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

Why not just use the closest one to the estimated frequency?

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

To notify the user that something is odd when the estimated frequency is far
off. Added this notification.

+        fs = f
+
+    print("Estimated frequency: {}".format(fs_est))
+    print("Guessed frequency: {}".format(fs))
+
+    for f in self.data_points:
+      f.real_send_time_ms = (f.timestamp -
+                             self.data_points[0].timestamp) / fs
+      f.delay = f.arrival_timestamp_ms - f.real_send_time_ms
+
+    min_delay = min(f.delay for f in self.data_points)
+
+    for f in self.data_points:
+      f.absdelay = f.delay - min_delay
+
+    stream_duration_sender = self.data_points[-1].real_send_time_ms / 1000
+    print("Stream duration at sender: {:.1f} seconds".format(
+        stream_duration_sender
+    ))
+
+    stream_duration_receiver = (self.data_points[-1].arrival_timestamp_ms -

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

Packet reordering could make this incorrect, max/min would be more accurate.

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

Done.

+                                self.data_points[0].arrival_timestamp_ms) / 1000
+    print("Stream duration at receiver: {:.1f} seconds".format(
+        stream_duration_receiver
+    ))
+
+    print("Clock drift: {:.2f}%".format(
+        100* (stream_duration_receiver / stream_duration_sender - 1)
+    ))
+
+    print("Send average bitrate: {:.2f} kbps".format(
+        sum(x.size for x
+            in self.data_points) * 8 / stream_duration_sender / 1000))
+
+    print("Receive average bitrate: {:.2f} kbps".format(
+        sum(x.size
+            for x in self.data_points) * 8 / stream_duration_receiver /
+        1000))
+
+  def remove_reordered(self):
+    last = self.data_points[0]
+    data_points_ordered = [last]
+    for x in self.data_points[1:]:
+      if x.seq_no > last.seq_no and (x.real_send_time_ms >
+                                     last.real_send_time_ms):
+        data_points_ordered.append(x)
+        last = x
+    self.data_points = data_points_ordered
+
+  def compute_bandwidth(self):
+    """Computes bandwidth averaged over several consecutive packets.
+
+    The number of consecutive packets used in the average is
+    BANDWIDTH_SMOOTHING_WINDOW_SIZE. Averaging is done in numpy by a
+    FFT convolution.

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

I don't think numpy actually uses an FFT implementation for filters as small as
10 (this is dynamically chosen, depending on the size of the filter), so please
remove the FFT comment.

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

Done.

+    """
+    self.bandwidth_kbps = []
+    for i in range(len(self.data_points)-1):
+      self.bandwidth_kbps.append(
+          self.data_points[i].size*8 / (self.data_points[i+1].real_send_time_ms
+                                        - self.data_points[i].real_send_time_ms)
+      )
+    convolve_filter = (numpy.ones(
+        RTPStatistics.BANDWIDTH_SMOOTHING_WINDOW_SIZE) /
+                       RTPStatistics.BANDWIDTH_SMOOTHING_WINDOW_SIZE)
+    self.smooth_bw_kbps = numpy.convolve(self.bandwidth_kbps, convolve_filter)

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

Please use numpy.correlate here (and update comments and variable names), since
the mirroring of the filter is not useful here.

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

Done.

+
+  def plot_statistics(self):
+    """Plots changes in delay and average bandwidth."""
+    plt.figure(1)
+    plt.plot([f.real_send_time_ms/1000 for f in self.data_points],
+             [f.absdelay for f in self.data_points])
+    plt.xlabel("Send time [s]")
+    plt.ylabel("Relative transport delay [ms]")
+
+    plt.figure(2)
+    plt.plot([f.real_send_time_ms / 1000 for f in
+              self.data_points][:len(self.smooth_bw_kbps)],

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

Formatting seems off here, please check.

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

No, it's right. pylint and the presubmit test would complain otherwise.

+             self.smooth_bw_kbps[:len(self.data_points)])
+    plt.xlabel("Send time [s]")
+    plt.ylabel("Bandwidth [kbps]")
+
+    plt.show()
+
+
+def main():
+
+  if len(sys.argv) < 2:
+    print("Usage: python rtp_analyzer.py <filename of rtc event log>")
+    sys.exit(0)
+
+  data_points = pb_parse.parse_protobuf(sys.argv[1])
+  rtp_stats = RTPStatistics(data_points)
+  chosen_ssrc = rtp_stats.choose_ssrc()
+  print("Chosen SSRC: 0X{:X}".format(chosen_ssrc))
+
+  rtp_stats.filter_ssrc(chosen_ssrc)
+  print("Statistics:")
+  rtp_stats.print_seq_no_statistics()
+  rtp_stats.print_frequency_duration_statistics()
+  rtp_stats.remove_reordered()
+  rtp_stats.compute_bandwidth()
+  rtp_stats.plot_statistics()
+
+if __name__ == "__main__":
+  main()