Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696133003:
Add parser to visualise the ana dump (Closed)
|
Created:
3 years, 10 months ago by michaelt Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, elad.alon_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd parser to visualise the ana dump
BUG=webrtc:7160
Review-Url: https://codereview.webrtc.org/2696133003
Cr-Commit-Position: refs/heads/master@{#17622}
Committed: https://chromium.googlesource.com/external/webrtc/+/a1ef71f6225c4ddd8c9f0af9772fb60c4ab22de9
Patch Set 1 #
Total comments: 32
Patch Set 2 : Respond to comments. #
Total comments: 6
Patch Set 3 : Respond to comments. #Patch Set 4 : fix pylint #Messages
Total messages: 29 (16 generated)
Description was changed from ========== Add parser to visualise the ana dump BUG=webrtc:7160 ========== to ========== Add parser to visualise the ana dump BUG=webrtc:7160 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org
Hi, I was finally able to upload the file :)
minyue@webrtc.org changed reviewers: + aleloi@webrtc.org
On 2017/02/15 08:09:45, michaelt wrote: > Hi, > I was finally able to upload the file :) Add Alex for python style review.
Please see my comments. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py (right): https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:1: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. Please add #!/usr/bin/python2 http://dev.chromium.org/chromium-os/python-style-guidelines#TOC-The-shebang-line https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:10: import debug_dump_pb2 This is a local application import. It should go last. Also, where will this script find the pb? GN+ninja can generate this and and puts somewhere in out/<target>/pyproto/. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:12: import matplotlib.pyplot as plt This is probably a third party imports. Please reorder according to https://www.python.org/dev/peps/pep-0008/#imports https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:36: (options, args) = parser.parse_args() What happens with this line when the file is imported? Should it perhaps be inside of if __name__ == "__main__"? https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:39: def GetSize(file_to_parse): The format is 4 bytes of message size, then message, right? The name made me think the format is whole file size, then file content. I suggest renaming this to GetNextMessageSize instead. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:47: Metrics = {} I think local vars should be lowercase. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:55: Decisions = {} Same here. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:65: Decisions = InitDecisions() And here. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:66: first_time_stamp = -1 Since Event.timestamp is a numeric value, i think None is more appropriate for 'no time stamp seen yet' than -1. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:75: break I don't think the error should be ignored. Perhaps print it? https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:75: break I suggest putting line 68-75 inside its own function for readability. E.g. GetNextMessageFromFile. I think that would simplify control flow, error handling and have one less name visible (size) in the rest of this function. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:80: first_time_stamp = event.timestamp The two time_stamp lines could go first. Less code duplication and nested constructs that way. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:98: (Metrics, Decisions) = ParseAnaDump(options.dump_file_to_parse) Lowercase https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:108: plot_index = 0 Can plot_index be updated via a loop construct like 'for (plot_index, key) in metric_keys' instead? I think manual increment at the end of a loop should be avoided if possible. It distracts from the more important program logic. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:111: plot = plots Make plots a singleton list to avoid this special case.
Thanks for reviewing this. I'm not really a very experienced python programmer, so I'm thankful for the help. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py (right): https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:1: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/03/30 11:52:55, aleloi wrote: > Please add #!/usr/bin/python2 > > http://dev.chromium.org/chromium-os/python-style-guidelines#TOC-The-shebang-line Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:10: import debug_dump_pb2 I'm struggling a bit with how to import the generated files from protobuf in a proper way. Should I add them to side packets ? Should we use virtualenv for this ? Just importing this form "out/<build_name>/." does not work since there are no __init__.py files. Any ideas ? https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:12: import matplotlib.pyplot as plt On 2017/03/30 11:52:55, aleloi wrote: > This is probably a third party imports. Please reorder according to > https://www.python.org/dev/peps/pep-0008/#imports Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:36: (options, args) = parser.parse_args() On 2017/03/30 11:52:55, aleloi wrote: > What happens with this line when the file is imported? Should it perhaps be > inside of if __name__ == "__main__"? Acknowledged. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:39: def GetSize(file_to_parse): On 2017/03/30 11:52:55, aleloi wrote: > The format is 4 bytes of message size, then message, right? The name made me > think the format is whole file size, then file content. I suggest renaming this > to GetNextMessageSize instead. Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:47: Metrics = {} On 2017/03/30 11:52:55, aleloi wrote: > I think local vars should be lowercase. Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:55: Decisions = {} On 2017/03/30 11:52:55, aleloi wrote: > Same here. Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:65: Decisions = InitDecisions() On 2017/03/30 11:52:55, aleloi wrote: > And here. Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:66: first_time_stamp = -1 On 2017/03/30 11:52:55, aleloi wrote: > Since Event.timestamp is a numeric value, i think None is more appropriate for > 'no time stamp seen yet' than -1. Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:75: break On 2017/03/30 11:52:55, aleloi wrote: > I don't think the error should be ignored. Perhaps print it? Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:75: break On 2017/03/30 11:52:55, aleloi wrote: > I suggest putting line 68-75 inside its own function for readability. E.g. > GetNextMessageFromFile. I think that would simplify control flow, error handling > and have one less name visible (size) in the rest of this function. Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:80: first_time_stamp = event.timestamp On 2017/03/30 11:52:55, aleloi wrote: > The two time_stamp lines could go first. Less code duplication and nested > constructs that way. Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:98: (Metrics, Decisions) = ParseAnaDump(options.dump_file_to_parse) On 2017/03/30 11:52:55, aleloi wrote: > Lowercase Done. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:108: plot_index = 0 Used plots.pop() to get the next plot in the list https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:111: plot = plots I was not quite sure what you meant with a singleton list. I made plots a list allays even in the case we have just one.
lgtm, but please add instructions for how to run the script and where to find the pb file. https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py (right): https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:10: import debug_dump_pb2 On 2017/04/03 09:55:05, michaelt wrote: > I'm struggling a bit with how to import the generated files from protobuf in a > proper way. > Should I add them to side packets ? > Should we use virtualenv for this ? > Just importing this form "out/<build_name>/." does not work since there are no > __init__.py files. > > Any ideas ? > I don't know. I'm fine if there are some instructions for how this should be run. In other places a python script is wrapped in a bash script that sets the path in which python looks for includes. Both scripts are copied to out/<build_name> during compilation through a GN rule. https://chromium.googlesource.com/external/webrtc/+/master/webrtc/tools/py_ev... https://codereview.webrtc.org/2696133003/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:111: plot = plots On 2017/04/03 09:55:05, michaelt wrote: > I was not quite sure what you meant with a singleton list. > I made plots a list allays even in the case we have just one. > That's what I wanted! Sorry for being unclear. https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py (right): https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:2: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. It's 2017 now. https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:11: from optparse import OptionParser Add blank line to separate system imports from third party imports https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:13: import debug_dump_pb2 Add blank line to third party imports from project imports
I added some instructions on how the script should be run. https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py (right): https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:2: # Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/04/04 09:30:16, aleloi wrote: > It's 2017 now. Done. https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:11: from optparse import OptionParser On 2017/04/04 09:30:16, aleloi wrote: > Add blank line to separate system imports from third party imports Done. https://codereview.webrtc.org/2696133003/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/parse_ana_dump.py:13: import debug_dump_pb2 On 2017/04/04 09:30:16, aleloi wrote: > Add blank line to third party imports from project imports Done.
The CQ bit was checked by minyue@webrtc.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2696133003/#ps40001 (title: "Respond to comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15886)
The CQ bit was checked by michaelt@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15956)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2696133003/#ps60001 (title: "fix pylint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491836221865340,
"parent_rev": "8d609f6b6d46a03a6fb58b1b58d53d481e524062", "commit_rev":
"a1ef71f6225c4ddd8c9f0af9772fb60c4ab22de9"}
Message was sent while issue was closed.
Description was changed from ========== Add parser to visualise the ana dump BUG=webrtc:7160 ========== to ========== Add parser to visualise the ana dump BUG=webrtc:7160 Review-Url: https://codereview.webrtc.org/2696133003 Cr-Commit-Position: refs/heads/master@{#17622} Committed: https://chromium.googlesource.com/external/webrtc/+/a1ef71f6225c4ddd8c9f0af97... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/a1ef71f6225c4ddd8c9f0af97... |
