Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2178973005:
Improvements to bandwidth plot. See Monorail issue for comparison. (Closed)
DescriptionImprovements to bandwidth plot. See Monorail issue for comparison.
NOTRY=true
BUG=webrtc:6070
Committed: https://crrev.com/b0dc30d28258244f50a8ad7699e2d36dc82013e0
Cr-Commit-Position: refs/heads/master@{#13606}
Patch Set 1 : Fixed bandwidth plot #Patch Set 2 : Improved the delay plot. #
Total comments: 6
Patch Set 3 : Formatting and comments in response to review. #
Total comments: 2
Patch Set 4 : Improved comment. #Messages
Total messages: 20 (8 generated)
Description was changed from ========== Improvements to bandwidth plot. See Monorail issue for comparison. BUG=6070 ========== to ========== Improvements to bandwidth plot. See Monorail issue for comparison. BUG=webrtc:6070 ==========
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org, phoglund@webrtc.org
aleloi@webrtc.org changed reviewers: + stefan@webrtc.org
ivoc@ discovered an issue in the bandwidth and delay plot. This CL should fix that. Could you please take a look?
lgtm https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:248: RTPStatistics.PLOT_RESOLUTION_MS, Nit: indent https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:267: Regularizes points by rounding the timestamps to the nearest point Isn't this "quantizing"?
https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:267: Regularizes points by rounding the timestamps to the nearest point On 2016/07/28 06:28:55, phoglund wrote: > Isn't this "quantizing"? Perhaps you can use numpy's digitize instead? http://docs.scipy.org/doc/numpy/reference/generated/numpy.digitize.html
Response to comments & new patch set. https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:248: RTPStatistics.PLOT_RESOLUTION_MS, On 2016/07/28 06:28:55, phoglund wrote: > Nit: indent Done. https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:267: Regularizes points by rounding the timestamps to the nearest point On 2016/07/28 06:28:55, phoglund wrote: > Isn't this "quantizing"? Wikipedia confirms that "quantizing" is the correct term :) https://codereview.webrtc.org/2178973005/diff/20001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:267: Regularizes points by rounding the timestamps to the nearest point On 2016/07/28 07:13:36, stefan-webrtc (holmer) wrote: > On 2016/07/28 06:28:55, phoglund wrote: > > Isn't this "quantizing"? > > Perhaps you can use numpy's digitize instead? > http://docs.scipy.org/doc/numpy/reference/generated/numpy.digitize.html I'd rather not use that, I think. Based on the description, it would do a search in the whole time series for every point and increase complexity.
lgtm, see a small nit below. https://codereview.webrtc.org/2178973005/diff/40001/webrtc/tools/py_event_log... File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/2178973005/diff/40001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:267: Quantizes points by rounding the timestamps to the nearest point It's not really rounding when you're using int(), and it doesn't round to the nearest point. If you want to do rounding you should use round(), or alternatively you can insert the word 'downwards' in the comment, i.e. "... rounding the timestamp downwards to ..."
https://codereview.webrtc.org/2178973005/diff/40001/webrtc/tools/py_event_log... File webrtc/tools/py_event_log_analyzer/rtp_analyzer.py (right): https://codereview.webrtc.org/2178973005/diff/40001/webrtc/tools/py_event_log... webrtc/tools/py_event_log_analyzer/rtp_analyzer.py:267: Quantizes points by rounding the timestamps to the nearest point On 2016/07/28 09:30:51, ivoc wrote: > It's not really rounding when you're using int(), and it doesn't round to the > nearest point. If you want to do rounding you should use round(), or > alternatively you can insert the word 'downwards' in the comment, i.e. "... > rounding the timestamp downwards to ..." Done.
stefan-holmer@: Friendly ping :D
lgtm
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org, ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2178973005/#ps60001 (title: "Improved comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Wohoo! Thanks for the comments :)
Description was changed from ========== Improvements to bandwidth plot. See Monorail issue for comparison. BUG=webrtc:6070 ========== to ========== Improvements to bandwidth plot. See Monorail issue for comparison. NOTRY=true BUG=webrtc:6070 ==========
Message was sent while issue was closed.
Description was changed from ========== Improvements to bandwidth plot. See Monorail issue for comparison. NOTRY=true BUG=webrtc:6070 ========== to ========== Improvements to bandwidth plot. See Monorail issue for comparison. NOTRY=true BUG=webrtc:6070 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Improvements to bandwidth plot. See Monorail issue for comparison. NOTRY=true BUG=webrtc:6070 ========== to ========== Improvements to bandwidth plot. See Monorail issue for comparison. NOTRY=true BUG=webrtc:6070 Committed: https://crrev.com/b0dc30d28258244f50a8ad7699e2d36dc82013e0 Cr-Commit-Position: refs/heads/master@{#13606} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b0dc30d28258244f50a8ad7699e2d36dc82013e0 Cr-Commit-Position: refs/heads/master@{#13606} |
