Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695613005:
Add ana config to event log visualiser (Closed)
DescriptionAdd ana config to event log visualiser
BUG=webrtc:7160
Review-Url: https://codereview.webrtc.org/2695613005
Cr-Commit-Position: refs/heads/master@{#16776}
Committed: https://chromium.googlesource.com/external/webrtc/+/6e5b2195d72fd469b7e222fd06d7612a09727b6f
Patch Set 1 #
Total comments: 6
Patch Set 2 : Respond to comments #
Total comments: 6
Patch Set 3 : Respond to comments #
Total comments: 2
Patch Set 4 : Respond to comments #
Total comments: 4
Patch Set 5 : Respond to comments #Patch Set 6 : Rebased #Patch Set 7 : Fix for windows build. #Patch Set 8 : Replaced std::function with FunctionView #
Dependent Patchsets: Messages
Total messages: 40 (23 generated)
Description was changed from ========== Add ana config to event log visualiser BUG=webrtc:7160 ========== to ========== Add ana config to event log visualiser BUG=webrtc:7160 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org
Hi, here the cl for event log and ana
good job! I have a few comments https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1284: for (auto& ana_event : audio_network_adaptation_events_) { looks like there is a lot of code redundancy. Do we have a template https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:77: "Plot the audio encoder enable FEC."); can remove "enable" https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:80: "Plot the audio encoder enable DTX."); same here
https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:1284: for (auto& ana_event : audio_network_adaptation_events_) { Rigth, but it seams not to easy to reduce the redundancy. Looks like I would have to introduce a template function in the header to be able to handle the options with different types. Or do you have a better idea ? https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:77: "Plot the audio encoder enable FEC."); On 2017/02/14 17:04:22, minyue-webrtc wrote: > can remove "enable" Done. https://codereview.webrtc.org/2695613005/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/main.cc:80: "Plot the audio encoder enable DTX."); On 2017/02/14 17:04:22, minyue-webrtc wrote: > same here Done.
https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:1283: plot->series_list_.push_back(TimeSeries()); Sorry, I said template but may be a utility function: CreateAnaGraphs(Plot* plot, enum type) { for (auto& ana_event : audio_network_adaptation_events_) { if (ana_event.config.bitrate_bps) { float x = static_cast<float>(ana_event.timestamp - begin_time_) / 1000000; float y = 0; switch (type) { case BITRATE: y = static_cast<float>(*ana_event.config.bitrate_bps); case ... } plot->series_list_.back().points.emplace_back(x, y); } } plot->series_list_.back().style = LINE_DOT_GRAPH; plot->SetXAxis(0, call_duration_s_, "Time (s)", kLeftMargin, kRightMargin); switch (xxx) { case BITRATE: plot->series_list_.back().label = "Audio encoder target bitrate"; plot->SetSuggestedYAxis(0, 1, "Bitrate (bps)", kBottomMargin, kTopMargin); plot->SetTitle("Reported audio encoder target bitrate"); case xxx } https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/main.cc:75: DEFINE_bool(audio_encoder_enable_fec, sorry for not saying it, but "enable" here can be dropped too. https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/main.cc:78: DEFINE_bool(audio_encoder_enable_dtx, and here
https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:1283: plot->series_list_.push_back(TimeSeries()); tried a solution with lamdas. https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/main.cc (right): https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/main.cc:75: DEFINE_bool(audio_encoder_enable_fec, On 2017/02/15 09:00:22, minyue-webrtc wrote: > sorry for not saying it, but "enable" here can be dropped too. Done. https://codereview.webrtc.org/2695613005/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/main.cc:78: DEFINE_bool(audio_encoder_enable_dtx, On 2017/02/15 09:00:22, minyue-webrtc wrote: > and here Done.
minyue@webrtc.org changed reviewers: + terelius@webrtc.org
https://codereview.webrtc.org/2695613005/diff/40001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/40001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:115: std::function<void(Plot* plot, can we make this function return y and put x= and embrace() in here
+terelius
https://codereview.webrtc.org/2695613005/diff/40001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/40001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:115: std::function<void(Plot* plot, Yes
https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:111: void FillAudioEncoderTimeSeries( I don't mind making this a member method to avoid passing audio_network_adaptation_events_ and begin_time_.
https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:111: void FillAudioEncoderTimeSeries( On 2017/02/16 09:54:56, minyue-webrtc wrote: > I don't mind making this a member method to avoid passing > audio_network_adaptation_events_ and begin_time_. The function "Pointwise" is quite similar to this except that it takes a function object rather than a lambda. (Name comes from processing the points one-by-one.) Maybe it would be useful to merge them?
https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:111: void FillAudioEncoderTimeSeries( The rtc::Optional in "FillAudioEncoderTimeSeries" is different than in "Pointwise". But we still should be able to merge them. The questions is just which approach we will use. lambda or function object. I would prefer lambda's since otherwise I would have to create a object for each CreateAudioEncoder function. But I'm open for your ideas.
https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:111: void FillAudioEncoderTimeSeries( On the other hand it would be quite a work, to change the current impl. to lambdas, as well.
On 2017/02/16 10:29:42, michaelt wrote: > https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... > File webrtc/tools/event_log_visualizer/analyzer.cc (right): > > https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... > webrtc/tools/event_log_visualizer/analyzer.cc:111: void > FillAudioEncoderTimeSeries( > On the other hand it would be quite a work, to change the current impl. to > lambdas, as well. Hi Bjorn, Would you suggest here?
On 2017/02/20 15:41:21, minyue-webrtc wrote: > On 2017/02/16 10:29:42, michaelt wrote: > > > https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... > > File webrtc/tools/event_log_visualizer/analyzer.cc (right): > > > > > https://codereview.webrtc.org/2695613005/diff/60001/webrtc/tools/event_log_vi... > > webrtc/tools/event_log_visualizer/analyzer.cc:111: void > > FillAudioEncoderTimeSeries( > > On the other hand it would be quite a work, to change the current impl. to > > lambdas, as well. > > Hi Bjorn, > > Would you suggest here? I think lambdas are cleaner, but function objects are slightly more powerful for template programming. Since it seems hard to tell whether lambdas or function objects are better without a demo implementation, I'm ok with landing this CL as-is. In that case, we should have a separate discussion regarding how we want to analyze data. lgtm assuming we unify the analysis functions in a follow up CL.
lgtm
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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17603) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23050) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21826) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/23520)
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_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
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: This issue passed the CQ dry run.
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2695613005/#ps140001 (title: "Replaced std::function with FunctionView")
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": 140001, "attempt_start_ts": 1487777435228200,
"parent_rev": "0335e6c4bfa9a3254a392c003344c71238ee832f", "commit_rev":
"6e5b2195d72fd469b7e222fd06d7612a09727b6f"}
Message was sent while issue was closed.
Description was changed from ========== Add ana config to event log visualiser BUG=webrtc:7160 ========== to ========== Add ana config to event log visualiser BUG=webrtc:7160 Review-Url: https://codereview.webrtc.org/2695613005 Cr-Commit-Position: refs/heads/master@{#16776} Committed: https://chromium.googlesource.com/external/webrtc/+/6e5b2195d72fd469b7e222fd0... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/6e5b2195d72fd469b7e222fd0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
