Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1995523002:
Visualization tool for WebrtcEventLogs (Closed)
DescriptionInitial version of the local visualization tool for WebrtcEventLogs.
Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs.
Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags.
Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d
Committed: https://crrev.com/54ce68017008907b4ee2b21d23cf616028cec2da
Cr-Original-Commit-Position: refs/heads/master@{#13443}
Cr-Commit-Position: refs/heads/master@{#13463}
Patch Set 1 #
Total comments: 78
Patch Set 2 : Review comments #
Total comments: 27
Patch Set 3 : Comments from holmer@ #Patch Set 4 : Style guide fix #
Total comments: 8
Patch Set 5 : More review comments #
Total comments: 1
Patch Set 6 : Drop partially implemented feature #Patch Set 7 : Avoid python warning when a plot is empty #Patch Set 8 : Fix trivially true DCHECK. #
Total comments: 2
Patch Set 9 : Add rtc_event_log_proto to the public_deps of rtc_event_log_parser. #
Total comments: 1
Patch Set 10 : Use include_tests==1 to avoid compiling if gflags isn't available. #
Messages
Total messages: 51 (22 generated)
Description was changed from ========== Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. BUG= ========== to ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. ==========
terelius@webrtc.org changed reviewers: + aleloi@webrtc.org, stefan@webrtc.org
The code could be a bit cleaner and the tool should have some additional features, but I think it is better to submit this first to let interested people take look at it and try it out.
aleloi, it would be good if you also took a look at this. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:43: std::string SSRCToString(uint32_t ssrc) { Ssrc https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:49: // Checks whether the an SSRC is contained in the list of desired SSRCs. -the https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:50: // Note that an empty SSRC list counts matches every SSRC. -counts? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:51: bool MatchingSSRC(uint32_t ssrc, const std::vector<uint32_t>& desired_ssrc) { MatchingSsrc https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:72: int64_t WrappingDifference(uint32_t later, uint32_t earlier, int64_t modulus) { Can you use philipel's mod_ops.h for this? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:87: // typedef StreamID uint64_t; Remove? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:88: uint64_t GetStreamID(uint32_t ssrc, GetStreamId https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:94: // stream_id = stream_id Remove https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:99: uint32_t GetSsrcFromStreamID(uint64_t stream) { GetSsrcFromStreamId https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:130: // No useful events in the log End with . https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:176: // Set labels and put in graph End with . https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:234: plot->xaxis_max = (end_time - begin_time) / 1000000 * 1.02; Maybe name the constant 1.02 "kXMargin" or something like that? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:237: plot->yaxis_max = max_y * 1.1; kYMargin? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:326: // Remove? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:425: // TODO(terelius): Refactor Is the plan to base this method on the previous one in some way? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:439: double accumulatedDelay; // In milliseconds No camel case https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:44: const ParsedRtcEventLog& parsed_log; End all members with _ https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:24: TimeSeriesPoint(float x_, float y_) : x(x_), y(y_) {} I don't think the _ are needed? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:48: public: Private? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:64: std::vector<std::unique_ptr<Plot> > plots; Would probably make sense to make this private, right?
Just a few comments (I have only started reading this) https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:336: const std::string& extension = config.rtp.extensions[j].name; RtpExtension.name was removed in 6f8d686d. It seems to have been renamed into `RtpExtension.uri' now. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/plot_python.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_python.h:22: virtual void draw(); The style guide says to use 'override'. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Explicitly annotate overrides of virtual functions or virtual destructors with an override... https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_python.h:29: virtual void draw(); here as well
Another small comment: it does not compile with the current (846b2d9) version of WebRTC, because it fails in the link phase. If I add '<(webrtc_root)/system_wrappers/system_wrappers.gyp:metrics_default' as a dependency in tools.gyp, the missing definitions are included again and it compiles. I have not figured out why (the system_wrappers.gyp has not changed, so probably some one removed the definitions of metrics::Histogram* from something that was included earlier).
Looks nice! A feature request and a short design comment: Add "if __name__ == \"__main__\"" to the python output code. This way you can import the generated code as a module and do quick analysis with e.g. numpy. Also add dynamically generated docstrings that explains what the data is in which array. Some parts of the code use C-style output while other use STL streams. Consider changing everything to STL streams. Then it is much simpler to write a test that runs Plot::draw() on some test data and compares it with expected output. A simple test would be useful as documentation! I have still two thirds left of analyzer.cc, so there will probably me more comments this week. /Alex https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:61: // time in second and then multiply by 1000000 to convert to microseconds. second -> seconds? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:62: static const double kTimestampToMicroSec = constexpr? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:72: int64_t WrappingDifference(uint32_t later, uint32_t earlier, int64_t modulus) { Is there a guarantee that this function is only given numbers with difference less than modulus? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:88: uint64_t GetStreamID(uint32_t ssrc, I'd like a comment that explains what StreamID is. E.g. 48-bit number consisting of ... defined in ... used in ... https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:124: if (timestamp < first_timestamp) You use if-statements here and the ternary ?: operator below. I'd say that std::min/max is more readable. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:143: std::string message; It looks scary when message is defined outside of the loop, conditionally updated and copied to a function. Nothing bad happens here, but in an artificial scenario where extra_point_info is updated while this function is running you could get points labeled with wrong messages. Since it's not a performance issue, I want |message| moved into the loop. The same for all the other variables except |header|. Also, consider renaming message to maybe 'point_info'. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:165: max_y = (y > max_y) ? y : max_y; Make it consistent with the other min/max calculations. Unless this is the standard in webrtc, I vote for std::max. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:188: plot->yaxis_max = max_y * 1.1; Make 1.1, 1.02, -1 constants. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:25: // duration of it's lifetime. The ParsedRtcEventLog must not be destroyed or it's -> its https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:27: EventLogAnalyzer(const ParsedRtcEventLog& log, bool extra_info); Add a comment describing extra_info https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:49: uint64_t step; Consider adding a comment that describes what |step| and |window_duration| is used for. The other members have more self-explanatory names and are used in a straight-forward way. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/generate_timeseries.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/generate_timeseries.cc:35: "Plot the difference in sequence number between consecutive packets."); number -> numbers, although I am not certain. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/generate_timeseries.cc:147: collection->plots.back().get()); Do you think it's possible to reduce the repetition somehow? E.g. there are 10 calls to collection->plots.back().get() and collection->append_plot(); What about auto get_last_plot = [&]() { return collection->plots.back().get(); }; ? Or make append_plot() return a pointer to the appended plot https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:39: points.swap(other.points); Use std::swap in all cases for consistency. It is specialized to strings and vectors and does the same as the member swap. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:64: std::vector<std::unique_ptr<Plot> > plots; On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Would probably make sense to make this private, right? It is accessed in the main function and in analysis. Maybe make analysis friend and make append_plot return the appended plot? https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_python.cc:90: plots.push_back(std::unique_ptr<Plot>(static_cast<Plot*>(new PythonPlot()))); The explicit cast is not needed here. PythonPlot* is a kind of Plot*, so it can be safely given to the unique_ptr constructor. (If I understand the C++ type system correctly. Let's run the bots or look it up to be sure).
Here is another thing: add this to the GN build system. Currently, it's only build with Gyp.
https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:43: std::string SSRCToString(uint32_t ssrc) { On 2016/05/31 18:53:39, stefan-webrtc (holmer) wrote: > Ssrc Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:49: // Checks whether the an SSRC is contained in the list of desired SSRCs. On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > -the Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:50: // Note that an empty SSRC list counts matches every SSRC. On 2016/05/31 18:53:39, stefan-webrtc (holmer) wrote: > -counts? Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:51: bool MatchingSSRC(uint32_t ssrc, const std::vector<uint32_t>& desired_ssrc) { On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > MatchingSsrc Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:61: // time in second and then multiply by 1000000 to convert to microseconds. On 2016/06/08 11:44:20, aleloi wrote: > second -> seconds? Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:62: static const double kTimestampToMicroSec = On 2016/06/08 11:44:20, aleloi wrote: > constexpr? Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:72: int64_t WrappingDifference(uint32_t later, uint32_t earlier, int64_t modulus) { On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Can you use philipel's mod_ops.h for this? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... No, at least not off the shelf. I want the signed difference with the smallest possible magnitude, but philipel's functions all return unsigned results. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:72: int64_t WrappingDifference(uint32_t later, uint32_t earlier, int64_t modulus) { On 2016/06/08 11:44:20, aleloi wrote: > Is there a guarantee that this function is only given numbers with difference > less than modulus? Yes, the numbers should always be less than modulus. I've documented this with a DCHECK. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:87: // typedef StreamID uint64_t; On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Remove? Done. I was considering whether a typedef would make the code more readable. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:88: uint64_t GetStreamID(uint32_t ssrc, On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > GetStreamId Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:88: uint64_t GetStreamID(uint32_t ssrc, On 2016/06/08 11:44:20, aleloi wrote: > I'd like a comment that explains what StreamID is. E.g. 48-bit number consisting > of ... defined in ... used in ... I've added a comment, but this isn't a defined concept. Normally there is no risk of mixing up incoming and outgoing packets, so incoming and outgoing streams could conceivably use the same SSRC. Since we store both incoming and outgoing packets as the same type, we need to distinguish packets and streams by some other parameters in addition to the SSRC. The simplest way is to append the other identifiers to the SSRC. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:94: // stream_id = stream_id On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Remove Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:99: uint32_t GetSsrcFromStreamID(uint64_t stream) { On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > GetSsrcFromStreamId Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:124: if (timestamp < first_timestamp) On 2016/06/08 11:44:20, aleloi wrote: > You use if-statements here and the ternary ?: operator below. I'd say that > std::min/max is more readable. Good point. Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:130: // No useful events in the log On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > End with . Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:143: std::string message; On 2016/06/08 11:44:20, aleloi wrote: > It looks scary when message is defined outside of the loop, conditionally > updated and copied to a function. Nothing bad happens here, but in an artificial > scenario where extra_point_info is updated while this function is running you > could get points labeled with wrong messages. > > Since it's not a performance issue, I want |message| moved into the loop. The > same for all the other variables except |header|. > > Also, consider renaming message to maybe 'point_info'. This is a rather artificial scenario. The code is not thread safe anyway, so the only thing to worry about is updating |extra_point_info| within this loop. However, changing the state of |extra_point_info| would be a major change to what you are plotting, similar to changing which packet direction or which log file you are plotting. This is typically not done at all and should definitely not be done while a plot is being generated. Anyway, I moved the declaration of |message| and made |extra_point_info| const, but I disagree with moving the other variables. They are always accessed together so it would be surprising if |header| wasn't declared with the others. Moreover, the logic of the loop would be harder to follow if it was interspersed with boiler-plate declarations. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:165: max_y = (y > max_y) ? y : max_y; On 2016/06/08 11:44:20, aleloi wrote: > Make it consistent with the other min/max calculations. Unless this is the > standard in webrtc, I vote for std::max. Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:176: // Set labels and put in graph On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > End with . Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:188: plot->yaxis_max = max_y * 1.1; On 2016/06/08 11:44:20, aleloi wrote: > Make 1.1, 1.02, -1 constants. Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:234: plot->xaxis_max = (end_time - begin_time) / 1000000 * 1.02; On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Maybe name the constant 1.02 "kXMargin" or something like that? Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:237: plot->yaxis_max = max_y * 1.1; On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > kYMargin? Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:326: // On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Remove? Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:336: const std::string& extension = config.rtp.extensions[j].name; On 2016/06/03 08:51:11, aleloi wrote: > RtpExtension.name was removed in 6f8d686d. It seems to have been renamed into > `RtpExtension.uri' now. Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:425: // TODO(terelius): Refactor On 2016/05/31 18:53:39, stefan-webrtc (holmer) wrote: > Is the plan to base this method on the previous one in some way? Not really base one on the other, but there is a lot of similarities in parsing and filtering the packets. I'm currently leaning towards keeping a list of parsed RTP headers in the class and add convenience functions to find the next packet that matches a certain set of conditions. I think that could reduce the code size of all/most of the CreateXYZGraph() functions. Though there are other options too, and it is possible that both graphs should be generated at the same time. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:439: double accumulatedDelay; // In milliseconds On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > No camel case Done. However, this means that I am no longer consistent with RtpUtility::RtpHeaderParser. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:25: // duration of it's lifetime. The ParsedRtcEventLog must not be destroyed or On 2016/06/08 11:44:20, aleloi wrote: > it's -> its Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:27: EventLogAnalyzer(const ParsedRtcEventLog& log, bool extra_info); On 2016/06/08 11:44:20, aleloi wrote: > Add a comment describing extra_info Done. Note that there isn't (yet) any way to visualize the additional information. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:44: const ParsedRtcEventLog& parsed_log; On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > End all members with _ Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:49: uint64_t step; On 2016/06/08 11:44:20, aleloi wrote: > Consider adding a comment that describes what |step| and |window_duration| is > used for. The other members have more self-explanatory names and are used in a > straight-forward way. Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/generate_timeseries.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/generate_timeseries.cc:35: "Plot the difference in sequence number between consecutive packets."); On 2016/06/08 11:44:20, aleloi wrote: > number -> numbers, although I am not certain. "Difference in" typically refers to a single thing. (In this case the sequence number property.) If it was "difference between the sequence numbers of consecutive packets", it would for sure be plural. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/generate_timeseries.cc:147: collection->plots.back().get()); On 2016/06/08 11:44:20, aleloi wrote: > Do you think it's possible to reduce the repetition somehow? E.g. there are 10 > calls to collection->plots.back().get() and collection->append_plot(); > > What about > > auto get_last_plot = [&]() { return collection->plots.back().get(); }; > > ? > > Or make append_plot() return a pointer to the appended plot Followed your second suggestion and returned the new plot. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:24: TimeSeriesPoint(float x_, float y_) : x(x_), y(y_) {} On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > I don't think the _ are needed? Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:39: points.swap(other.points); On 2016/06/08 11:44:20, aleloi wrote: > Use std::swap in all cases for consistency. It is specialized to strings and > vectors and does the same as the member swap. Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:48: public: On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Private? The members are set by all the CreateGraph() functions. Making the CreateGraph() functions friends or members isn't a good solution since that would couple the representation framework to the application. I could create setters and getters for everything, but that would just be a verbose way to bypass the private access specifier. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:64: std::vector<std::unique_ptr<Plot> > plots; On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > Would probably make sense to make this private, right? It would have to be protected since we want to access this from the derived classes. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_base.h:64: std::vector<std::unique_ptr<Plot> > plots; On 2016/06/08 11:44:20, aleloi wrote: > On 2016/05/31 18:53:40, stefan-webrtc (holmer) wrote: > > Would probably make sense to make this private, right? > > It is accessed in the main function and in analysis. Maybe make analysis friend > and make append_plot return the appended plot? The abstract representation of a graph should not have a dependency on any particular analysis of a concrete data set, but I can make append_plot return a pointer to the new Plot. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_python.cc:90: plots.push_back(std::unique_ptr<Plot>(static_cast<Plot*>(new PythonPlot()))); On 2016/06/08 11:44:20, aleloi wrote: > The explicit cast is not needed here. PythonPlot* is a kind of Plot*, so it can > be safely given to the unique_ptr constructor. > > (If I understand the C++ type system correctly. Let's run the bots or look it up > to be sure). Rewritten in accordance with other comments. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/plot_python.h (right): https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_python.h:22: virtual void draw(); On 2016/06/03 08:51:11, aleloi wrote: > The style guide says to use 'override'. > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > Explicitly annotate overrides of virtual functions or virtual destructors with > an override... Done. https://codereview.webrtc.org/1995523002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/plot_python.h:29: virtual void draw(); On 2016/06/03 08:51:11, aleloi wrote: > here as well Done.
lgtm
https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:33: std::string HeaderToString(const webrtc::RTPHeader& parsed_header) { Should we make this a method on RTPHeader instead? https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:92: uint64_t GetStreamId(uint32_t ssrc, Wouldn't it be nicer to make stream id a comparable type instead? class StreamId { public: operator<() {...} private: uint32_t ssrc_; webrtc::PacketDirection direction_; webrtc::MediaType media_type_; }; Seems like a less hacky way of achieving the same thing https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:142: step_ = 10000; These two members should be set in the initializer list. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:187: plot->series.back().swap(kv.second); Why not simply push_back(kv.second)? https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:334: if (event_type == ParsedRtcEventLog::VIDEO_RECEIVER_CONFIG_EVENT) { Handling of config events seems to happen in many Create...Graph() methods. Can we break it out? I assume it can be handled similarly for all graphs? https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:374: if (MatchingSsrc(parsed_header.ssrc, desired_ssrc_)) { Maybe we could have a method similar to: GetEvents(ssrc, direction, types) which returns a list of events matching the filters we're interested in? That way we would separate filtering from the plotting and computation code. WDYT? https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:628: // For each SSRC, plot the bandwitch used by that stream. bandwidth https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_base.h:51: float xaxis_min; Can these be protected and only set in the constructor? https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_python.cc:12: #include <stdio.h> Empty line above. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_python.cc:27: // TODO(terelius): Check non-zero number of data series. Seems like it should be easily done already?
https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:33: std::string HeaderToString(const webrtc::RTPHeader& parsed_header) { On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > Should we make this a method on RTPHeader instead? I have no opinion on this, but this function does not serialize all fields of the header, only a few that I though would be interesting to look at. If it was a method on RTPHEader, I guess it should be more general. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:92: uint64_t GetStreamId(uint32_t ssrc, On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > Wouldn't it be nicer to make stream id a comparable type instead? > class StreamId { > public: > operator<() {...} > > private: > uint32_t ssrc_; > webrtc::PacketDirection direction_; > webrtc::MediaType media_type_; > }; > > Seems like a less hacky way of achieving the same thing Done. However, the advantage of precomputing a 64-bit identifier is that we would only need 1 comparison instead of 5 when we're comparing two streams. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:142: step_ = 10000; On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > These two members should be set in the initializer list. Done. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:187: plot->series.back().swap(kv.second); On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > Why not simply push_back(kv.second)? Because I don't want to copy all the data in the TimeSeries object. I guess the better way to achieve the same effect is to push_back(std::move(kv.second)) and overload the move constructor and move assignment operator. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:334: if (event_type == ParsedRtcEventLog::VIDEO_RECEIVER_CONFIG_EVENT) { On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > Handling of config events seems to happen in many Create...Graph() methods. Can > we break it out? I assume it can be handled similarly for all graphs? Config events are used in CreateDelayChangeGraph and CreateAccumulatedDelayChangeGraph to get the header extensions. I have another CL that breaks out parsing of the RTP packets. That will parse the packets once and cache a map from SSRC to RTPHeader. Do you want me to merge that into this CL? https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:374: if (MatchingSsrc(parsed_header.ssrc, desired_ssrc_)) { On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > Maybe we could have a method similar to: > GetEvents(ssrc, direction, types) > > which returns a list of events matching the filters we're interested in? That > way we would separate filtering from the plotting and computation code. WDYT? The follow up CL that caches a map from streams to parsed headers would mostly accomplish this. We could iterate over the streams and "continue;" for all streams we are not interested in. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:628: // For each SSRC, plot the bandwitch used by that stream. On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > bandwidth Done. :)
https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:33: std::string HeaderToString(const webrtc::RTPHeader& parsed_header) { On 2016/07/01 16:54:45, terelius wrote: > On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > > Should we make this a method on RTPHeader instead? > > I have no opinion on this, but this function does > not serialize all fields of the header, only a few that > I though would be interesting to look at. If it was a > method on RTPHEader, I guess it should be more general. I think it's fine as is for now. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:334: if (event_type == ParsedRtcEventLog::VIDEO_RECEIVER_CONFIG_EVENT) { On 2016/07/01 16:54:45, terelius wrote: > On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > > Handling of config events seems to happen in many Create...Graph() methods. > Can > > we break it out? I assume it can be handled similarly for all graphs? > > Config events are used in CreateDelayChangeGraph and > CreateAccumulatedDelayChangeGraph to get the header extensions. > > I have another CL that breaks out parsing of the RTP packets. That will parse > the packets once and cache a map from SSRC to RTPHeader. Do you want me to merge > that into this CL? If it becomes better in a follow up I think I'm ok with waiting for that. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:374: if (MatchingSsrc(parsed_header.ssrc, desired_ssrc_)) { On 2016/07/01 16:54:45, terelius wrote: > On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > > Maybe we could have a method similar to: > > GetEvents(ssrc, direction, types) > > > > which returns a list of events matching the filters we're interested in? That > > way we would separate filtering from the plotting and computation code. WDYT? > > The follow up CL that caches a map from streams to parsed headers would mostly > accomplish this. We could iterate over the streams and "continue;" for all > streams we are not interested in. Acknowledged. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:628: // For each SSRC, plot the bandwitch used by that stream. On 2016/07/01 16:54:45, terelius wrote: > On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > > bandwidth > > Done. :) Nope, still there... :) https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_python.cc:12: #include <stdio.h> On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > Empty line above. Could the comments in this file still be addressed? https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:95: bool operator<(const StreamId& other) const { empty line above https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:111: bool operator==(const StreamId& other) const { empty line https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:115: uint32_t GetSsrc() const { return ssrc_; } and here https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_base.h:34: TimeSeries(TimeSeries&& other) : label(std::move(other.label)), Not sure about this one, but should this be explicit?
https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:628: // For each SSRC, plot the bandwitch used by that stream. On 2016/07/05 08:58:11, stefan-webrtc (holmer) wrote: > On 2016/07/01 16:54:45, terelius wrote: > > On 2016/06/29 11:13:04, stefan-webrtc (holmer) wrote: > > > bandwidth > > > > Done. :) > > Nope, still there... :) Done. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_base.h:51: float xaxis_min; On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > Can these be protected and only set in the constructor? No, the axis limits depend on the data points which aren't known when the object is constructed. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_python.cc:12: #include <stdio.h> On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > Empty line above. Done. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_python.cc:12: #include <stdio.h> On 2016/07/05 08:58:11, stefan-webrtc (holmer) wrote: > On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > > Empty line above. > > Could the comments in this file still be addressed? Done. https://codereview.webrtc.org/1995523002/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_python.cc:27: // TODO(terelius): Check non-zero number of data series. On 2016/06/29 11:13:05, stefan-webrtc (holmer) wrote: > Seems like it should be easily done already? Done. https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:95: bool operator<(const StreamId& other) const { On 2016/07/05 08:58:12, stefan-webrtc (holmer) wrote: > empty line above Done. https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:111: bool operator==(const StreamId& other) const { On 2016/07/05 08:58:11, stefan-webrtc (holmer) wrote: > empty line Done. https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.cc:115: uint32_t GetSsrc() const { return ssrc_; } On 2016/07/05 08:58:12, stefan-webrtc (holmer) wrote: > and here Done. https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/1995523002/diff/60001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_base.h:34: TimeSeries(TimeSeries&& other) : label(std::move(other.label)), On 2016/07/05 08:58:12, stefan-webrtc (holmer) wrote: > Not sure about this one, but should this be explicit? I don't really think it matters, but the style guide say: "Type conversion operators, and constructors that are callable with a single argument, must be marked explicit in the class definition. As an exception, copy and move constructors should not be explicit, since they do not perform type conversion."
lgtm with nit fixed. https://codereview.webrtc.org/1995523002/diff/80001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/1995523002/diff/80001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_python.cc:26: if (series.size() > 0) { !series.empty()
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1995523002/#ps140001 (title: "Fix trivially true DCHECK.")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6734)
terelius@webrtc.org changed reviewers: + phoglund@webrtc.org
https://codereview.webrtc.org/1995523002/diff/140001/webrtc/tools/BUILD.gn File webrtc/tools/BUILD.gn (right): https://codereview.webrtc.org/1995523002/diff/140001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:199: "../modules/rtp_rtcp:rtp_rtcp", The dependency lists differ slightly between gyp and gn. Is there no need to depend on the proto in the gyp case?
https://codereview.webrtc.org/1995523002/diff/140001/webrtc/tools/BUILD.gn File webrtc/tools/BUILD.gn (right): https://codereview.webrtc.org/1995523002/diff/140001/webrtc/tools/BUILD.gn#ne... webrtc/tools/BUILD.gn:199: "../modules/rtp_rtcp:rtp_rtcp", On 2016/07/11 14:28:53, phoglund wrote: > The dependency lists differ slightly between gyp and gn. Is there no need to > depend on the proto in the gyp case? As discussed offline, the gyp file uses export_dependent_settings to avoid having an explicit dependency on the proto. I've added the proto to the public_deps of the parser, to make the gn case analogous.
lgtm https://codereview.webrtc.org/1995523002/diff/160001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/1995523002/diff/160001/webrtc/BUILD.gn#newcode362 webrtc/BUILD.gn:362: ":rtc_event_log_proto", Nice, now it matches the export_dependent_settings of the gyp version. Not sure if it should also export webrtc_common but I can't find an equivalent of that in gyp so I'm not going to complain.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1995523002/#ps160001 (title: "Add rtc_event_log_proto to the public_deps of rtc_event_log_parser.")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6817)
terelius@webrtc.org changed reviewers: + henrika@webrtc.org
LGTM
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 ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. ========== to ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. ========== to ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d Cr-Commit-Position: refs/heads/master@{#13443} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a478786ef1513790194792010f766324a469db4d Cr-Commit-Position: refs/heads/master@{#13443}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.webrtc.org/2147453002/ by terelius@webrtc.org. The reason for reverting is: Reverting while investigating a downstream build failure..
Message was sent while issue was closed.
Description was changed from ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d Cr-Commit-Position: refs/heads/master@{#13443} ========== to ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d Cr-Commit-Position: refs/heads/master@{#13443} ==========
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: This issue passed the CQ dry run.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, phoglund@webrtc.org, stefan@webrtc.org, henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/1995523002/#ps180001 (title: "Use include_tests==1 to avoid compiling if gflags isn't available.")
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 ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d Cr-Commit-Position: refs/heads/master@{#13443} ========== to ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d Cr-Commit-Position: refs/heads/master@{#13443} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d Cr-Commit-Position: refs/heads/master@{#13443} ========== to ========== Initial version of the local visualization tool for WebrtcEventLogs. Plot graphs to python code. Generate packet, playout, sequence number and total bitrate graphs. Add bitrate and latency plots. Generate multiple figures, and control which ones are used through command line flags. Committed: https://crrev.com/a478786ef1513790194792010f766324a469db4d Committed: https://crrev.com/54ce68017008907b4ee2b21d23cf616028cec2da Cr-Original-Commit-Position: refs/heads/master@{#13443} Cr-Commit-Position: refs/heads/master@{#13463} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/54ce68017008907b4ee2b21d23cf616028cec2da Cr-Commit-Position: refs/heads/master@{#13463} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
