Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2234883002:
Adds function for computing moving average to visualization tool. (Closed)
|
Created:
4 years, 4 months ago by terelius Modified:
4 years, 4 months ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds function for computing moving average to visualization tool.
Uses generic functions to plot packet sizes, sequence number delta and bitrate per SSRC. Also detects and prints warnings if delay differences seem unrealistic.
NOTRY=True
Committed: https://crrev.com/6addf49913915a392826960e3b3a23e496992cbc
Cr-Commit-Position: refs/heads/master@{#13872}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Comments from Stefan #
Messages
Total messages: 20 (9 generated)
terelius@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:169: if (delay_change < -10000 || 10000 < delay_change) { I'd say you could argue that 10 seconds delay change is possible although uncommon... But maybe worth warning about it anyway. https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:228: void MovingAverage(const std::vector<typename Extractor::DataType>& data, Comment on what this does, e.g. something like, computes a moving average across the "data" and stores each average in the resulting time series. https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:240: // Calculate a moving average of the bitrate and store in a TimeSeries. Not necessarily a bitrate now right? https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:245: window_index_end++; ++window...; https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:250: window_index_begin++; ++window...;
https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:169: if (delay_change < -10000 || 10000 < delay_change) { On 2016/08/11 15:40:15, stefan-webrtc (holmer) wrote: > I'd say you could argue that 10 seconds delay change is possible although > uncommon... But maybe worth warning about it anyway. Though real-time communications won't work if the network stops transmitting packets for more than 10 seconds. I definitely think a warning is warranted, but I reformulated it to avoid the implication that it has to be a bug in our code. https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:228: void MovingAverage(const std::vector<typename Extractor::DataType>& data, On 2016/08/11 15:40:15, stefan-webrtc (holmer) wrote: > Comment on what this does, e.g. something like, computes a moving average across > the "data" and stores each average in the resulting time series. Done. https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:240: // Calculate a moving average of the bitrate and store in a TimeSeries. On 2016/08/11 15:40:15, stefan-webrtc (holmer) wrote: > Not necessarily a bitrate now right? Done. https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:245: window_index_end++; On 2016/08/11 15:40:15, stefan-webrtc (holmer) wrote: > ++window...; Done. https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:250: window_index_begin++; On 2016/08/11 15:40:15, stefan-webrtc (holmer) wrote: > ++window...; Done.
lgtm assuming we don't see it as a problem that the logs might be spammy (after all, it's just a tool and not production logs). https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:169: if (delay_change < -10000 || 10000 < delay_change) { On 2016/08/19 15:12:19, terelius wrote: > On 2016/08/11 15:40:15, stefan-webrtc (holmer) wrote: > > I'd say you could argue that 10 seconds delay change is possible although > > uncommon... But maybe worth warning about it anyway. > > Though real-time communications won't work if the network stops transmitting > packets for more than 10 seconds. I definitely think a warning is warranted, but > I reformulated it to avoid the implication that it has to be a bug in our code. Ok, no risk of it becoming too spammy?
https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2234883002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:169: if (delay_change < -10000 || 10000 < delay_change) { On 2016/08/23 10:01:57, OOO stefan-webrtc (holmer) wrote: > On 2016/08/19 15:12:19, terelius wrote: > > On 2016/08/11 15:40:15, stefan-webrtc (holmer) wrote: > > > I'd say you could argue that 10 seconds delay change is possible although > > > uncommon... But maybe worth warning about it anyway. > > > > Though real-time communications won't work if the network stops transmitting > > packets for more than 10 seconds. I definitely think a warning is warranted, > but > > I reformulated it to avoid the implication that it has to be a bug in our > code. > > Ok, no risk of it becoming too spammy? No, I think the risk is minimal. I haven't seen it happen except when the clock appears to jump after the offset is changed (which is what we want to be warned about anyway). If it turns out to happen in practice, we'll adjust the thresholds.
The CQ bit was checked by terelius@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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by terelius@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 terelius@webrtc.org
Description was changed from ========== Adds function for computing moving average to visualization tool. Uses generic functions to plot packet sizes, sequence number delta and bitrate per SSRC. Also detects and prints warnings if delay differences seem unrealistic. ========== to ========== Adds function for computing moving average to visualization tool. Uses generic functions to plot packet sizes, sequence number delta and bitrate per SSRC. Also detects and prints warnings if delay differences seem unrealistic. NOTRY=True ==========
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Adds function for computing moving average to visualization tool. Uses generic functions to plot packet sizes, sequence number delta and bitrate per SSRC. Also detects and prints warnings if delay differences seem unrealistic. NOTRY=True ========== to ========== Adds function for computing moving average to visualization tool. Uses generic functions to plot packet sizes, sequence number delta and bitrate per SSRC. Also detects and prints warnings if delay differences seem unrealistic. NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Adds function for computing moving average to visualization tool. Uses generic functions to plot packet sizes, sequence number delta and bitrate per SSRC. Also detects and prints warnings if delay differences seem unrealistic. NOTRY=True ========== to ========== Adds function for computing moving average to visualization tool. Uses generic functions to plot packet sizes, sequence number delta and bitrate per SSRC. Also detects and prints warnings if delay differences seem unrealistic. NOTRY=True Committed: https://crrev.com/6addf49913915a392826960e3b3a23e496992cbc Cr-Commit-Position: refs/heads/master@{#13872} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6addf49913915a392826960e3b3a23e496992cbc Cr-Commit-Position: refs/heads/master@{#13872} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
