Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1999113002:
New rtc dump analyzing tool in Python (Closed)
DescriptionNew rtc dump analyzing tool in Python
R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org
Committed: https://crrev.com/7ebbf9007706ba51be44e065a7c14f18a6d2b009
Cr-Commit-Position: refs/heads/master@{#13218}
Patch Set 1 #Patch Set 2 : Fixed space for presubmit checks (I think) #Patch Set 3 : pylint issues #
Total comments: 112
Patch Set 4 : Response to comments, replace own with numpy #Patch Set 5 : division and floats #Patch Set 6 : Small changes, responses to comments. #Patch Set 7 : Updated README to reflect changes in code. #
Total comments: 60
Patch Set 8 : Fraction dictionaries and other small fixes #
Total comments: 18
Patch Set 9 : Return values, user interface, spaces around operators #Patch Set 10 : misc.uwrap doc string, README protoc call #
Total comments: 3
Patch Set 11 : Unit test for misc.unwrap #
Total comments: 5
Patch Set 12 : Moved unit test. #
Total comments: 13
Patch Set 13 : Python coding style; build system #Patch Set 14 : Added copying to BUILD.gn #
Total comments: 18
Patch Set 15 : Updated instructions, autotests run automatically. #Patch Set 16 : Formatting, licence and usage string. #
Total comments: 8
Patch Set 17 : Changed default to py-3 #
Total comments: 25
Patch Set 18 : Build system: removed copy script, updated gyp/gn #Patch Set 19 : Removed mention of copy_files.py #Patch Set 20 : Moved to webrtc/tools and updated build system #
Total comments: 12
Patch Set 21 : Removed protobuf dependency from Gyp #Patch Set 22 : Small changes to build in accordance with comments. #Patch Set 23 : Added protobuf dependency back. #Messages
Total messages: 115 (41 generated)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999113002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5710)
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org
This is mostly a re-implementation of https://chromium.googlesource.com/external/webrtc/+/master/tools/matlab/rtpAn... in Python. It is a stand-alone tool that parses an RTC protobuf dump and shows statistics and plots. It is supposed to run in both python 2 and python 3. There are a few issues that doesn't make it quite stand-alone: 1. In python 2, the protobuf file generated by the webrtc build tools (out/Debug/pyproto/webrtc/call/rtc_event_log_pb2.py) has a version incompatible with the system protobuf version. 1. "third_party/protobuf/python/" is a compatible python version. The problem is that the system protobuf version is always used instead. 2. Prepending that folder instead of appending to sys.path does not change this behavior. Disabling the system protobuf version doesn't work either. It cannot be uninstalled on the office desktops without uninstalling important things like 'goobuntu-minimal' 3. Putting everything in a 'venv' introduces other problems. Then matplotlib doesn't work. Installation appears successful, but it doesn't plot. http://stackoverflow.com/a/13982451 says matplotlib should'nt be installed through venv. 2. In python 3, there is no system protobuf, and everything works fine with the gyp-generated protobuf python file. 3. Another probotuf-related issue is this: if 'protoc' from the apt repositories is used, it generates a python-2 protobuf file. 2to3 fails to fix it, but changing =str("", "utf-8")= into just =""= in a few places fixes it. Then it works with the https://pypi.python.org/pypi/python3-protobuf package. 4. Here is the list of required packages: 1. 'future' (only py 2) 2. 'enum34' (only py 2) 3. 'numpy' 4. 'matplotlib' Since it is a little large-ish and has multiple dependencies, I thought about adding some packaging. But I am not sure whether that is a good idea. Please take a look!
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999113002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5740)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999113002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Looks like a useful tool, see my comments below. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:7: where <rtc event log> is a protobuf dump of the RTC event stream. Such I recommend rephrasing this a bit, for example: where <rtc event log> is a recorded RTC event log, which is stored in protobuf format. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:8: dumps are generated by Chrome through chrome://webrtc-internals I would rephrase to make it more clear that there are multiple ways to generate the RTC event logs (depending on which app/program you're using). For example: Such dumps can be generated in multiple ways, for example by Chrome through the chrome://webrtc-internals page. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:18: *matplotlib* (http://matplotlib.org/). These libraries can probably be I don't think it's safe to assume all of our users use linux, so I think these lines can be removed (These libraries...Debian-based systems). https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:25: If you use python 2, you also need *enum34* This might be a small/personal thing, but I prefer not to use "you" when it's not really necessary. You could rephrase this as: When using python 2, the package *enum34* and *future* are also needed. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:30: You also need a version of Protocol Buffers. One is distributed along Same here, I would recommend: A version of the Protocol Buffer library is needed as well. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:35: use it, you have to compile WebRTC before using this tool. To generate it, WebRTC should be compiled before using this tool. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:38: Protocol Buffers and the `protoc`, the Protocol Buffer compiler. On "the" should be removed here, so: ...Buffers and 'protoc', the... https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:39: Debian-based systems, it is Again, too linux/debian specific. Would remove: On Debian-based...python-protobuf https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:9: """Utility functions for statistics calculations. I would reorder this into: Utility functions for calculating statistics. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:16: def count_reordered(seq_no_s): please rename seq_no_s to sequence_numbers https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:39: d[dt] += 1 These 3 lines can be replaced by: d = collections.Counter(data) https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:50: bins with even proportion of data points in each. Is there a difference between this and numpy.histogram? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:59: values in data inside this Please add a comment on the boundary values (included in the range or not?) https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:66: if cumulative >= 1/float(n_bins): Is this correct? What if the list contains only very large numbers? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:79: an integer multiple of `mod` is added to one of the elements. By reading the code below it looks like the 'integer multiple' is at most one and it is always added to the latter of the consecutive elements. Perhaps that should be in the description? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:28: DIR = enum.Enum("Direction", "incoming outgoing") I would prefer a less fancy solution that does not add a dependency to the code. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:44: first2header_bytes, self.seq_no, self.timestamp, self.ssrc = unpacked The temporary 'unpacked' variable can be removed here. Please rename seq_no to sequence_number. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:56: _, _, _, _, _, self.pt = bit_unpack([2, 1, 1, 4, 1, 7], first2header_bytes) 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. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:57: self.direction = direction Move to line 42 please. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:67: all RtpPacket events from the event stream as a list of DataPoint:s Remove the : please. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:29: """Initializes data_points and does computations. I suggest merging this with the comment below, so: Initializes data_points and calculates statistics for ... https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:31: Calculates statistics for number of packages and size of packages package -> packet https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:35: data_points: list of pb_parse.DataPoint:s on which statistics are Remove the : in DataPoint:s please. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:40: # currently does nothing, because parse_protobuf() only returns RTP packages Please start comment with a capital and end with a . https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:41: no_rtcp_packages = [x for x in data_points if not 72 <= x.pt <= 76] 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. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:87: assert self.ssrc_frequencies This should be at the top of the function. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:125: def print_seq_no_statistics(self): rename to print_sequence_number_statistics https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:153: if abs((fs_est - f)/float(f)) < 0.05: Why not just use the closest one to the estimated frequency? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:174: stream_duration_receiver = (self.data_points[-1].arrival_timestamp_ms - Packet reordering could make this incorrect, max/min would be more accurate. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:208: FFT convolution. 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. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:219: self.smooth_bw_kbps = numpy.convolve(self.bandwidth_kbps, convolve_filter) Please use numpy.correlate here (and update comments and variable names), since the mirroring of the filter is not useful here. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:231: self.data_points][:len(self.smooth_bw_kbps)], Formatting seems off here, please check.
Either link to a bug, or remove BUG= from the commit message. Also, the Google Python style guide says to indent 4 spaces (the same as PEP 8.) Some of the nontrivial functions in misc could really use some unit tests. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:16: def count_reordered(seq_no_s): A short comment that describes what this function does? And all the other functions... https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:22: largest = max(largest, seq_no) Nit: "else: largest = seq_no" instead of this statement. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:30: total = sum(d.values()) float(sum(d.values())) here, so that you don't have to call float() in each iteration of the loop. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:50: bins with even proportion of data points in each. Not sure I understand. The intervals will have different sizes, to make the proportion of data points in each bin as close to 1/n_bins as possible? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:59: values in data inside this Are the intervals open, closed, or something else? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:64: for i in sorted(data): You sort data twice. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:64: for i in sorted(data): i is a bad name for a variable that's not an index. Or is data really supposed to be a sequence of indexes? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:65: cumulative += data[i] You add data[i] here rather than 1, using the elements of data as indexes into data. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:66: if cumulative >= 1/float(n_bins): cumulative and n_bins are both integers, right, with n_bins > 0? If so, this condition is equivalent to cumulative > 0. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:68: start = i+1 You surround binary operators with spaces almost but not quite everywhere. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:79: an integer multiple of `mod` is added to one of the elements. I think this is under-specified. What integer multiple, and to which element? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:24: sys.path.append(os.path.join(webrtc_root, "third_party/protobuf/python/")) 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/"]] ? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:28: DIR = enum.Enum("Direction", "incoming outgoing") Why is this in all caps? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:44: first2header_bytes, self.seq_no, self.timestamp, self.ssrc = unpacked I find it's good for readability to surround tuples by parentheses. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:47: """unpacks a big-endian number into blocks of bits.""" This doesn't sound right. Python integers are always host-endian; struct.unpack_from has done endian conversion for you. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:56: _, _, _, _, _, self.pt = bit_unpack([2, 1, 1, 4, 1, 7], first2header_bytes) 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... https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:81: direction)) 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. :-) https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:23: """Acts as namespace for RTP statistics. 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. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:29: """Initializes data_points and does computations. This sounds misleading. __init__ doesn't do anything to data_points, does it? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:41: no_rtcp_packages = [x for x in data_points if not 72 <= x.pt <= 76] "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? https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:47: data_points = no_rtcp_packages Please don't reuse variable names lightly. It makes the code harder to read. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:57: """Prints packet and size statistics for given SSRC. Explain the other argument. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:103: return chosen_ssrc 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. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:115: 2**16-1) # 65535 This comment is probably not that useful. You've picked a self-documenting format for the number in the code. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:121: 2**32-1)) # 4294967295 Remove this comment too. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:123: self.data_points[i].timestamp = timestamp_unwrap_value 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. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:129: len(set(sortseq_no)) You can get the min and max elements without sorting the sequence numbers. Also, you convert to a set twice. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:131: print("Duplicated packets: {}".format(sortseq_no.count(0))) 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.)
Changes according to comments. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:7: where <rtc event log> is a protobuf dump of the RTC event stream. Such On 2016/05/24 11:50:17, ivoc wrote: > I recommend rephrasing this a bit, for example: where <rtc event log> is a > recorded RTC event log, which is stored in protobuf format. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:8: dumps are generated by Chrome through chrome://webrtc-internals On 2016/05/24 11:50:17, ivoc wrote: > I would rephrase to make it more clear that there are multiple ways to generate > the RTC event logs (depending on which app/program you're using). For example: > Such dumps can be generated in multiple ways, for example by Chrome through the > chrome://webrtc-internals page. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:18: *matplotlib* (http://matplotlib.org/). These libraries can probably be On 2016/05/24 11:50:17, ivoc wrote: > I don't think it's safe to assume all of our users use linux, so I think these > lines can be removed (These libraries...Debian-based systems). Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:25: If you use python 2, you also need *enum34* On 2016/05/24 11:50:17, ivoc wrote: > This might be a small/personal thing, but I prefer not to use "you" when it's > not really necessary. You could rephrase this as: When using python 2, the > package *enum34* and *future* are also needed. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:30: You also need a version of Protocol Buffers. One is distributed along On 2016/05/24 11:50:17, ivoc wrote: > Same here, I would recommend: A version of the Protocol Buffer library is needed > as well. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:35: use it, you have to compile WebRTC before using this tool. On 2016/05/24 11:50:17, ivoc wrote: > To generate it, WebRTC should be compiled before using this tool. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:38: Protocol Buffers and the `protoc`, the Protocol Buffer compiler. On On 2016/05/24 11:50:17, ivoc wrote: > "the" should be removed here, so: ...Buffers and 'protoc', the... Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/README:39: Debian-based systems, it is On 2016/05/24 11:50:17, ivoc wrote: > Again, too linux/debian specific. Would remove: On > Debian-based...python-protobuf Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:9: """Utility functions for statistics calculations. On 2016/05/24 11:50:17, ivoc wrote: > I would reorder this into: Utility functions for calculating statistics. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:16: def count_reordered(seq_no_s): On 2016/05/24 11:50:18, ivoc wrote: > please rename seq_no_s to sequence_numbers Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:16: def count_reordered(seq_no_s): On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > A short comment that describes what this function does? And all the other > functions... A little better now, I think https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:22: largest = max(largest, seq_no) On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > Nit: "else: largest = seq_no" instead of this statement. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:30: total = sum(d.values()) On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > float(sum(d.values())) here, so that you don't have to call float() in each > iteration of the loop. removed "float" and added "from __future__ import division" on top instead https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:39: d[dt] += 1 On 2016/05/24 11:50:17, ivoc wrote: > These 3 lines can be replaced by: d = collections.Counter(data) Thank you! https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:50: bins with even proportion of data points in each. On 2016/05/24 11:50:18, ivoc wrote: > Is there a difference between this and numpy.histogram? I got about the same result with numpy.histogram. hists is deleted. Thank you for the pointer! https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:59: values in data inside this On 2016/05/24 11:50:17, ivoc wrote: > Please add a comment on the boundary values (included in the range or not?) not longer relevant https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:64: for i in sorted(data): On 2016/05/24 12:09:50, kwiberg-webrtc wrote: > i is a bad name for a variable that's not an index. Or is data really supposed > to be a sequence of indexes? here too https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:65: cumulative += data[i] On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > You add data[i] here rather than 1, using the elements of data as indexes into > data. here too https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:66: if cumulative >= 1/float(n_bins): On 2016/05/24 11:50:17, ivoc wrote: > Is this correct? What if the list contains only very large numbers? here too https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:68: start = i+1 On 2016/05/24 12:09:50, kwiberg-webrtc wrote: > You surround binary operators with spaces almost but not quite everywhere. here too https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/misc.py:79: an integer multiple of `mod` is added to one of the elements. On 2016/05/24 12:09:50, kwiberg-webrtc wrote: > I think this is under-specified. What integer multiple, and to which element? unwrap should work like matlab's unwrap (http://se.mathworks.com/help/matlab/ref/unwrap.html), but for integers. I added more description, but it is still a little unclear. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:24: sys.path.append(os.path.join(webrtc_root, "third_party/protobuf/python/")) On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > 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/"]] > > ? 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. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:28: DIR = enum.Enum("Direction", "incoming outgoing") On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > Why is this in all caps? Got convinced by Ivo and removed. Directions are not used at all, so I removed it. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:44: first2header_bytes, self.seq_no, self.timestamp, self.ssrc = unpacked On 2016/05/24 11:50:18, ivoc wrote: > The temporary 'unpacked' variable can be removed here. Please rename seq_no to > sequence_number. Acknowledged. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:47: """unpacks a big-endian number into blocks of bits.""" On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > This doesn't sound right. Python integers are always host-endian; > struct.unpack_from has done endian conversion for you. Yes, thank you. Anyway, I removed it completely by your and Ivo's suggestions. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:56: _, _, _, _, _, self.pt = bit_unpack([2, 1, 1, 4, 1, 7], first2header_bytes) On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > 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... Thank you! Changed to "& 0b01111111". I think it's a little more readable. Your hex arithmetic was correct. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:57: self.direction = direction On 2016/05/24 11:50:18, ivoc wrote: > Move to line 42 please. completely removed because not used. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:67: all RtpPacket events from the event stream as a list of DataPoint:s On 2016/05/24 11:50:18, ivoc wrote: > Remove the : please. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:81: direction)) On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > 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. :-) Thank you, that's nicer! https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:23: """Acts as namespace for RTP statistics. On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > 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. Changed. Still not really satisfied with the comment. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:29: """Initializes data_points and does computations. On 2016/05/24 11:50:18, ivoc wrote: > I suggest merging this with the comment below, so: Initializes data_points and > calculates statistics for ... Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:29: """Initializes data_points and does computations. On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > This sounds misleading. __init__ doesn't do anything to data_points, does it? Hopefully a little better now. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:31: Calculates statistics for number of packages and size of packages On 2016/05/24 11:50:18, ivoc wrote: > package -> packet Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:35: data_points: list of pb_parse.DataPoint:s on which statistics are On 2016/05/24 11:50:18, ivoc wrote: > Remove the : in DataPoint:s please. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:40: # currently does nothing, because parse_protobuf() only returns RTP packages On 2016/05/24 11:50:18, ivoc wrote: > Please start comment with a capital and end with a . Removed completely. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:41: no_rtcp_packages = [x for x in data_points if not 72 <= x.pt <= 76] On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > "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? removed, because RTCP were filtered in protobuf already. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:57: """Prints packet and size statistics for given SSRC. On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > Explain the other argument. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:87: assert self.ssrc_frequencies On 2016/05/24 11:50:18, ivoc wrote: > This should be at the top of the function. Was not really needed, because constructor initializes ssrc_frequencies https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:103: return chosen_ssrc On 2016/05/24 12:09:52, kwiberg-webrtc wrote: > 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. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:115: 2**16-1) # 65535 On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > This comment is probably not that useful. You've picked a self-documenting > format for the number in the code. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:121: 2**32-1)) # 4294967295 On 2016/05/24 12:09:52, kwiberg-webrtc wrote: > Remove this comment too. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:123: self.data_points[i].timestamp = timestamp_unwrap_value On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > 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. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:125: def print_seq_no_statistics(self): On 2016/05/24 11:50:18, ivoc wrote: > rename to print_sequence_number_statistics Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:129: len(set(sortseq_no)) On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > You can get the min and max elements without sorting the sequence numbers. > > Also, you convert to a set twice. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:131: print("Duplicated packets: {}".format(sortseq_no.count(0))) On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > 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.) Yes, that was wrong. Fixed now! https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:153: if abs((fs_est - f)/float(f)) < 0.05: On 2016/05/24 11:50:18, ivoc wrote: > Why not just use the closest one to the estimated frequency? To notify the user that something is odd when the estimated frequency is far off. Added this notification. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:174: stream_duration_receiver = (self.data_points[-1].arrival_timestamp_ms - On 2016/05/24 11:50:18, ivoc wrote: > Packet reordering could make this incorrect, max/min would be more accurate. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:208: FFT convolution. On 2016/05/24 11:50:18, ivoc wrote: > 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. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:219: self.smooth_bw_kbps = numpy.convolve(self.bandwidth_kbps, convolve_filter) On 2016/05/24 11:50:18, ivoc wrote: > Please use numpy.correlate here (and update comments and variable names), since > the mirroring of the filter is not useful here. Done. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/rtp_analyzer.py:231: self.data_points][:len(self.smooth_bw_kbps)], On 2016/05/24 11:50:18, ivoc wrote: > Formatting seems off here, please check. No, it's right. pylint and the presubmit test would complain otherwise.
Description was changed from ========== New rtc dump analyzing tool in Python BUG= ========== to ========== New rtc dump analyzing tool in Python ==========
https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:24: sys.path.append(os.path.join(webrtc_root, "third_party/protobuf/python/")) On 2016/05/24 15:54:41, aleloi wrote: > On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > > 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/"]] > > > > ? > > 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. 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). https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:23: installed through pip, (https://docs.python.org/2.7/installing/). Hmm. You use very few things from this library---just builtins.input and builtins.range if I'm not missing anything. Can you do without it? https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:18: sequence_numbres[i] >= sequence_numbres[i+1] There's an extra space here. Also, Spanish-sounding spelling! https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:27: return result Hmm. return sum(1 for (s1, s2) in zip(sequence_numbers, sequence_numbers[1:]) if s1 >= s2) Or more efficiently, return sum(1 for (s1, s2) in itertools.izip( sequence_numbers, itertools.islice(sequence_numbers, 1, None)) if s1 >= s2) https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:27: return result Unit test this function! https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:38: return d This method is practically identical to percent_table; remove it. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:41: def percent_table(data): The name is misleading, since you give relative frequencies in the [0, 1] range. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:59: 4, 5, 4, 5] This description isn't quite correct. You also add multiples of mod to elements not in a pair with too-large difference (such as the 5th element in the list in your example). Perhaps something like this: "Add integer multiples of mod to all elements of data except the first, such that all pairs of consecutive elements (a, b) satisfy -mod / 2 <= b - a < mod / 2." https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:75: return data Unit test this function! https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:75: return data I think this does the same thing with less overhead: for i in range(1, len(data)): data[i] = (data[i - 1] + (data[i] - data[i - 1] + mod // 2) % mod - mod // 2) https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:42: ) = unpacked Eliminate "unpacked"? (first2header_bytes, self.sequence_number, self.timestamp, self.ssrc) = struct.unpack_from("!HHII", rtp_header_str, 0) https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:32: """Initializes object with data_points computes simple statistics: and https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:67: print("{} 0X{:X} {}, {:.2f}% packets, {:.2f}% data".format( Consider using lowercase hexadecimal. It causes less eye bleeding. :-) https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:69: self.ssrc_size_table[ssrc]*100)) You still have some binary operators not surrounded by spaces. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:83: ssrc_frequencies_lst = list(enumerate(self.ssrc_frequencies)) Eliminate this variable. You don't use it for anything where self.ssrc_frequencies wouldn't work just as well or better. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:132: )) Unless I'm mistaken, it's simpler and more efficient to compute this number as len(self.data_points) - len(seq_no_set). https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:154: if abs((fs_est - f)/float(f)) < 0.05: No need for float, since you import division from __future__. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:162: print("Guessed frequency: {}".format(fs)) Don't print this if the user entered a frequency.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Very nice. I'm not a python programmer, so I cannot review the language use, but see comments on algorithm and misc. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:18: sequence_numbres[i] >= sequence_numbres[i+1] numbres -> numbers, twice. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:22: for sequence_number in sequence_numbers: You could start the loop on the second item, but it doesn't matter in practice. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:30: def ssrc_size_table(data_points): Can't this be implemented in terms of percent_table below? https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:30: def ssrc_size_table(data_points): The naming of this and the next functions are inconsistent. This is called *size* while the next is *percent* -- both return a fraction. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:35: As defined in RFC 3550 section 5.1. Technically, the arrival timestamp is not part of the RFC. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:25: """Has methods for calculating and plotting statistics for RPT RPT -> RTP https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:56: Exception: when different payload types are present in data This will happen, and can be legit. When comfort noise is used, it can either be codec-internal (such as Opus has) which will use the same payload type as the regular speech packets, or be codec-external which uses separate payload types for the speech and noise packets, both under the same SSRC. You will have to handle this case. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:104: numbers and time stamps for the chosen selection. Nit: 'timestamps' is typically written as one word ('sequence numbers' is not). https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:151: fs_vec = [8, 16, 32, 48, 90] # TODO(aleloi) 90 is a hack for video Not really a "hack"; https://tools.ietf.org/html/rfc3551#section-5. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:171: for f in self.data_points: Oh. Python cannot do "vector +/- scalar" without explicitly looping?
https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:171: for f in self.data_points: On 2016/05/25 12:54:23, hlundin-webrtc wrote: > Oh. Python cannot do "vector +/- scalar" without explicitly looping? No. list + list returns a new list that's a concatenation of the two argument lists, and list + number is simply disallowed.
https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:171: for f in self.data_points: On 2016/05/25 13:00:31, kwiberg-webrtc wrote: > On 2016/05/25 12:54:23, hlundin-webrtc wrote: > > Oh. Python cannot do "vector +/- scalar" without explicitly looping? > > No. list + list returns a new list that's a concatenation of the two argument > lists, and list + number is simply disallowed. It could do it if it was a numpy array instead of a list, but in this case it's actually a list of objects, so I don't think that would work in matlab either.
https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:171: for f in self.data_points: On 2016/05/25 13:17:37, ivoc wrote: > On 2016/05/25 13:00:31, kwiberg-webrtc wrote: > > On 2016/05/25 12:54:23, hlundin-webrtc wrote: > > > Oh. Python cannot do "vector +/- scalar" without explicitly looping? > > > > No. list + list returns a new list that's a concatenation of the two argument > > lists, and list + number is simply disallowed. > > It could do it if it was a numpy array instead of a list, Right, I'd forgotten about numpy, because I never use it---but if you're coming from Matlab, that's probably the first thing you should look at once you're past the initial Python tutorial.
Nice code! I definitely learned more python programming just by reading this! I have some comments though. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:20: largest = sequence_numbers[0] I also think the numbres<->numbers confusion will cause a runtime error on this line. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:22: for sequence_number in sequence_numbers: I don't think this tests really what the method states it tests. For instance (assuming increasing sequence numbers) 0 5 3 2 would only be counted as 1 reordered number but according to the method specification a reordered number is when sequence_numbres[i] >= sequence_numbres[i+1] which would mean that both 5 and 3 are reordered which gives a count of 2. Please either change the reordering counting, or change the method description to match what is done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:46: total = sum(d.values()) This looks very similar to lines 35-38. It would be nice to unify these methods to avoid code duplication. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:47: """Parses rtc event log from protobuf file. I think the abbreviation rtc should be capitalized. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:53: all RtpPacket events from the event stream as a list of DataPoints Rtp should be capitalized according to how it is capitalized elsewhere herein. My suggestion would be to write this as RTP packet, or if matching the field name below, as rtp_packet. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:49: """Prints packet and size statistics for given SSRC. Maybe "the" or "a" should be added between "for" and "given"? https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:136: def print_frequency_duration_statistics(self): Suggestion: This is a fairly long method which does many things, I think it may benefit in terms of readability and encapsulation if it is split into separate methods calls. For instance, the computation of the estimated frequency could be split into a non-class member method declarable as. def estimate_frequency(f,data_points): ... return fs https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:151: fs_vec = [8, 16, 32, 48, 90] # TODO(aleloi) 90 is a hack for video The python style guide actually has an example where a comment is not terminated by a period, but I think it would be nice with a terminating period in this comment.
aleloi@google.com changed reviewers: + aleloi@google.com
Responses to comments and fixes. Function bodies and docstrings should now agree in most places. Simplified the frequency dictionaries in misc. https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/40001/tools/py_event_log_analyz... tools/py_event_log_analyzer/pb_parse.py:24: sys.path.append(os.path.join(webrtc_root, "third_party/protobuf/python/")) On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > On 2016/05/24 15:54:41, aleloi wrote: > > On 2016/05/24 12:09:51, kwiberg-webrtc wrote: > > > 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/"]] > > > > > > ? > > > > 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. > > 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). Thank you! This is better. Ignore the latest comment then; I misunderstood. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:23: installed through pip, (https://docs.python.org/2.7/installing/). On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > Hmm. You use very few things from this library---just builtins.input and > builtins.range if I'm not missing anything. Can you do without it? Yes, I changed bultins.range to just 'range', which should be fine because the only resevant difference is memory consumption. We already keep all of the data in memory. I replaced the builtins.input dependency with a custom input function in latest misc.py https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:22: for sequence_number in sequence_numbers: On 2016/05/26 06:44:18, peah-webrtc wrote: > I don't think this tests really what the method states it tests. > For instance (assuming increasing sequence numbers) > 0 5 3 2 > would only be counted as 1 reordered number but according to the method > specification > a reordered number is when sequence_numbres[i] >= sequence_numbres[i+1] > which would mean that both 5 and 3 are reordered which gives a count of 2. > > Please either change the reordering counting, or change the method description > to match what is done. I rewrote it to match the specification. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:27: return result On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > Hmm. > > return sum(1 for (s1, s2) in zip(sequence_numbers, > sequence_numbers[1:]) > if s1 >= s2) > > Or more efficiently, > > return sum(1 for (s1, s2) in itertools.izip( > sequence_numbers, > itertools.islice(sequence_numbers, 1, None)) > if s1 >= s2) Your suggestion is simpler to read. I went with the first one, because it is easier to read, and because I thought it was the simplest way to handle py2/3 differences. py3's zip behaves as py2's itertools.izip (which doesn't exist in py3). https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:30: def ssrc_size_table(data_points): On 2016/05/25 12:54:23, hlundin-webrtc wrote: > Can't this be implemented in terms of percent_table below? Both changed in next version. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:38: return d On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > This method is practically identical to percent_table; remove it. See next version! https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:59: 4, 5, 4, 5] On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > This description isn't quite correct. You also add multiples of mod to elements > not in a pair with too-large difference (such as the 5th element in the list in > your example). Perhaps something like this: > > "Add integer multiples of mod to all elements of data except the first, such > that all pairs of consecutive elements (a, b) satisfy -mod / 2 <= b - a < mod / > 2." Thank you, updated! https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:75: return data On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > I think this does the same thing with less overhead: > > for i in range(1, len(data)): > data[i] = (data[i - 1] + > (data[i] - data[i - 1] + mod // 2) % mod - mod // 2) Yes, it does the same thing simpler. Changed! https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:35: As defined in RFC 3550 section 5.1. On 2016/05/25 12:54:23, hlundin-webrtc wrote: > Technically, the arrival timestamp is not part of the RFC. Updated a little. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:42: ) = unpacked On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > Eliminate "unpacked"? > > (first2header_bytes, self.sequence_number, self.timestamp, > self.ssrc) = struct.unpack_from("!HHII", rtp_header_str, 0) Done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:47: """Parses rtc event log from protobuf file. On 2016/05/26 06:44:18, peah-webrtc wrote: > I think the abbreviation rtc should be capitalized. Done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:53: all RtpPacket events from the event stream as a list of DataPoints On 2016/05/26 06:44:18, peah-webrtc wrote: > Rtp should be capitalized according to how it is capitalized elsewhere herein. > My suggestion would be to write this as RTP packet, or if matching the field > name below, as rtp_packet. Done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:32: """Initializes object with data_points computes simple statistics: On 2016/05/25 12:42:42, kwiberg-webrtc wrote: > and Done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:49: """Prints packet and size statistics for given SSRC. On 2016/05/26 06:44:18, peah-webrtc wrote: > Maybe "the" or "a" should be added between "for" and "given"? Done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:56: Exception: when different payload types are present in data On 2016/05/25 12:54:23, hlundin-webrtc wrote: > This will happen, and can be legit. When comfort noise is used, it can either be > codec-internal (such as Opus has) which will use the same payload type as the > regular speech packets, or be codec-external which uses separate payload types > for the speech and noise packets, both under the same SSRC. You will have to > handle this case. Now it prints the different payload types instead of raising an exception. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:67: print("{} 0X{:X} {}, {:.2f}% packets, {:.2f}% data".format( On 2016/05/25 12:42:42, kwiberg-webrtc wrote: > Consider using lowercase hexadecimal. It causes less eye bleeding. :-) Ok. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:69: self.ssrc_size_table[ssrc]*100)) On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > You still have some binary operators not surrounded by spaces. Done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:83: ssrc_frequencies_lst = list(enumerate(self.ssrc_frequencies)) On 2016/05/25 12:42:42, kwiberg-webrtc wrote: > Eliminate this variable. You don't use it for anything where > self.ssrc_frequencies wouldn't work just as well or better. It was used to pair ssrc with an integer counter, which came from the iteration order of dictionary keys. I checked the documentation to make sure that the iteration order stays the same for list(d) and enumerate(d). https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:104: numbers and time stamps for the chosen selection. On 2016/05/25 12:54:23, hlundin-webrtc wrote: > Nit: 'timestamps' is typically written as one word ('sequence numbers' is not). Done. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:132: )) On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > Unless I'm mistaken, it's simpler and more efficient to compute this number as > len(self.data_points) - len(seq_no_set). I agree. Changed. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:136: def print_frequency_duration_statistics(self): On 2016/05/26 06:44:19, peah-webrtc wrote: > Suggestion: > This is a fairly long method which does many things, I think it may benefit in > terms of readability and encapsulation if it is split into separate methods > calls. > > For instance, the computation of the estimated frequency could be split into a > non-class member method declarable as. > > def estimate_frequency(f,data_points): > ... > return fs > I agree. I divided it into a frequency related and a duration/bitrate related part. I didn't do exactly as you said: both new parts are member functions. There is no strong reason for it, but I think it looks more uniform this way. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:154: if abs((fs_est - f)/float(f)) < 0.05: On 2016/05/25 12:42:42, kwiberg-webrtc wrote: > No need for float, since you import division from __future__. OK. https://codereview.webrtc.org/1999113002/diff/120001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:162: print("Guessed frequency: {}".format(fs)) On 2016/05/25 12:42:41, kwiberg-webrtc wrote: > Don't print this if the user entered a frequency. Done.
Very nearly out of things to complain about. :-) https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:33: print(d) This looks like debug code. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:45: return counter The standard operating procedure in Python is to either modify the argument or return a modified value. If you do both, call sites (such as the one on line 34) look like they don't modify the argument even though they do, which can cause bugs later. Given that performance probably isn't a top priority here, I'd recommend that you make this function return a new value. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:62: return data Here too. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:60: ssrc_id, ssrc, payload_info, self.ssrc_frequencies[ssrc]*100, Spaces around * https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:63: bin_counts, bin_bounds = numpy.histogram([x.size for x in (bin_counts, bin_bounds) = as you have done in other places would be nice. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:81: for i, ssrc in enumerate(self.ssrc_frequencies): Parenthesis around the tuple here too. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:149: print("Guessed frequency: {}".format(fs)) In all three messages, it'll probably be useful to state that the frequency is in Hz. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:189: 1000)) You can extract a substantial common subexpression from these two. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:209: for i in range(len(self.data_points)-1): Spaces around - https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:211: self.data_points[i].size*8 / (self.data_points[i+1].real_send_time_ms Spaces around * and + https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:222: plt.plot([f.real_send_time_ms/1000 for f in self.data_points], Spaces around /
https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:33: print(d) On 2016/05/31 09:04:47, kwiberg-webrtc wrote: > This looks like debug code. Removed, thank you! (I did run the code before uploading, but somehow missed this). https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:45: return counter On 2016/05/31 09:04:47, kwiberg-webrtc wrote: > The standard operating procedure in Python is to either modify the argument or > return a modified value. If you do both, call sites (such as the one on line 34) > look like they don't modify the argument even though they do, which can cause > bugs later. > > Given that performance probably isn't a top priority here, I'd recommend that > you make this function return a new value. Done. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:62: return data On 2016/05/31 09:04:47, kwiberg-webrtc wrote: > Here too. Done. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:60: ssrc_id, ssrc, payload_info, self.ssrc_frequencies[ssrc]*100, On 2016/05/31 09:04:47, kwiberg-webrtc wrote: > Spaces around * Added whitespace here and in rest of CL. Exponentiation and unary minus have no spaces though. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:149: print("Guessed frequency: {}".format(fs)) On 2016/05/31 09:04:48, kwiberg-webrtc wrote: > In all three messages, it'll probably be useful to state that the frequency is > in Hz. Yes, in particular in the input query. https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:189: 1000)) On 2016/05/31 09:04:47, kwiberg-webrtc wrote: > You can extract a substantial common subexpression from these two. Done.
lgtm Well done! https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/140001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:60: ssrc_id, ssrc, payload_info, self.ssrc_frequencies[ssrc]*100, On 2016/05/31 09:49:48, aleloi wrote: > On 2016/05/31 09:04:47, kwiberg-webrtc wrote: > > Spaces around * > > Added whitespace here and in rest of CL. Exponentiation and unary minus have no > spaces though. OK. A bit inconsistent to have one binary operator treated differently from the rest, but since this happens to match my personal preference for ** I'll let it slide. :-) https://codereview.webrtc.org/1999113002/diff/180001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/180001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:42: return {key: counter[key] / total for key in counter} return {key: val / total for (key, val) in counter.items()} Is simpler and might also be faster, since you don't have to do dict lookups. (But in Python 2 you probably end up losing performance for using items instead of iteritems.) https://codereview.webrtc.org/1999113002/diff/180001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/180001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:149: print("Guessed frequency: {}kHz".format(fs)) I think it's customary to have a space between number and unit. It's readable either way, though, so your call. https://codereview.webrtc.org/1999113002/diff/180001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:210: 1].real_send_time_ms - There's a really unfortunate line break here. Consider inserting a line break after the opening parenthesis on line 208 instead.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1999113002/#ps200001 (title: "Unit test for misc.unwrap")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999113002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5969)
aleloi@webrtc.org changed reviewers: + kjellander@webrtc.org, phoglund@webrtc.org
aleloi@webrtc.org changed reviewers: - aleloi@google.com, kjellander@webrtc.org
(To Patrik being the owner): can you please take a look at this new analyzing tool?
LGTM, nice work!
https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... File tools/py_event_log_analyzer/analyzer_unittest.py (right): https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... tools/py_event_log_analyzer/analyzer_unittest.py:52: self.assertEqual(random_data, random_data_copy) Excellent. But could you put the unit test in the same file as the code you're testing? That'll make it easier to find. https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:60: return lst return [data[0]] + [a + (b - a + mod // 2) % mod - mod // 2 for (a, b) in zip(data, data[1:])] I think. Hopefully your unit test can check it...
https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... File tools/py_event_log_analyzer/analyzer_unittest.py (right): https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... tools/py_event_log_analyzer/analyzer_unittest.py:52: self.assertEqual(random_data, random_data_copy) On 2016/05/31 12:30:53, kwiberg-webrtc wrote: > Excellent. But could you put the unit test in the same file as the code you're > testing? That'll make it easier to find. Moved in new version. https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:60: return lst On 2016/05/31 12:30:53, kwiberg-webrtc wrote: > return [data[0]] + [a + (b - a + mod // 2) % mod - mod // 2 > for (a, b) in zip(data, data[1:])] > > I think. Hopefully your unit test can check it... No, wont work, because 'a' comes from the original list. 'a' needs to come from the result.
https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/200001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:60: return lst On 2016/05/31 12:40:32, aleloi wrote: > On 2016/05/31 12:30:53, kwiberg-webrtc wrote: > > return [data[0]] + [a + (b - a + mod // 2) % mod - mod // 2 > > for (a, b) in zip(data, data[1:])] > > > > I think. Hopefully your unit test can check it... > > No, wont work, because 'a' comes from the original list. 'a' needs to come from > the result. Ah, right. Then it's not possible to write as just a list comprehension, since we can't look at previous elements. Something like return itertools.accumulate([data[0]] + [(b - a + mod // 2) % mod - mod // 2 for x in zip(data, data[1:])], operator.add) would work, if itertools.accumulate had existed in Python 2. But let's use your version. ;-)
https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:26: def count_reordered(sequence_numbers): You sure this passes the linter? We should always have two blank lines before top-level functions or classes. Apply throughout. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:28: sequence_numbers[i] >= sequence_numbers[i+1] Try to make this fit on line 27. Docstrings should always be on the form """Title that explains the gist of the function. More stuff here in case the first line is longer than 80 chars. """ Apply throughout https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:39: d = collections.defaultdict(int) You would get away with this in go, but python culture suggests names should be a bit longer. "result" or "mapping" sounds about right. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:40: for pt in data_points: Same here, "point" https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:55: """Returns `data` unwrapped modulo `mod`. Does not modify data. This docstring is good, except the blank line on line 63. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:78: class TestMisc(unittest.TestCase): Don't mix production code and unit tests; just move this to misc_unittest.py and update the comment at the top. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:80: def testUnwrap(self): This test tests at least 5 different things, so it should be at least 5 tests. A rule of thumb is "a test should have one reason to break". There is not even any state in this file so breaking up into smaller, well-named test method should be trivial. For instance, def testDataShouldNotChangeAfterUnwrap(self): is one good name, and you can even get rid of the comment on #91! The above name makes it easy for a maintainer to see at a glance what the test expected to happen. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:10: """ Nit: pull up line #10 to #9. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:19: # makes sure to import correct protobuf Nit: uppercase M, end with . https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:21: ["out/Debug/pyproto/webrtc/call/", We should not do import hacks, and especially not if they build on brittle assumptions as this one. If you build with GN, for instance, things will tend to end up in out/Default (but the user can really choose whatever name they want). This will not even do what you expect for gyp, in all cases. Suppose for instance you build with debug first but then switch to release. Since debug is before release in the path, it will not pick up any protobuf changes if you change + rebuild in release. Furthermore, these scripts do not properly depend on the protobufs, so they count on that the user has hand-built those before running. That's ugly and barbaric, so we need to fix that :) Instead, make gyp/gn rules to copy this script to the appropriate output folder. This means you can't just run the scripts, but you need to "build" them and then run them. If we build, for instance, ninja -C out/Debug rtp_analyzer, all scripts in here are copied to out/Debug. To run, you'd do out/Debug/rtp_analyzer.py From this file you can then just do import pyproto.webrtc.call.rtc_event_log_pb2 as rtc_pb since those files are already in out/Debug. Create a py_event_log_analyzer.gyp file with something like this in it: { 'targets': [ { 'target_name': 'rtp_analyzer', 'type': 'none', 'actions': [ { 'action_name': 'copy_py_scripts', 'variables': { 'copy_output_dir%': '<(PRODUCT_DIR)', } 'inputs': [ '<(DEPTH)/pb_parse.py', '<(DEPTH)/misc.py', # ... etc ... ], 'outputs': [ '<(copy_output_dir)', ], 'action': ['python', '<(DEPTH)/copy_files.py', '--input_dir=<(DEPTH)', '--output_dir=<(copy_output_dir)'], 'message': 'Copying *py files code to <(copy_output_dir)', 'process_outputs_as_sources': 1, }, ], 'dependencies': [ # Depend on protos which makes sure they are built before this # target builds. Not sure if this is the right target to depend # on (does it generate python protobufs?) 'webrtc/webrtc.gyp:rtc_event_log_proto', ], }], ], }, ], } We could also simply invoke cp or cp -r for the action, but this needs to work on Windows, so write a simple python script that copies files from input_dir to output_dir. Note that PRODUCT_DIR will be out/Debug or whatever the user has chosen - don't ever hard-code that! DEPTH is the dir of the gyp file.
lgtm on algorithm. Cannot speak for the Python code quality.
I have made an attempt to update the CL according to Patrik's comments. There are still problems: WebRTC must be built before running the tool, it does not currently work with Python 2. Also, the tool has to be run from the build directory (e.g. out/Debug) to which it is copied. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc.py (right): https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:28: sequence_numbers[i] >= sequence_numbers[i+1] On 2016/05/31 13:47:42, phoglund wrote: > Try to make this fit on line 27. Docstrings should always be on the form > > """Title that explains the gist of the function. > > More stuff here in case the first line is longer than 80 chars. > """ > > Apply throughout Rewrote docstrings everywhere. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc.py:78: class TestMisc(unittest.TestCase): On 2016/05/31 13:47:42, phoglund wrote: > Don't mix production code and unit tests; just move this to misc_unittest.py and > update the comment at the top. Test moved and broken up in pieces. https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:21: ["out/Debug/pyproto/webrtc/call/", On 2016/05/31 13:47:43, phoglund wrote: > We should not do import hacks, and especially not if they build on brittle > assumptions as this one. If you build with GN, for instance, things will tend to > end up in out/Default (but the user can really choose whatever name they want). > > This will not even do what you expect for gyp, in all cases. Suppose for > instance you build with debug first but then switch to release. Since debug is > before release in the path, it will not pick up any protobuf changes if you > change + rebuild in release. Furthermore, these scripts do not properly depend > on the protobufs, so they count on that the user has hand-built those before > running. That's ugly and barbaric, so we need to fix that :) > > Instead, make gyp/gn rules to copy this script to the appropriate output folder. > This means you can't just run the scripts, but you need to "build" them and then > run them. If we build, for instance, ninja -C out/Debug rtp_analyzer, all > scripts in here are copied to out/Debug. To run, you'd do > out/Debug/rtp_analyzer.py > > From this file you can then just do > > import pyproto.webrtc.call.rtc_event_log_pb2 as rtc_pb > > since those files are already in out/Debug. > > Create a py_event_log_analyzer.gyp file with something like this in it: > > { 'targets': [ > { > 'target_name': 'rtp_analyzer', > 'type': 'none', > 'actions': [ > { > 'action_name': 'copy_py_scripts', > 'variables': { > 'copy_output_dir%': '<(PRODUCT_DIR)', > } > 'inputs': [ > '<(DEPTH)/pb_parse.py', > '<(DEPTH)/misc.py', > # ... etc ... > ], > 'outputs': [ > '<(copy_output_dir)', > ], > 'action': ['python', > '<(DEPTH)/copy_files.py', > '--input_dir=<(DEPTH)', > '--output_dir=<(copy_output_dir)'], > 'message': 'Copying *py files code to <(copy_output_dir)', > 'process_outputs_as_sources': 1, > }, > ], > 'dependencies': [ > # Depend on protos which makes sure they are built before this > # target builds. Not sure if this is the right target to depend > # on (does it generate python protobufs?) > 'webrtc/webrtc.gyp:rtc_event_log_proto', > ], > }], > ], > }, > ], > } > > We could also simply invoke cp or cp -r for the action, but this needs to work > on Windows, so write a simple python script that copies files from input_dir to > output_dir. Note that PRODUCT_DIR will be out/Debug or whatever the user has > chosen - don't ever hard-code that! DEPTH is the dir of the gyp file. I added the changes to /webrtc/webrtc.gyp and /webrtc/BUILD.gn. Both gyp and gn seem to work as intended, but there are two python problems: python2 won't import pyproto.webrtc.call..., because the folders lack __init__.py files. ../../third_party/protobuf/python still needs to be added to path.
On 2016/06/02 14:01:57, aleloi wrote: > I have made an attempt to update the CL according to Patrik's comments. There > are still problems: WebRTC must be built before running the tool, it does not > currently work with Python 2. Also, the tool has to be run from the build > directory (e.g. out/Debug) to which it is copied. > > https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... > File tools/py_event_log_analyzer/misc.py (right): > > https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... > tools/py_event_log_analyzer/misc.py:28: sequence_numbers[i] >= > sequence_numbers[i+1] > On 2016/05/31 13:47:42, phoglund wrote: > > Try to make this fit on line 27. Docstrings should always be on the form > > > > """Title that explains the gist of the function. > > > > More stuff here in case the first line is longer than 80 chars. > > """ > > > > Apply throughout > > Rewrote docstrings everywhere. > > https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... > tools/py_event_log_analyzer/misc.py:78: class TestMisc(unittest.TestCase): > On 2016/05/31 13:47:42, phoglund wrote: > > Don't mix production code and unit tests; just move this to misc_unittest.py > and > > update the comment at the top. > > Test moved and broken up in pieces. > > https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... > File tools/py_event_log_analyzer/pb_parse.py (right): > > https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy... > tools/py_event_log_analyzer/pb_parse.py:21: ["out/Debug/pyproto/webrtc/call/", > On 2016/05/31 13:47:43, phoglund wrote: > > We should not do import hacks, and especially not if they build on brittle > > assumptions as this one. If you build with GN, for instance, things will tend > to > > end up in out/Default (but the user can really choose whatever name they > want). > > > > This will not even do what you expect for gyp, in all cases. Suppose for > > instance you build with debug first but then switch to release. Since debug is > > before release in the path, it will not pick up any protobuf changes if you > > change + rebuild in release. Furthermore, these scripts do not properly depend > > on the protobufs, so they count on that the user has hand-built those before > > running. That's ugly and barbaric, so we need to fix that :) > > > > Instead, make gyp/gn rules to copy this script to the appropriate output > folder. > > This means you can't just run the scripts, but you need to "build" them and > then > > run them. If we build, for instance, ninja -C out/Debug rtp_analyzer, all > > scripts in here are copied to out/Debug. To run, you'd do > > out/Debug/rtp_analyzer.py > > > > From this file you can then just do > > > > import pyproto.webrtc.call.rtc_event_log_pb2 as rtc_pb > > > > since those files are already in out/Debug. > > > > Create a py_event_log_analyzer.gyp file with something like this in it: > > > > { 'targets': [ > > { > > 'target_name': 'rtp_analyzer', > > 'type': 'none', > > 'actions': [ > > { > > 'action_name': 'copy_py_scripts', > > 'variables': { > > 'copy_output_dir%': '<(PRODUCT_DIR)', > > } > > 'inputs': [ > > '<(DEPTH)/pb_parse.py', > > '<(DEPTH)/misc.py', > > # ... etc ... > > ], > > 'outputs': [ > > '<(copy_output_dir)', > > ], > > 'action': ['python', > > '<(DEPTH)/copy_files.py', > > '--input_dir=<(DEPTH)', > > '--output_dir=<(copy_output_dir)'], > > 'message': 'Copying *py files code to <(copy_output_dir)', > > 'process_outputs_as_sources': 1, > > }, > > ], > > 'dependencies': [ > > # Depend on protos which makes sure they are built before this > > # target builds. Not sure if this is the right target to > depend > > # on (does it generate python protobufs?) > > 'webrtc/webrtc.gyp:rtc_event_log_proto', > > ], > > }], > > ], > > }, > > ], > > } > > > > We could also simply invoke cp or cp -r for the action, but this needs to work > > on Windows, so write a simple python script that copies files from input_dir > to > > output_dir. Note that PRODUCT_DIR will be out/Debug or whatever the user has > > chosen - don't ever hard-code that! DEPTH is the dir of the gyp file. > > I added the changes to /webrtc/webrtc.gyp and /webrtc/BUILD.gn. Both gyp and gn > seem to work as intended, but there are two python problems: > > python2 won't import pyproto.webrtc.call..., because the folders lack > __init__.py files. > > ../../third_party/protobuf/python still needs to be added to path. lgtm Great work!
On 2016/06/02 14:01:57, aleloi wrote:
> I have made an attempt to update the CL according to Patrik's comments. There
> are still problems: WebRTC must be built before running the tool, it does not
> currently work with Python 2.
Do you mean ninja -C out/Debug rtp_analyzer doesn't build everything it needs?
It should,
otherwise you have a missing dependency you should add to deps for the tool. All
build
targets should always be self-contained.
Also, the tool has to be run from the build
> directory (e.g. out/Debug) to which it is copied.
Yes, that's how it should be. This is how it works with c++ binaries, so we
should think the same
with python. If you want you can also copy a small shell script with the python
files:
#!/bin/sh
set -e
python rtp_analyzer.py
Then build + run becomes
ninja -C out/Debug rtp_analyzer
out/Debug/rtp_analyzer
If you want you can even check in the .py file that no one invokes it directly
from the source tree:
this_scripts_path = os.path.dirname(os.path.abspath(__file__))
if 'py_event_log_analyzer' in this_scripts_path:
raise RuntimeError('Do not run this script directly, build with ninja and run
from out/... dir')
>
>
https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy...
> File tools/py_event_log_analyzer/misc.py (right):
>
>
https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/misc.py:28: sequence_numbers[i] >=
> sequence_numbers[i+1]
> On 2016/05/31 13:47:42, phoglund wrote:
> > Try to make this fit on line 27. Docstrings should always be on the form
> >
> > """Title that explains the gist of the function.
> >
> > More stuff here in case the first line is longer than 80 chars.
> > """
> >
> > Apply throughout
>
> Rewrote docstrings everywhere.
>
>
https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/misc.py:78: class TestMisc(unittest.TestCase):
> On 2016/05/31 13:47:42, phoglund wrote:
> > Don't mix production code and unit tests; just move this to misc_unittest.py
> and
> > update the comment at the top.
>
> Test moved and broken up in pieces.
>
>
https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy...
> File tools/py_event_log_analyzer/pb_parse.py (right):
>
>
https://codereview.webrtc.org/1999113002/diff/220001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/pb_parse.py:21: ["out/Debug/pyproto/webrtc/call/",
> On 2016/05/31 13:47:43, phoglund wrote:
> > We should not do import hacks, and especially not if they build on brittle
> > assumptions as this one. If you build with GN, for instance, things will
tend
> to
> > end up in out/Default (but the user can really choose whatever name they
> want).
> >
> > This will not even do what you expect for gyp, in all cases. Suppose for
> > instance you build with debug first but then switch to release. Since debug
is
> > before release in the path, it will not pick up any protobuf changes if you
> > change + rebuild in release. Furthermore, these scripts do not properly
depend
> > on the protobufs, so they count on that the user has hand-built those before
> > running. That's ugly and barbaric, so we need to fix that :)
> >
> > Instead, make gyp/gn rules to copy this script to the appropriate output
> folder.
> > This means you can't just run the scripts, but you need to "build" them and
> then
> > run them. If we build, for instance, ninja -C out/Debug rtp_analyzer, all
> > scripts in here are copied to out/Debug. To run, you'd do
> > out/Debug/rtp_analyzer.py
> >
> > From this file you can then just do
> >
> > import pyproto.webrtc.call.rtc_event_log_pb2 as rtc_pb
> >
> > since those files are already in out/Debug.
> >
> > Create a py_event_log_analyzer.gyp file with something like this in it:
> >
> > { 'targets': [
> > {
> > 'target_name': 'rtp_analyzer',
> > 'type': 'none',
> > 'actions': [
> > {
> > 'action_name': 'copy_py_scripts',
> > 'variables': {
> > 'copy_output_dir%': '<(PRODUCT_DIR)',
> > }
> > 'inputs': [
> > '<(DEPTH)/pb_parse.py',
> > '<(DEPTH)/misc.py',
> > # ... etc ...
> > ],
> > 'outputs': [
> > '<(copy_output_dir)',
> > ],
> > 'action': ['python',
> > '<(DEPTH)/copy_files.py',
> > '--input_dir=<(DEPTH)',
> > '--output_dir=<(copy_output_dir)'],
> > 'message': 'Copying *py files code to <(copy_output_dir)',
> > 'process_outputs_as_sources': 1,
> > },
> > ],
> > 'dependencies': [
> > # Depend on protos which makes sure they are built before
this
> > # target builds. Not sure if this is the right target to
> depend
> > # on (does it generate python protobufs?)
> > 'webrtc/webrtc.gyp:rtc_event_log_proto',
> > ],
> > }],
> > ],
> > },
> > ],
> > }
> >
> > We could also simply invoke cp or cp -r for the action, but this needs to
work
> > on Windows, so write a simple python script that copies files from input_dir
> to
> > output_dir. Note that PRODUCT_DIR will be out/Debug or whatever the user has
> > chosen - don't ever hard-code that! DEPTH is the dir of the gyp file.
>
> I added the changes to /webrtc/webrtc.gyp and /webrtc/BUILD.gn. Both gyp and
gn
> seem to work as intended, but there are two python problems:
>
> python2 won't import pyproto.webrtc.call..., because the folders lack
> __init__.py files.
>
> ../../third_party/protobuf/python still needs to be added to path.
A few smaller comments on the python code. It's nice and clean code, so good work there! https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:5: python rtp_analyzer.py <rtc event log> Update the instructions when you settle on how to invoke the tool. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:15: Dependencies This should not be needed, but should be handled by the build system. The exception would the the instructions on how to replace the proto file in case anyone wants that. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/copy_files.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/copy_files.py:20: (options, _) = parser.parse_args() Nit: remove ( ). https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc_unittest.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc_unittest.py:23: class TestMisc(unittest.TestCase): We need to get these tests actually running on presubmit as well. I think if you edit PRESUBMIT.py in the WebRTC src/ dir and add this dir to test_directories for _RunPythonTests it should work. You can test by running git cl presubmit (perhaps intentionally break one of the tests to ensure the errors fails the presubmit). https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc_unittest.py:25: def testUnwrapMod3(self): Nice! https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:32: (first2header_bytes, self.sequence_number, Try this instead: header = struct.unpack_from("!HHII", rtp_header_str, 0) first2header_bytes, self.sequence_number, self.timestamp, self.ssrc = header https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:65: (bin_counts, bin_bounds) = numpy.histogram([point.size for point in Nit: remove ( ) https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:239: Nit: remove blank line
I have made a new version. There is one important problem: it doesn't work in python 2. What happens is that |pb_parse| can't import the generated protobuf library, because the folders pyproto.webrtc.call don't have __init__.py files and the module import system works differently in 2/3. I don't know how to solve that. Here are the changes since last version: The |misc| unit tests now run with presubmit. The structure had to change a little to make that possible. The way to run the script now is to first compile it, and then run ./out/my_build/rtp_analysis.sh <dump> The README is updated with the new running instructions. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:5: python rtp_analyzer.py <rtc event log> On 2016/06/03 08:11:07, phoglund wrote: > Update the instructions when you settle on how to invoke the tool. Done. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:15: Dependencies On 2016/06/03 08:11:07, phoglund wrote: > This should not be needed, but should be handled by the build system. The > exception would the the instructions on how to replace the proto file in case > anyone wants that. Done. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/copy_files.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/copy_files.py:20: (options, _) = parser.parse_args() On 2016/06/03 08:11:07, phoglund wrote: > Nit: remove ( ). The standings among reviewers are currently 1-1 for parentheses vs. no parentheses. The style guide says "use parentheses sparingly", but it also seems to encourage parentheses around tuples rather than discourage them. Personally, I think it adds readability. Ref to style guide: https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc_unittest.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc_unittest.py:23: class TestMisc(unittest.TestCase): On 2016/06/03 08:11:07, phoglund wrote: > We need to get these tests actually running on presubmit as well. I think if you > edit PRESUBMIT.py in the WebRTC src/ dir and add this dir to test_directories > for _RunPythonTests it should work. You can test by running git cl presubmit > (perhaps intentionally break one of the tests to ensure the errors fails the > presubmit). I did that. The test had to be renamed to end with "_test.py", to run the tests on execution and to be executable. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:32: (first2header_bytes, self.sequence_number, On 2016/06/03 08:11:07, phoglund wrote: > Try this instead: > > header = struct.unpack_from("!HHII", rtp_header_str, 0) > first2header_bytes, self.sequence_number, self.timestamp, self.ssrc = header Here there are also divided views between different reviewers. I like the latest suggestion more, because it is more readable, and because an extra local variable is no big thing. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:65: (bin_counts, bin_bounds) = numpy.histogram([point.size for point in On 2016/06/03 08:11:07, phoglund wrote: > Nit: remove ( ) I know the parens are redundant, but the style guide does not explicitly forbids them. Can I have some motivation for why removing them is better?
On 2016/06/09 12:00:44, aleloi wrote:
> I have made a new version.
>
> There is one important problem: it doesn't work in python 2. What happens is
> that |pb_parse| can't import the generated protobuf library, because the
folders
> pyproto.webrtc.call don't have __init__.py files and the module import system
> works differently in 2/3. I don't know how to solve that.
>
You can do a
PROTO_PATH = os.path.join("..", "..", "third_party", "protobuf", "python")
open(os.path.join(PROTO_PATH, '__init__.py'), 'a').close()
sys.path.append("../../third_party/protobuf/python/")
but that is pretty hacky. Why would pyproto produce unimportable python files?
That seems very strange to me.
> Here are the changes since last version:
>
> The |misc| unit tests now run with presubmit. The structure had to change a
> little to make that possible.
>
> The way to run the script now is to first compile it, and then run
> ./out/my_build/rtp_analysis.sh <dump>
>
> The README is updated with the new running instructions.
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> File tools/py_event_log_analyzer/README (right):
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/README:5: python rtp_analyzer.py <rtc event log>
> On 2016/06/03 08:11:07, phoglund wrote:
> > Update the instructions when you settle on how to invoke the tool.
>
> Done.
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/README:15: Dependencies
> On 2016/06/03 08:11:07, phoglund wrote:
> > This should not be needed, but should be handled by the build system. The
> > exception would the the instructions on how to replace the proto file in
case
> > anyone wants that.
>
> Done.
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> File tools/py_event_log_analyzer/copy_files.py (right):
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/copy_files.py:20: (options, _) =
parser.parse_args()
> On 2016/06/03 08:11:07, phoglund wrote:
> > Nit: remove ( ).
>
> The standings among reviewers are currently 1-1 for parentheses vs. no
> parentheses. The style guide says "use parentheses sparingly", but it also
seems
> to encourage parentheses around tuples rather than discourage them.
Personally,
> I think it adds readability.
>
> Ref to style guide:
>
https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s...
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> File tools/py_event_log_analyzer/misc_unittest.py (right):
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/misc_unittest.py:23: class
> TestMisc(unittest.TestCase):
> On 2016/06/03 08:11:07, phoglund wrote:
> > We need to get these tests actually running on presubmit as well. I think if
> you
> > edit PRESUBMIT.py in the WebRTC src/ dir and add this dir to
test_directories
> > for _RunPythonTests it should work. You can test by running git cl presubmit
> > (perhaps intentionally break one of the tests to ensure the errors fails the
> > presubmit).
>
> I did that. The test had to be renamed to end with "_test.py", to run the
tests
> on execution and to be executable.
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> File tools/py_event_log_analyzer/pb_parse.py (right):
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/pb_parse.py:32: (first2header_bytes,
> self.sequence_number,
> On 2016/06/03 08:11:07, phoglund wrote:
> > Try this instead:
> >
> > header = struct.unpack_from("!HHII", rtp_header_str, 0)
> > first2header_bytes, self.sequence_number, self.timestamp, self.ssrc = header
>
> Here there are also divided views between different reviewers. I like the
latest
> suggestion more, because it is more readable, and because an extra local
> variable is no big thing.
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> File tools/py_event_log_analyzer/rtp_analyzer.py (right):
>
>
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy...
> tools/py_event_log_analyzer/rtp_analyzer.py:65: (bin_counts, bin_bounds) =
> numpy.histogram([point.size for point in
> On 2016/06/03 08:11:07, phoglund wrote:
> > Nit: remove ( )
>
> I know the parens are redundant, but the style guide does not explicitly
forbids
> them. Can I have some motivation for why removing them is better?
https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/copy_files.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/copy_files.py:20: (options, _) = parser.parse_args() On 2016/06/09 12:00:44, aleloi wrote: > On 2016/06/03 08:11:07, phoglund wrote: > > Nit: remove ( ). > > The standings among reviewers are currently 1-1 for parentheses vs. no > parentheses. The style guide says "use parentheses sparingly", but it also seems > to encourage parentheses around tuples rather than discourage them. Personally, > I think it adds readability. > > Ref to style guide: > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... If it helps resolve the impasse, I can change my +1 to a +2. :-) Parentheses around tuples are good. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:32: (first2header_bytes, self.sequence_number, On 2016/06/09 12:00:44, aleloi wrote: > On 2016/06/03 08:11:07, phoglund wrote: > > Try this instead: > > > > header = struct.unpack_from("!HHII", rtp_header_str, 0) > > first2header_bytes, self.sequence_number, self.timestamp, self.ssrc = header > > Here there are also divided views between different reviewers. I like the latest > suggestion more, because it is more readable, and because an extra local > variable is no big thing. Pro tip: If two reviewers give you conflicting advice, pick the option you like best while the two of them are busy arguing. I still like the current version best, but this function is small enough that introducing an extra local variable doesn't hurt readability much. https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:5: ninja -C out/my_build webrtc:rtp_analyzer Inconsistent indentation. You use four spaces below. https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:21: The script has been tested to work in python versions 3.4.1 and 2.7.6, Is this true? Didn't you state elsewhere that only Python 3 works? https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... File tools/py_event_log_analyzer/copy_files.py (right): https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... tools/py_event_log_analyzer/copy_files.py:19: parser.add_option("--output_dir", help="where to copy *py and *sh files") *.py and *.sh would be better. But it looks like that will cause line breaks... https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc_test.py (right): https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc_test.py:14: python3 misc_test.py Question (because I'm too lazy to find it out myself): Are the unit tests actually run with both Python 2 and 3? If not, the untested version will inevitably rot after a while.
lgtm https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/pb_parse.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/pb_parse.py:32: (first2header_bytes, self.sequence_number, On 2016/06/09 12:27:20, kwiberg-webrtc wrote: > On 2016/06/09 12:00:44, aleloi wrote: > > On 2016/06/03 08:11:07, phoglund wrote: > > > Try this instead: > > > > > > header = struct.unpack_from("!HHII", rtp_header_str, 0) > > > first2header_bytes, self.sequence_number, self.timestamp, self.ssrc = header > > > > Here there are also divided views between different reviewers. I like the > latest > > suggestion more, because it is more readable, and because an extra local > > variable is no big thing. > > Pro tip: If two reviewers give you conflicting advice, pick the option you like > best while the two of them are busy arguing. > > I still like the current version best, but this function is small enough that > introducing an extra local variable doesn't hurt readability much. All right, I won't fight you on this. https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/260001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:65: (bin_counts, bin_bounds) = numpy.histogram([point.size for point in On 2016/06/09 12:00:44, aleloi wrote: > On 2016/06/03 08:11:07, phoglund wrote: > > Nit: remove ( ) > > I know the parens are redundant, but the style guide does not explicitly forbids > them. Can I have some motivation for why removing them is better? I think it looks nicer! Also, all the python code I've seen at Google so far tends to avoid them. https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc_test.py (right): https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc_test.py:14: python3 misc_test.py On 2016/06/09 12:27:20, kwiberg-webrtc wrote: > Question (because I'm too lazy to find it out myself): Are the unit tests > actually run with both Python 2 and 3? If not, the untested version will > inevitably rot after a while. It's python 2, and all scripts we have are in webrtc/chromium are written for python 2. If you want coverage on python 3 that's probably easy to add.
The current status is that everything works if py-3 is installed. First build
with ninja, then run with
./out/Debug/rpt_analyzer.sh <log file>.
There is a serious problem with py-2, which I have found the reason for. On the
computer I use to test this, there is an old system version of protobufs
installed in a py-2 package named 'google'. The generated protobuf file needs a
newer version of protobufs, which is included in //third_party/protobuf/python/.
I tried to include that instead of the system version by prepending to sys.path
or by defining PYTHONPATH. The problem is the file
'//third_party/protobuf/python/google/__init__.py. It contains this line
__import__('pkg_resources').declare_namespace(__name__)
which changes the default import behavior so that content is added to the other
'google' package instead of importing this.
My current solution is currently to just support py-3.
It's also possible to run the script with another python version with
PYTHON_EXECUTABLE=python ./out/Debug/rpt_analyzer.sh <log file>
https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy...
File tools/py_event_log_analyzer/README (right):
https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy...
tools/py_event_log_analyzer/README:5: ninja -C out/my_build webrtc:rtp_analyzer
On 2016/06/09 12:27:20, kwiberg-webrtc wrote:
> Inconsistent indentation. You use four spaces below.
thanks!
https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy...
tools/py_event_log_analyzer/README:21: The script has been tested to work in
python versions 3.4.1 and 2.7.6,
On 2016/06/09 12:27:20, kwiberg-webrtc wrote:
> Is this true? Didn't you state elsewhere that only Python 3 works?
Well, the truth is that it works in py-2 if you manage to run it in py-2. After
running it through the build system, only py-3 works. I just figured out why and
how to (sort of) fix it. I wrote about this in the general comments above.
https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy...
File tools/py_event_log_analyzer/copy_files.py (right):
https://codereview.webrtc.org/1999113002/diff/300001/tools/py_event_log_analy...
tools/py_event_log_analyzer/copy_files.py:19: parser.add_option("--output_dir",
help="where to copy *py and *sh files")
On 2016/06/09 12:27:20, kwiberg-webrtc wrote:
> *.py and *.sh would be better. But it looks like that will cause line
breaks...
Done.
The current status is that everything works if py-3 is installed. First build
with ninja, then run with
./out/Debug/rpt_analyzer.sh <log file>.
There is a serious problem with py-2, which I have found the reason for. On the
computer I use to test this, there is an old system version of protobufs
installed in a py-2 package named 'google'. The generated protobuf file needs a
newer version of protobufs, which is included in //third_party/protobuf/python/.
I tried to include that instead of the system version by prepending to sys.path
or by defining PYTHONPATH. The problem is the file
'//third_party/protobuf/python/google/__init__.py. It contains this line
__import__('pkg_resources').declare_namespace(__name__)
which changes the default import behavior so that content is added to the other
'google' package instead of importing this.
My current solution is currently to just support py-3.
It's also possible to run the script with another python version with
PYTHON_EXECUTABLE=python ./out/Debug/rpt_analyzer.sh <log file>
still lgtm, ship it
aleloi@webrtc.org changed reviewers: + kjellander@webrtc.org
Nice to see good tooling being added. I have a few concerns that needs to be addressed though. Please run at least one trybot for Chromium too, since I suspect this breaks the GN build. https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:21: The script has been tested to work in python versions 3.4.1 and 2.7.6, Please add a section about which third party Python modules are required to run this, or preferably provide copies of them in a third_party subdirectory so this script can run out of the box with a vanilla Python installation. That's the preferred way. https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/copy_files.py (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/copy_files.py:1: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. This script shouldn't be needed if it's only for build-time copying. See comment in webrtc/webrtc.gyp for more details. https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc_test.py (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc_test.py:21: import misc misc is not a part of the standard library. Can you mention the system requirements in the README or preferably provide a copy of it in a third_party subdirectory? https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:16: import matplotlib.pyplot as plt matplotlib is not a part of the standard library. Can you mention the system requirements in the README and put it in a separate import section? Preferably a copy would be provided in a third_party subdirectory (i.e. tools/py_event_log_analyzer/third_party/matplotlib) https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:17: import misc misc is not a part of the standard library. Can you mention the system requirements in the README and put it in a separate import section? Preferably a copy would be provided in a third_party subdirectory. https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:18: import numpy numpy is not a part of the standard library. Can you mention the system requirements in the README and put it in a separate import section? Preferably a copy would be provided in a third_party subdirectory. https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.sh (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.sh:16: exec $PYTHON_EXECUTABLE "rtp_analyzer.py" $@ I would prefer not having this script. You can alter the PYTHONPATH in the Python script itself instead in order to add third_party/protobuf/python. Many scripts to similar things, see https://chromium.googlesource.com/external/webrtc/+/master/tools/autoroller/r... for an example. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn#newcode356 webrtc/BUILD.gn:356: group("rtp_analyzer") { I don't think we should expose more targets to Chromium than necessary to avoid slowing down their build. Can you move these targets into an if (!build_with_chromium) { } block? Or move them into the if (rtc_include_tests) { } block further down. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn#newcode365 webrtc/BUILD.gn:365: "//tools/py_event_log_analyzer/copy_files.py", Since WebRTC's GN configuration needs to work both in Chromium and WebRTC, we cannot reference our sources with absolute paths. These would need to be like "tools/py_event_log_analyzer/copy_files.py" instead. You can run the patch on a Linux Chromium trybot to find out how/if this breaks. See https://webrtc.org/contributing/#tryjobs-on-chromium-trybots for details on that. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:193: 'action_name': 'copy_py_scripts', Can you use https://gyp.gsrc.io/docs/LanguageSpecification.md#Copies instead? See https://cs.chromium.org/search/?q=f:%5C.gyp+copies&sq=package:chromium&type=cs for examples. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:198: '<(DEPTH)/tools/py_event_log_analyzer/pb_parse.py', sort alphabetically. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:209: '<(copy_output_dir)/rtp_analyzer.sh', sort alphabetically.
https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/copy_files.py (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/copy_files.py:1: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2016/06/13 12:33:31, kjellander_webrtc wrote: > This script shouldn't be needed if it's only for build-time copying. See comment > in webrtc/webrtc.gyp for more details. You mean there's a general copy action in gyp? https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.sh (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.sh:16: exec $PYTHON_EXECUTABLE "rtp_analyzer.py" $@ On 2016/06/13 12:33:31, kjellander_webrtc wrote: > I would prefer not having this script. You can alter the PYTHONPATH in the > Python script itself instead in order to add third_party/protobuf/python. > Many scripts to similar things, see > https://chromium.googlesource.com/external/webrtc/+/master/tools/autoroller/r... > for an example. One problem is that the script doesn't run in python 2, so I think this can save people some confusion as this script invokes python3 for you. I suggested this script to make it a bit more convenient to run the script.
https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/copy_files.py (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/copy_files.py:1: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2016/06/13 13:40:48, phoglund wrote: > On 2016/06/13 12:33:31, kjellander_webrtc wrote: > > This script shouldn't be needed if it's only for build-time copying. See > comment > > in webrtc/webrtc.gyp for more details. > > You mean there's a general copy action in gyp? Yes (see my other comment). https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.sh (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.sh:16: exec $PYTHON_EXECUTABLE "rtp_analyzer.py" $@ On 2016/06/13 13:40:48, phoglund wrote: > On 2016/06/13 12:33:31, kjellander_webrtc wrote: > > I would prefer not having this script. You can alter the PYTHONPATH in the > > Python script itself instead in order to add third_party/protobuf/python. > > Many scripts to similar things, see > > > https://chromium.googlesource.com/external/webrtc/+/master/tools/autoroller/r... > > for an example. > > One problem is that the script doesn't run in python 2, so I think this can save > people some confusion as this script invokes python3 for you. I suggested this > script to make it a bit more convenient to run the script. Fair enough, let's keep it around. I don't see the point of spending time on supporting python3 though - migration is likely far away.
Description was changed from ========== New rtc dump analyzing tool in Python ========== to ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:bot1,bot2;tryserver.chromium.mac:bot3 ==========
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: bot1 on tryserver.chromium.linux (JOB_FAILED, no build URL) bot2 on tryserver.chromium.linux (JOB_FAILED, no build URL) bot3 on tryserver.chromium.mac (JOB_FAILED, no build URL)
Description was changed from ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:bot1,bot2;tryserver.chromium.mac:bot3 ========== to ========== New rtc dump analyzing tool in Python ==========
Description was changed from ========== New rtc dump analyzing tool in Python ========== to ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload ==========
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_upload on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I changed the build files, removed the copy script and updated the README. https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/README (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/README:21: The script has been tested to work in python versions 3.4.1 and 2.7.6, On 2016/06/13 12:33:31, kjellander_webrtc wrote: > Please add a section about which third party Python modules are required to run > this, or preferably provide copies of them in a third_party subdirectory so this > script can run out of the box with a vanilla Python installation. That's the > preferred way. Done. https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/misc_test.py (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/misc_test.py:21: import misc On 2016/06/13 12:33:31, kjellander_webrtc wrote: > misc is not a part of the standard library. Can you mention the system > requirements in the README or preferably provide a copy of it in a third_party > subdirectory? It's misc.py in this directory. Do you think I should change the package structure somehow? https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... File tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:16: import matplotlib.pyplot as plt On 2016/06/13 12:33:31, kjellander_webrtc wrote: > matplotlib is not a part of the standard library. Can you mention the system > requirements in the README and put it in a separate import section? > Preferably a copy would be provided in a third_party subdirectory (i.e. > tools/py_event_log_analyzer/third_party/matplotlib) It seems rather hard to ship a working matplotlib in a subdirectory. It has to be compiled (with its own compilation tools), has lots of other dependencies and needs a drawing backend. On the other hand, there are supported system installers for all platforms. I'll add it to the README. matplotlib and numpy are both part of the SciPy stack. If a user has one, she probably also has the other. http://www.scipy.org/install.html https://codereview.webrtc.org/1999113002/diff/320001/tools/py_event_log_analy... tools/py_event_log_analyzer/rtp_analyzer.py:18: import numpy On 2016/06/13 12:33:31, kjellander_webrtc wrote: > numpy is not a part of the standard library. Can you mention the system > requirements in the README and put it in a separate import section? > Preferably a copy would be provided in a third_party subdirectory. The same comment applies as for matplotlib. Numpy is also a huge package that has to be compiled. I don't know how hard it would be to ship it with webrtc. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn#newcode365 webrtc/BUILD.gn:365: "//tools/py_event_log_analyzer/copy_files.py", On 2016/06/13 12:33:31, kjellander_webrtc wrote: > Since WebRTC's GN configuration needs to work both in Chromium and WebRTC, we > cannot reference our sources with absolute paths. These would need to be like > "tools/py_event_log_analyzer/copy_files.py" instead. > > > You can run the patch on a Linux Chromium trybot to find out how/if this breaks. > See https://webrtc.org/contributing/#tryjobs-on-chromium-trybots for details on > that. But then the path is wrong. The tool is in src/tools, not in src/webrtc/tools. Should I the whole tool to //webrtc? I changed to the relative path "../" in next version. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp File webrtc/webrtc.gyp (right): https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:193: 'action_name': 'copy_py_scripts', On 2016/06/13 12:33:31, kjellander_webrtc wrote: > Can you use https://gyp.gsrc.io/docs/LanguageSpecification.md#Copies instead? > See > https://cs.chromium.org/search/?q=f:%5C.gyp+copies&sq=package:chromium&type=cs > for examples. Done, it works as before now. I removed the copy script in next version. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:198: '<(DEPTH)/tools/py_event_log_analyzer/pb_parse.py', On 2016/06/13 12:33:31, kjellander_webrtc wrote: > sort alphabetically. Done. https://codereview.webrtc.org/1999113002/diff/320001/webrtc/webrtc.gyp#newcod... webrtc/webrtc.gyp:209: '<(copy_output_dir)/rtp_analyzer.sh', On 2016/06/13 12:33:31, kjellander_webrtc wrote: > sort alphabetically. Done.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/501) ios64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/504) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3950)
https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn#newcode365 webrtc/BUILD.gn:365: "//tools/py_event_log_analyzer/copy_files.py", On 2016/06/13 12:33:31, kjellander_webrtc wrote: > Since WebRTC's GN configuration needs to work both in Chromium and WebRTC, we > cannot reference our sources with absolute paths. These would need to be like > "tools/py_event_log_analyzer/copy_files.py" instead. > > > You can run the patch on a Linux Chromium trybot to find out how/if this breaks. > See https://webrtc.org/contributing/#tryjobs-on-chromium-trybots for details on > that. I tried to follow the instructions to add trybots. The command line one says "User user:aleloi@webrtc.org cannot add builds to bucket master.tryserver.chromium.linux". The Rietveld UI says "An error occured". I made it work by adding a line to the CL description. Now it crashes for another reason (https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...).
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/360001
On 2016/06/14 08:29:35, aleloi wrote: > https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn > File webrtc/BUILD.gn (right): > > https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn#newcode365 > webrtc/BUILD.gn:365: "//tools/py_event_log_analyzer/copy_files.py", > On 2016/06/13 12:33:31, kjellander_webrtc wrote: > > Since WebRTC's GN configuration needs to work both in Chromium and WebRTC, we > > cannot reference our sources with absolute paths. These would need to be like > > "tools/py_event_log_analyzer/copy_files.py" instead. > > > > > > You can run the patch on a Linux Chromium trybot to find out how/if this > breaks. > > See https://webrtc.org/contributing/#tryjobs-on-chromium-trybots for details > on > > that. > > I tried to follow the instructions to add trybots. The command line one says > "User user:aleloi@webrtc.org cannot add builds to bucket > master.tryserver.chromium.linux". The Rietveld UI says "An error occured". I > made it work by adding a line to the CL description. Now it crashes for another > reason > (https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...). Right. You need Chromium trybot for your chromium.org account. I'll request that for you.
On 2016/06/14 08:39:09, kjellander_webrtc wrote: > On 2016/06/14 08:29:35, aleloi wrote: > > https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn > > File webrtc/BUILD.gn (right): > > > > > https://codereview.webrtc.org/1999113002/diff/320001/webrtc/BUILD.gn#newcode365 > > webrtc/BUILD.gn:365: "//tools/py_event_log_analyzer/copy_files.py", > > On 2016/06/13 12:33:31, kjellander_webrtc wrote: > > > Since WebRTC's GN configuration needs to work both in Chromium and WebRTC, > we > > > cannot reference our sources with absolute paths. These would need to be > like > > > "tools/py_event_log_analyzer/copy_files.py" instead. > > > > > > > > > You can run the patch on a Linux Chromium trybot to find out how/if this > > breaks. > > > See https://webrtc.org/contributing/#tryjobs-on-chromium-trybots for details > > on > > > that. > > > > I tried to follow the instructions to add trybots. The command line one says > > "User user:aleloi@webrtc.org cannot add builds to bucket > > master.tryserver.chromium.linux". The Rietveld UI says "An error occured". I > > made it work by adding a line to the CL description. Now it crashes for > another > > reason > > > (https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...). > > Right. You need Chromium trybot for your http://chromium.org account. I'll request that > for you. I meant "Chromium trybot access".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_upload on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/380001
I have moved the tool from //tools to //webrtc/tools in order to avoid problems with the Chromium build. The build part is updated accordingly. There was a problem: presubmit triggered on the GYP file. It said that this line references source files above the directory of the GYP file: '<(webrtc_root)/webrtc.gyp:rtc_event_log_proto' I looked at PRESUBMIT.py:_CheckNoSourcesAboveGyp for a while, but couldn't understand why the dependency matched the pattern for source files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6305)
Here is the current status: The CL cannot be easily tested on the Chromium bots for technical reasons explained below. It does however not break the Chrome GN build, which I tested by applying the patch locally. The Gyp build file references a Gyp file directly above it, which makes the presubmit check fail. This is because I want protobufs to be compiled before running. The protobuf target now sits in the build file above this. I can either remove the dependency, or move the build target up to the other gyp file. Why the Chromium bot doesn't work: The diff of this CL with master says that rtp_analyzer.sh is actually a copy of tools/python_charts/__init__.py with some small changes. The chromium bot tries to apply the diff to its own webrtc version, which doesn't contain //tools at all. Since I have no way of changing the way the diff is computed, I can't make the chromium bot to test this change.
Sorry about the bug in PRESUBMIT (I wrote that code; fixing in https://codereview.webrtc.org/2065253003). If you address my comments this should be safe to land without running Chromium trybots. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn File webrtc/tools/BUILD.gn (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:284: group("rtp_analyzer") { Please remove this group target. You can add a dependency to the copy-target below instead, like this: deps = [ "..:rtc_event_log_proto", ] I verified it works with building and looking at the output from gn desc out/Default/ //webrtc/tools:rtp_analyzer https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:287: "../:rtc_event_log_proto", nit: ..:rtc_event_log_proto is sufficient. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:291: copy("rtp_analyzer-copy") { After addressing the above comments, rename this to rtp_analyzer instead. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/py_event_lo... File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/py_event_lo... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:16: import matplotlib.pyplot as plt Imports should be grouped with the order being most generic to least generic: - standard library imports - third-party imports - application-specific imports See https://google.github.io/styleguide/pyguide.html#Imports_formatting https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp File webrtc/tools/tools.gyp (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp#n... webrtc/tools/tools.gyp:101: 'target_name': 'rtp_analyzer', Please move this inside the include_tests condition block below to prevent it from being processed by the Chromium build.
On 2016/06/15 14:40:31, aleloi wrote: > Here is the current status: > > The CL cannot be easily tested on the Chromium bots for technical reasons > explained below. It does however not break the Chrome GN build, which I tested > by applying the patch locally. > > The Gyp build file references a Gyp file directly above it, which makes the > presubmit check fail. This is because I want protobufs to be compiled before > running. The protobuf target now sits in the build file above this. I can either > remove the dependency, or move the build target up to the other gyp file. > > Why the Chromium bot doesn't work: The diff of this CL with master says that > rtp_analyzer.sh is actually a copy of tools/python_charts/__init__.py with some > small changes. The chromium bot tries to apply the diff to its own webrtc > version, which doesn't contain //tools at all. Since I have no way of changing > the way the diff is computed, I can't make the chromium bot to test this change. Interesting. I haven't had this problem before. I did a new CL using git cl upload --similarity=100, which made it become a pure add. Proof: https://codereview.webrtc.org/2066293002/ Try that if you want.
Message was sent while issue was closed.
Description was changed from ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload ========== to ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org Committed: https://crrev.com/74290b9d9c686603f1b8cafe9b4131a03485bf0c Cr-Commit-Position: refs/heads/master@{#13154} ==========
Message was sent while issue was closed.
Description was changed from ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload ========== to ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/74290b9d9c686603f1b8cafe9... ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/74290b9d9c686603f1b8cafe9b4131a03485bf0c Cr-Commit-Position: refs/heads/master@{#13154}
Message was sent while issue was closed.
Committed patchset #21 (id:400001) manually as 74290b9d9c686603f1b8cafe9b4131a03485bf0c (presubmit successful).
Message was sent while issue was closed.
Small changes in build and include order in accordance with kjellander's comments. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn File webrtc/tools/BUILD.gn (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:284: group("rtp_analyzer") { On 2016/06/15 14:46:53, kjellander_webrtc wrote: > Please remove this group target. You can add a dependency to the copy-target > below instead, like this: > deps = [ > "..:rtc_event_log_proto", > ] > > I verified it works with building and looking at the output from > gn desc out/Default/ //webrtc/tools:rtp_analyzer Done. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:287: "../:rtc_event_log_proto", On 2016/06/15 14:46:53, kjellander_webrtc wrote: > nit: ..:rtc_event_log_proto is sufficient. Done. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:291: copy("rtp_analyzer-copy") { On 2016/06/15 14:46:53, kjellander_webrtc wrote: > After addressing the above comments, rename this to rtp_analyzer instead. Done. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/py_event_lo... File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/py_event_lo... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:16: import matplotlib.pyplot as plt On 2016/06/15 14:46:54, kjellander_webrtc wrote: > Imports should be grouped with the order being most generic to least generic: > - standard library imports > - third-party imports > - application-specific imports > > See https://google.github.io/styleguide/pyguide.html#Imports_formatting Done. https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp File webrtc/tools/tools.gyp (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp#n... webrtc/tools/tools.gyp:101: 'target_name': 'rtp_analyzer', On 2016/06/15 14:46:54, kjellander_webrtc wrote: > Please move this inside the include_tests condition block below to prevent it > from being processed by the Chromium build. Done.
Message was sent while issue was closed.
A question: https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp File webrtc/tools/tools.gyp (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp#n... webrtc/tools/tools.gyp:119: '<(webrtc_root)/webrtc.gyp:rtc_event_log_proto', Didn't you want to keep this dependency? Since the PRESUBMIT check was incorrect..
Message was sent while issue was closed.
https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp File webrtc/tools/tools.gyp (right): https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp#n... webrtc/tools/tools.gyp:119: '<(webrtc_root)/webrtc.gyp:rtc_event_log_proto', On 2016/06/16 11:44:42, kjellander_webrtc wrote: > Didn't you want to keep this dependency? Since the PRESUBMIT check was > incorrect.. Yes, thank you! Fixed in next patch set.
Message was sent while issue was closed.
Patchset #23 (id:440001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/74290b9d9c686603f1b8cafe9... ========== to ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ==========
Description was changed from ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ========== to ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ==========
On 2016/06/16 12:18:57, aleloi wrote: > https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp > File webrtc/tools/tools.gyp (right): > > https://codereview.webrtc.org/1999113002/diff/380001/webrtc/tools/tools.gyp#n... > webrtc/tools/tools.gyp:119: '<(webrtc_root)/webrtc.gyp:rtc_event_log_proto', > On 2016/06/16 11:44:42, kjellander_webrtc wrote: > > Didn't you want to keep this dependency? Since the PRESUBMIT check was > > incorrect.. > > Yes, thank you! Fixed in next patch set. lgtm sorry for the delay! I was confused by my own CL that closed yours after committing manually (I manually patched in your CL to test my PRESUBMIT.py change and accidentally committed it since I forgot to remove the reference to the review).
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, kwiberg@webrtc.org, henrik.lundin@webrtc.org, peah@webrtc.org, phoglund@webrtc.org Link to the patchset: https://codereview.webrtc.org/1999113002/#ps460001 (title: "Added protobuf dependency back.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_upload on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/06/20 13:55:22, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_gn_upload on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) You need to remove the CQ_INCLUDE_TRYBOTS line due to the PRESUBMIT.py change.
Description was changed from ========== New rtc dump analyzing tool in Python CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_gn_upload R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ========== to ========== New rtc dump analyzing tool in Python R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ==========
The CQ bit was checked by aleloi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999113002/460001
Thank you for all the comments everyone! It has been very interesting and educational to play with the build system
Message was sent while issue was closed.
Description was changed from ========== New rtc dump analyzing tool in Python R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ========== to ========== New rtc dump analyzing tool in Python R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #23 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== New rtc dump analyzing tool in Python R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org ========== to ========== New rtc dump analyzing tool in Python R=henrik.lundin@webrtc.org, ivoc@webrtc.org, kwiberg@webrtc.org, peah@webrtc.org, phoglund@webrtc.org Committed: https://crrev.com/7ebbf9007706ba51be44e065a7c14f18a6d2b009 Cr-Commit-Position: refs/heads/master@{#13218} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/7ebbf9007706ba51be44e065a7c14f18a6d2b009 Cr-Commit-Position: refs/heads/master@{#13218} |
