Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220383004:
Visualize delay changes based on both abs-send-time and capture time. (Closed)
DescriptionVisualize delay changes based on both abs-send-time and capture time.
Adds reusable template function objects to extract interesting
statistics from data sets. A few more of these will be added later
to reduce the code size.
Committed: https://crrev.com/ccbbf8da3820ed3fe7505b968b44db22028a4627
Cr-Commit-Position: refs/heads/master@{#13713}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Move LoggedRtcpPacket and BwePacketLossEvent out of EventLogAnalyzer. #Patch Set 3 : Fix compile error #Patch Set 4 : Comments from Danil. #Patch Set 5 : Format #
Total comments: 2
Patch Set 6 : Nit #
Messages
Total messages: 27 (12 generated)
terelius@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
Please take a look.
https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: class OneWayDelayChangeAbsSendTime { Can't we make these classes inherit from an Extractor base class instead of using templates? To me it looks like we always pass in new and old LoggedRtpPackets and get double back, it's just how it's implemented that differs, not the types. Or perhaps it isn't possible because of Pairwise()? https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:84: struct LoggedRtcpPacket { Maybe move this out too for consistency?
https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: class OneWayDelayChangeAbsSendTime { On 2016/08/09 11:43:17, stefan-webrtc (holmer) wrote: > Can't we make these classes inherit from an Extractor base class instead of > using templates? To me it looks like we always pass in new and old > LoggedRtpPackets and get double back, it's just how it's implemented that > differs, not the types. > > Or perhaps it isn't possible because of Pairwise()? The idea is to add more functions that will be used to generate all (or most) of the graphs. Typically it will be LoggedRtpPacket, but sometimes we have to operate on other types. E.g it should be possible to do something like this Pointwise<LoggedRtpPacket, PacketSizeBytes>(packets); Pairwise<uint64_t, PlayoutDifference>(audio_playouts); Pointwise<BwePacketLossEvent, FractionLoss>(bwe_loss_updates); MovingAverage<LoggedRtpPacket, PacketSizeBytes>(packets, window_size, step, scale_bytes_to_kilobits); We could add a base class, but then we would have to create a new one for each type of data we are going to analyze. https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:84: struct LoggedRtcpPacket { On 2016/08/09 11:43:17, stefan-webrtc (holmer) wrote: > Maybe move this out too for consistency? Done. Also moved BwePacketLossEvent.
lgtm https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: class OneWayDelayChangeAbsSendTime { On 2016/08/09 12:04:27, terelius wrote: > On 2016/08/09 11:43:17, stefan-webrtc (holmer) wrote: > > Can't we make these classes inherit from an Extractor base class instead of > > using templates? To me it looks like we always pass in new and old > > LoggedRtpPackets and get double back, it's just how it's implemented that > > differs, not the types. > > > > Or perhaps it isn't possible because of Pairwise()? > > The idea is to add more functions that will be used to generate all (or most) of > the graphs. Typically it will be LoggedRtpPacket, but sometimes we have to > operate on other types. > > E.g it should be possible to do something like this > Pointwise<LoggedRtpPacket, PacketSizeBytes>(packets); > Pairwise<uint64_t, PlayoutDifference>(audio_playouts); > Pointwise<BwePacketLossEvent, FractionLoss>(bwe_loss_updates); > MovingAverage<LoggedRtpPacket, PacketSizeBytes>(packets, window_size, step, > scale_bytes_to_kilobits); > > We could add a base class, but then we would have to create a new one for each > type of data we are going to analyze. Makes sense.
The CQ bit was checked by terelius@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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/9405)
https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: class OneWayDelayChangeAbsSendTime { It is not easy to see from from the long name of the functor what does it do. May be split the name: OneWayDelay::AbsSendTime::Delta Delta/Diff (function name instead of operator() ) shows what Extractors calculate in general OneWayDelay (namespace or parent class) shows what this particular extractors calculates AbsSendTime (trait class name) hints how it does that. https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:141: template <typename Extractor> Accumulated designed just for LoggedRtpPacket, but Pairwise take packet class as template argument. may be in the extractors above you can add what type they work on: class OneWayDelayChangeCaptureTime { public: using Packet = LoggedRtpPacket; double Delta(const Packet& old_packet, const Packet& new_packet) { ... Then Accumulated can be generalized: template <typename Extractor> class Accumulated { using Packet = typename Extractor::Packet; double operator()(const Packet& old_packet, ... and Pairwise will not need an extra template argument: template <typename Extractor> void Pairwise(const std::vector<typename Extractor::Packet>& data, ...) https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:144: Accumulated() : extract(), sum(0) {} default constructor for Extractor probably better to omit. default constructor for Accumulated can be removed too if you add initializing to declaration: private: Extractor extract; double sum = 0; }; https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:159: webrtc::plotting::TimeSeries* result) { since you are in ::webrtc::plotting namesapce, you probably do not need full name, but if you want write full name, add root: ::webrtc::plotting::TimeSeries
https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: class OneWayDelayChangeAbsSendTime { On 2016/08/09 12:39:58, danilchap wrote: > It is not easy to see from from the long name of the functor what does it do. > May be split the name: > OneWayDelay::AbsSendTime::Delta > > Delta/Diff (function name instead of operator() ) shows what Extractors > calculate in general > OneWayDelay (namespace or parent class) shows what this particular extractors > calculates > AbsSendTime (trait class name) hints how it does that. The Extractors compute a difference in this CL but they won't do so "in general". The "typical" Extractor will just take a single packet or event and select whatever member we are interested in. Not sure adding a pair of colons will aid readability, but please take a look. https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:144: Accumulated() : extract(), sum(0) {} On 2016/08/09 12:39:58, danilchap wrote: > default constructor for Extractor probably better to omit. > default constructor for Accumulated can be removed too if you add initializing > to declaration: > private: > Extractor extract; > double sum = 0; > }; Done. https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:159: webrtc::plotting::TimeSeries* result) { On 2016/08/09 12:39:58, danilchap wrote: > since you are in ::webrtc::plotting namesapce, you probably do not need full > name, but if you want write full name, add root: > ::webrtc::plotting::TimeSeries Done. Was moving some code around.
lgtm % nit https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: class OneWayDelayChangeAbsSendTime { On 2016/08/09 14:42:54, terelius wrote: > On 2016/08/09 12:39:58, danilchap wrote: > > It is not easy to see from from the long name of the functor what does it do. > > May be split the name: > > OneWayDelay::AbsSendTime::Delta > > > > Delta/Diff (function name instead of operator() ) shows what Extractors > > calculate in general > > OneWayDelay (namespace or parent class) shows what this particular extractors > > calculates > > AbsSendTime (trait class name) hints how it does that. > > The Extractors compute a difference in this CL but they won't do so "in > general". The "typical" Extractor will just take a single packet or event and > select whatever member we are interested in. > > Not sure adding a pair of colons will aid readability, but please take a look. It does for me, like a space between words or an extra empty line. Thank you. https://codereview.webrtc.org/2220383004/diff/80001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/80001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:153: ResultType operator()(const LoggedRtpPacket& old_packet, DataType instead of LoggedRtpPacket
https://codereview.webrtc.org/2220383004/diff/80001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2220383004/diff/80001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:153: ResultType operator()(const LoggedRtpPacket& old_packet, On 2016/08/09 15:09:44, danilchap wrote: > DataType instead of LoggedRtpPacket Done.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2220383004/#ps100001 (title: "Nit")
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: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/17038)
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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
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.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Visualize delay changes based on both abs-send-time and capture time. Adds reusable template function objects to extract interesting statistics from data sets. A few more of these will be added later to reduce the code size. ========== to ========== Visualize delay changes based on both abs-send-time and capture time. Adds reusable template function objects to extract interesting statistics from data sets. A few more of these will be added later to reduce the code size. Committed: https://crrev.com/ccbbf8da3820ed3fe7505b968b44db22028a4627 Cr-Commit-Position: refs/heads/master@{#13713} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ccbbf8da3820ed3fe7505b968b44db22028a4627 Cr-Commit-Position: refs/heads/master@{#13713} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
