Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876423002:
Add NetEq delay plotting to event_log_visualizer (Closed)
|
Created:
3 years, 7 months ago by hlundin-webrtc Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd NetEq delay plotting to event_log_visualizer
This CL adds the capability to analyze and plot how NetEq behaves in
response to a network trace.
BUG=webrtc:7467
Review-Url: https://codereview.webrtc.org/2876423002
Cr-Commit-Position: refs/heads/master@{#18590}
Committed: https://chromium.googlesource.com/external/webrtc/+/3c938fc5ea57d4a90dbc550eb235bbff53a079f7
Patch Set 1 #
Total comments: 38
Patch Set 2 : Updated after first review #
Total comments: 3
Patch Set 3 : Change to upper_bound search #Patch Set 4 : Move unnamed namespace to top of file #Patch Set 5 : Rebase #
Total comments: 4
Patch Set 6 : After terilius's review #
Total comments: 7
Patch Set 7 : Store start-end pairs #Patch Set 8 : Rebase #Patch Set 9 : Take care of missing LOG_END events #
Depends on Patchset: Messages
Total messages: 26 (6 generated)
henrik.lundin@webrtc.org changed reviewers: + ivoc@webrtc.org
Ivo, please, take a look at this CL. You may find that it needs better structure or better documentation. Please, call that out, and I will update. Thanks!
Looks like a useful tool! See some comments below. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:34: bool muted, Since this is not used you can comment out the name. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:47: RTC_CHECK(!it->second.decode_get_audio_count) I think this code block would be easier to read with an intermediary variable. Something like auto& it_timing = it->second; https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:61: void NetEqDelayAnalyzer::CreateGraphs( This function is pretty long, so you can consider breaking out a part of it into a helper function. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:72: nominal_get_audio_time_ms.begin(), nominal_get_audio_time_ms.end() - 1, Will this crash if nominal_get_audio_time_ms is empty? https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:87: for (auto& d : data_) { Please add a comment to explain what this loop does. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:90: arrival_time_ms.push_back(d.second.arrival_time_ms); Also here I would suggest to introduce an intermediate variable for d.second, to make the code more readable. Something like d_timing. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:109: double y; The variable names are a bit cryptic in this function, some comments to explain (or more descriptive names) would be helpful. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:137: for (size_t i = 0; i < send_time_s->size(); ++i) { Please add a comment to explain what this for loop does. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:140: corrected_arrival_delay_ms->emplace_back(y); This is exactly the same as push_back for a vector<float>, right? https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.h (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.h:47: uint16_t rtp_sn; I think it would be nicer to spell out the abbreviation here, i.e. rtp_sequence_number. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1408: NetEqStreamInput(const std::vector<LoggedRtpPacket>* packet_stream, It looks like this class is not actually modifying the contents of packet_stream or output_events_us, so wouldn't a const reference be nicer here? https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1473: const std::vector<uint64_t>& output_events_us_; Since all that is used is the end iterator (for both packet_stream_ and output_events_us_), I think it makes sense to just store that instead of a reference to the vector. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1481: void EventLogAnalyzer::CreateAudioJitterBufferGraph( This function is a bit long, so I think it would be nice to extract one or several parts into smaller, anonymous functions. Here are some ideas: - Filling the vector of audio output events - Setting up the NetEqTest - Plotting the results https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1505: if (!ssrc || *ssrc == 0) { Isn't it better to check that this_ssrc == stream_id.GetSsrc() ? https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:235: "/usr/local/google/home/hlundin/webrtc_src/junk/krefsme48.pcm", 48000, This looks like it could use an update :-)
https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:233: if (FLAGS_plot_all || FLAGS_plot_audio_jitter_buffer) { I think it should be FLAG instead of FLAGS
Thanks for reviewing! Please, take a look at the new patch set. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:34: bool muted, On 2017/05/16 13:25:50, ivoc wrote: > Since this is not used you can comment out the name. Done. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:47: RTC_CHECK(!it->second.decode_get_audio_count) On 2017/05/16 13:25:50, ivoc wrote: > I think this code block would be easier to read with an intermediary variable. > Something like > auto& it_timing = it->second; Done. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:61: void NetEqDelayAnalyzer::CreateGraphs( On 2017/05/16 13:25:50, ivoc wrote: > This function is pretty long, so you can consider breaking out a part of it into > a helper function. Done. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:72: nominal_get_audio_time_ms.begin(), nominal_get_audio_time_ms.end() - 1, On 2017/05/16 13:25:50, ivoc wrote: > Will this crash if nominal_get_audio_time_ms is empty? Presumably, but we already set the first element on line 70, so it will always be at least 1 element long. But we should probably check that get_audio_time_ms is not empty. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:87: for (auto& d : data_) { On 2017/05/16 13:25:50, ivoc wrote: > Please add a comment to explain what this loop does. Done. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:90: arrival_time_ms.push_back(d.second.arrival_time_ms); On 2017/05/16 13:25:50, ivoc wrote: > Also here I would suggest to introduce an intermediate variable for d.second, to > make the code more readable. Something like d_timing. Done. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:109: double y; On 2017/05/16 13:25:50, ivoc wrote: > The variable names are a bit cryptic in this function, some comments to explain > (or more descriptive names) would be helpful. I broke this part out to a generalized interpolation function. In the new context, I think that x and y makes sense. WDYT? https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:137: for (size_t i = 0; i < send_time_s->size(); ++i) { On 2017/05/16 13:25:50, ivoc wrote: > Please add a comment to explain what this for loop does. Done. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:140: corrected_arrival_delay_ms->emplace_back(y); On 2017/05/16 13:25:50, ivoc wrote: > This is exactly the same as push_back for a vector<float>, right? You are right. This is over-kill. I changed them all, since I think emplace_back doesn't improve the case with std::vector<rtc::Optional<float>> either. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.h (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.h:47: uint16_t rtp_sn; On 2017/05/16 13:25:50, ivoc wrote: > I think it would be nicer to spell out the abbreviation here, i.e. > rtp_sequence_number. Turns out I didn't even use it... https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1408: NetEqStreamInput(const std::vector<LoggedRtpPacket>* packet_stream, On 2017/05/16 13:25:50, ivoc wrote: > It looks like this class is not actually modifying the contents of packet_stream > or output_events_us, so wouldn't a const reference be nicer here? This is a recommended pattern. It avoids the potential problem of dangling references in the class. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1473: const std::vector<uint64_t>& output_events_us_; On 2017/05/16 13:25:50, ivoc wrote: > Since all that is used is the end iterator (for both packet_stream_ and > output_events_us_), I think it makes sense to just store that instead of a > reference to the vector. Done. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1481: void EventLogAnalyzer::CreateAudioJitterBufferGraph( On 2017/05/16 13:25:50, ivoc wrote: > This function is a bit long, so I think it would be nice to extract one or > several parts into smaller, anonymous functions. Here are some ideas: > - Filling the vector of audio output events > - Setting up the NetEqTest > - Plotting the results I broke out two parts but left the actual graph population in this function. WDYT? https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1505: if (!ssrc || *ssrc == 0) { On 2017/05/16 13:25:50, ivoc wrote: > Isn't it better to check that this_ssrc == stream_id.GetSsrc() ? Won't we miss out on the events with SSRC == 0 then? I presume they are the ones that happen before any incoming packets. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:233: if (FLAGS_plot_all || FLAGS_plot_audio_jitter_buffer) { On 2017/05/17 19:31:39, owb1 wrote: > I think it should be FLAG instead of FLAGS Not in gflags: https://gflags.github.io/gflags/#using https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:235: "/usr/local/google/home/hlundin/webrtc_src/junk/krefsme48.pcm", 48000, On 2017/05/16 13:25:50, ivoc wrote: > This looks like it could use an update :-) You think? :) Done!
Lgtm, but see some comments below. https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:72: nominal_get_audio_time_ms.begin(), nominal_get_audio_time_ms.end() - 1, On 2017/05/30 14:56:06, hlundin-webrtc wrote: > On 2017/05/16 13:25:50, ivoc wrote: > > Will this crash if nominal_get_audio_time_ms is empty? > > Presumably, but we already set the first element on line 70, so it will always > be at least 1 element long. But we should probably check that get_audio_time_ms > is not empty. Acknowledged. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1408: NetEqStreamInput(const std::vector<LoggedRtpPacket>* packet_stream, On 2017/05/30 14:56:07, hlundin-webrtc wrote: > On 2017/05/16 13:25:50, ivoc wrote: > > It looks like this class is not actually modifying the contents of > packet_stream > > or output_events_us, so wouldn't a const reference be nicer here? > > This is a recommended pattern. It avoids the potential problem of dangling > references in the class. Acknowledged. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1481: void EventLogAnalyzer::CreateAudioJitterBufferGraph( On 2017/05/30 14:56:07, hlundin-webrtc wrote: > On 2017/05/16 13:25:50, ivoc wrote: > > This function is a bit long, so I think it would be nice to extract one or > > several parts into smaller, anonymous functions. Here are some ideas: > > - Filling the vector of audio output events > > - Setting up the NetEqTest > > - Plotting the results > > I broke out two parts but left the actual graph population in this function. > WDYT? Looks good! Nice work. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1505: if (!ssrc || *ssrc == 0) { On 2017/05/30 14:56:07, hlundin-webrtc wrote: > On 2017/05/16 13:25:50, ivoc wrote: > > Isn't it better to check that this_ssrc == stream_id.GetSsrc() ? > > Won't we miss out on the events with SSRC == 0 then? I presume they are the ones > that happen before any incoming packets. Good point. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:233: if (FLAGS_plot_all || FLAGS_plot_audio_jitter_buffer) { On 2017/05/30 14:56:07, hlundin-webrtc wrote: > On 2017/05/17 19:31:39, owb1 wrote: > > I think it should be FLAG instead of FLAGS > > Not in gflags: https://gflags.github.io/gflags/#using It turns out someone updated this tool to use base/flags instead of gflags while you're working on it, so you'll find out when you rebase that Oliver was actually right ;) https://codereview.webrtc.org/2876423002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:60: namespace { Shouldn't this block be at the top of the file? https://codereview.webrtc.org/2876423002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:70: auto it = std::find_if(x_vec.begin(), x_vec.end(), If we know that x_vec is sorted (is it?), this can be done more efficiently with std::upper_bound, which uses a binary search.
Change to upper_bound search
Thanks! Updated according to your suggestions. https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2876423002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:233: if (FLAGS_plot_all || FLAGS_plot_audio_jitter_buffer) { On 2017/05/30 16:29:45, ivoc wrote: > On 2017/05/30 14:56:07, hlundin-webrtc wrote: > > On 2017/05/17 19:31:39, owb1 wrote: > > > I think it should be FLAG instead of FLAGS > > > > Not in gflags: https://gflags.github.io/gflags/#using > > It turns out someone updated this tool to use base/flags instead of gflags while > you're working on it, so you'll find out when you rebase that Oliver was > actually right ;) Oh, I see. I'll get to the rebase when everything else is done and dusted. https://codereview.webrtc.org/2876423002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/tools/neteq_delay_analyzer.cc:70: auto it = std::find_if(x_vec.begin(), x_vec.end(), On 2017/05/30 16:29:45, ivoc wrote: > If we know that x_vec is sorted (is it?), this can be done more efficiently with > std::upper_bound, which uses a binary search. Awesome! Great improvement.
henrik.lundin@webrtc.org changed reviewers: + kjellander@webrtc.org, terelius@webrtc.org
Adding more reviewers for OWNERS review and approval: terelius: webrtc/tools/event_log_visualizer/ kjellander webrtc/tools/BUILD.gn and DEPS Thanks!
lgtm for webrtc/test
https://codereview.webrtc.org/2876423002/diff/80001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/80001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:1483: void CreateOutputEventVector(const ParsedRtcEventLog& parsed_log, Would it make sense to split the audio playouts by SSRC in the analyser constructor? I mean similar to the way RTP and RTCP are handled? https://codereview.webrtc.org/2876423002/diff/80001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:1625: series.second.label = "Relative packet arrival delay"; If there are multiple series, you might want to label them differently, e.g. by appending the stream_id.
Thanks for the comments. PTAL again. https://codereview.webrtc.org/2876423002/diff/80001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/80001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:1483: void CreateOutputEventVector(const ParsedRtcEventLog& parsed_log, On 2017/06/01 12:59:08, terelius wrote: > Would it make sense to split the audio playouts by SSRC in the analyser > constructor? I mean similar to the way RTP and RTCP are handled? Done. https://codereview.webrtc.org/2876423002/diff/80001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:1625: series.second.label = "Relative packet arrival delay"; On 2017/06/01 12:59:08, terelius wrote: > If there are multiple series, you might want to label them differently, e.g. by > appending the stream_id. This code will only produce a single stream to plot. Added checks and comments to document this. https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... webrtc/tools/event_log_visualizer/main.cc:112: webrtc::test::SetExecutablePath(argv[0]); This is needed to make webrtc::test::ResourcePath work.
lgtm with a couple of minor nits/questions https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... webrtc/tools/event_log_visualizer/analyzer.cc:1626: // to be plotted, they should likely be given distingt labels below. nit: "distinct" https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... webrtc/tools/event_log_visualizer/analyzer.h:173: std::vector<uint64_t> log_end_events_; std::vector<std::pair<uint64_t, uint64_t>> log_segments; to store both begin and end times together?
lgtm
Thanks. Björn, please, take a look at my questions, and advise me. https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... webrtc/tools/event_log_visualizer/analyzer.cc:1626: // to be plotted, they should likely be given distingt labels below. On 2017/06/09 14:48:36, terelius wrote: > nit: "distinct" Done. https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... webrtc/tools/event_log_visualizer/analyzer.h:173: std::vector<uint64_t> log_end_events_; On 2017/06/09 14:48:36, terelius wrote: > std::vector<std::pair<uint64_t, uint64_t>> log_segments; > to store both begin and end times together? I did something, but I'm not sure what assumptions we can make about the order of these events. For instance, do we know that we always have an alternating sequence like (start, stop, start, stop, ...), or could we have multiple start or stop events in sequence? Also, will we always have a start event at the beginning, and a stop event at the end?
Rebase
https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... webrtc/tools/event_log_visualizer/analyzer.h:173: std::vector<uint64_t> log_end_events_; On 2017/06/12 07:15:08, hlundin-webrtc wrote: > On 2017/06/09 14:48:36, terelius wrote: > > std::vector<std::pair<uint64_t, uint64_t>> log_segments; > > to store both begin and end times together? > > I did something, but I'm not sure what assumptions we can make about the order > of these events. For instance, do we know that we always have an alternating > sequence like (start, stop, start, stop, ...), or could we have multiple start > or stop events in sequence? Also, will we always have a start event at the > beginning, and a stop event at the end? Yes, we write a LOG_START event whenever we start logging to file. https://cs.chromium.org/chromium/src/third_party/webrtc/logging/rtc_event_log... We attempt to write a LOG_END event whenever we stop logging to file, but there is a small risk that the event can't be written if the file is full already. https://cs.chromium.org/chromium/src/third_party/webrtc/logging/rtc_event_log... If a second LOG_START or EOF is encountered when LOG_END was expected, I'd suggest artificially adding a LOG_END with the largest timestamp found so far. I don't think we need to handle LOG_END without matching LOG_START. WDYT?
https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2876423002/diff/100001/webrtc/tools/event_log_v... webrtc/tools/event_log_visualizer/analyzer.h:173: std::vector<uint64_t> log_end_events_; On 2017/06/13 15:16:26, terelius wrote: > On 2017/06/12 07:15:08, hlundin-webrtc wrote: > > On 2017/06/09 14:48:36, terelius wrote: > > > std::vector<std::pair<uint64_t, uint64_t>> log_segments; > > > to store both begin and end times together? > > > > I did something, but I'm not sure what assumptions we can make about the order > > of these events. For instance, do we know that we always have an alternating > > sequence like (start, stop, start, stop, ...), or could we have multiple start > > or stop events in sequence? Also, will we always have a start event at the > > beginning, and a stop event at the end? > > Yes, we write a LOG_START event whenever we start logging to file. > https://cs.chromium.org/chromium/src/third_party/webrtc/logging/rtc_event_log... > > We attempt to write a LOG_END event whenever we stop logging to file, but there > is a small risk that the event can't be written if the file is full already. > https://cs.chromium.org/chromium/src/third_party/webrtc/logging/rtc_event_log... > > If a second LOG_START or EOF is encountered when LOG_END was expected, I'd > suggest artificially adding a LOG_END with the largest timestamp found so far. I > don't think we need to handle LOG_END without matching LOG_START. WDYT? Done. That seems like a good solution.
re-lgtm
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2876423002/#ps150001 (title: "Take care of missing LOG_END events")
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": 150001, "attempt_start_ts": 1497443168423400,
"parent_rev": "3c81a1afd87a1764a6515fca365b8723828a089f", "commit_rev":
"3c938fc5ea57d4a90dbc550eb235bbff53a079f7"}
Message was sent while issue was closed.
Description was changed from ========== Add NetEq delay plotting to event_log_visualizer This CL adds the capability to analyze and plot how NetEq behaves in response to a network trace. BUG=webrtc:7467 ========== to ========== Add NetEq delay plotting to event_log_visualizer This CL adds the capability to analyze and plot how NetEq behaves in response to a network trace. BUG=webrtc:7467 Review-Url: https://codereview.webrtc.org/2876423002 Cr-Commit-Position: refs/heads/master@{#18590} Committed: https://chromium.googlesource.com/external/webrtc/+/3c938fc5ea57d4a90dbc550eb... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://chromium.googlesource.com/external/webrtc/+/3c938fc5ea57d4a90dbc550eb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
