Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826313004:
Added -show_detector_state which show the detector state in the total bitrate graph. (Closed)
DescriptionAdded -show_detector_state which show the detector state in the total bitrate graph.
BUG=none
Review-Url: https://codereview.webrtc.org/2826313004
Cr-Commit-Position: refs/heads/master@{#19020}
Committed: https://chromium.googlesource.com/external/webrtc/+/23c7f2526621ae9283e694a7c329780713dafb25
Patch Set 1 #
Total comments: 11
Patch Set 2 : Feedback #
Total comments: 3
Patch Set 3 : Feedback #Patch Set 4 : Rebase #Patch Set 5 : IntervalSeries struct #
Total comments: 12
Patch Set 6 : Feedback #
Total comments: 6
Patch Set 7 : Revert window_duration and rebase #Patch Set 8 : kVertical ---> kHorizontal #
Total comments: 6
Patch Set 9 : Intervals now show up in the legend. #
Messages
Total messages: 26 (4 generated)
philipel@webrtc.org changed reviewers: + terelius@webrtc.org
https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:948: overusing_series.color = "#ff8e82"; Color seems like a UI choice. I'd prefer to not include that in the TimeSeries object. https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:956: BandwidthUsage detector_state = BandwidthUsage::kBwNormal; last_detector_state to make it correspond to last_series? https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:90: bool show_detector_state); Please give show_detector_state a default value false to maintain a backwards compatible interface.
https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:948: overusing_series.color = "#ff8e82"; On 2017/04/24 11:58:06, terelius wrote: > Color seems like a UI choice. I'd prefer to not include that in the TimeSeries > object. I think it makes sense to have defined colors for overuse/underuse/normal state. If we used generated colors they might be different every time, but also overuse might be marked as green, which seems counter intuitive to me. https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:956: BandwidthUsage detector_state = BandwidthUsage::kBwNormal; On 2017/04/24 11:58:06, terelius wrote: > last_detector_state to make it correspond to last_series? Having a detector state makes things so much easier to detect when the state has changed (line 963). https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:90: bool show_detector_state); On 2017/04/24 11:58:06, terelius wrote: > Please give show_detector_state a default value false to maintain a backwards > compatible interface. Done.
https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:948: overusing_series.color = "#ff8e82"; On 2017/04/26 12:56:28, philipel wrote: > On 2017/04/24 11:58:06, terelius wrote: > > Color seems like a UI choice. I'd prefer to not include that in the > TimeSeries > > object. > > I think it makes sense to have defined colors for overuse/underuse/normal state. > If we used generated colors they might be different every time, but also overuse > might be marked as green, which seems counter intuitive to me. But with the new code, every TimeSeries will have a color attribute which gets ignored by the drawing code in almost all cases. This seems even more counter intuitive to me. If we want to hard-code colors, then I think it would be more appropriate to do so in the drawing code. Maybe a VSPAN series labeled "Overusing" should always be rendered in red? Or maybe the VSPAN shouldn't be a TimeSeries at all, but a completely separate object? https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:956: BandwidthUsage detector_state = BandwidthUsage::kBwNormal; On 2017/04/26 12:56:28, philipel wrote: > On 2017/04/24 11:58:06, terelius wrote: > > last_detector_state to make it correspond to last_series? > > Having a detector state makes things so much easier to detect when the state has > changed (line 963). I meant changing the name from detector_state to last_detector_state, since it is not the current state but the state at the last BWE update.
https://codereview.webrtc.org/2826313004/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/2826313004/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_base.h:27: VSPAN_GRAPH Shouldn't this be called HSPAN (or HORIZONTAL_SPAN or HORIZONTAL_INTERVAL) since it spans a horizontal region?
https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:948: overusing_series.color = "#ff8e82"; On 2017/04/26 13:40:09, terelius wrote: > On 2017/04/26 12:56:28, philipel wrote: > > On 2017/04/24 11:58:06, terelius wrote: > > > Color seems like a UI choice. I'd prefer to not include that in the > > TimeSeries > > > object. > > > > I think it makes sense to have defined colors for overuse/underuse/normal > state. > > If we used generated colors they might be different every time, but also > overuse > > might be marked as green, which seems counter intuitive to me. > > But with the new code, every TimeSeries will have a color attribute which gets > ignored by the drawing code in almost all cases. This seems even more counter > intuitive to me. > > If we want to hard-code colors, then I think it would be more appropriate to do > so in the drawing code. Maybe a VSPAN series labeled "Overusing" should always > be rendered in red? Or maybe the VSPAN shouldn't be a TimeSeries at all, but a > completely separate object? I think the best would be to make color optional, and if it is set then we use that, if not let the plot tool chose color. https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:956: BandwidthUsage detector_state = BandwidthUsage::kBwNormal; On 2017/04/26 13:40:09, terelius wrote: > On 2017/04/26 12:56:28, philipel wrote: > > On 2017/04/24 11:58:06, terelius wrote: > > > last_detector_state to make it correspond to last_series? > > > > Having a detector state makes things so much easier to detect when the state > has > > changed (line 963). > > I meant changing the name from detector_state to last_detector_state, since it > is not the current state but the state at the last BWE update. Ah... Done :) https://codereview.webrtc.org/2826313004/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/2826313004/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_base.h:27: VSPAN_GRAPH On 2017/04/26 13:51:51, terelius wrote: > Shouldn't this be called HSPAN (or HORIZONTAL_SPAN or HORIZONTAL_INTERVAL) since > it spans a horizontal region? Renamed to VERTICAL_SPAN.
https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:948: overusing_series.color = "#ff8e82"; On 2017/04/26 14:08:01, philipel wrote: > On 2017/04/26 13:40:09, terelius wrote: > > On 2017/04/26 12:56:28, philipel wrote: > > > On 2017/04/24 11:58:06, terelius wrote: > > > > Color seems like a UI choice. I'd prefer to not include that in the > > > TimeSeries > > > > object. > > > > > > I think it makes sense to have defined colors for overuse/underuse/normal > > state. > > > If we used generated colors they might be different every time, but also > > overuse > > > might be marked as green, which seems counter intuitive to me. > > > > But with the new code, every TimeSeries will have a color attribute which gets > > ignored by the drawing code in almost all cases. This seems even more counter > > intuitive to me. > > > > If we want to hard-code colors, then I think it would be more appropriate to > do > > so in the drawing code. Maybe a VSPAN series labeled "Overusing" should always > > be rendered in red? Or maybe the VSPAN shouldn't be a TimeSeries at all, but a > > completely separate object? > > I think the best would be to make color optional, and if it is set then we use > that, if not let the plot tool chose color. That would make it hard to write the drawing code with a clean color scheme or palette. For instance, you can't guarantee that different colors are clearly distinguishable. Adding a color also mixes the semantic content of the data with the visualization details. Since the new series type seems to be a special case compared the existing ones, I still think it would be better to either: 1) In the drawing code for VSPAN, set the color to #ff8e82, #5092fc or #c4ffc4 depending on the label used. It would still be a "hack", but at least it would be localized to only the VSPAN type. 2) Create a new class to represent the colored background. This would open up many new possibilities both for more efficient encodings, clearer variable names, and alternative visualizations in the future. For example, struct Interval { double begin; double end; } struct IntervalSeries { string label; string color; vector<Interval> intervals; } In particular, we would get rid of the unused y-coordinates and we would no longer have to interpret points differently depending on whether they are placed in an even or odd position in the TimeSeries. https://codereview.webrtc.org/2826313004/diff/20001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/2826313004/diff/20001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/plot_base.h:27: VSPAN_GRAPH On 2017/04/26 14:08:01, philipel wrote: > On 2017/04/26 13:51:51, terelius wrote: > > Shouldn't this be called HSPAN (or HORIZONTAL_SPAN or HORIZONTAL_INTERVAL) > since > > it spans a horizontal region? > > Renamed to VERTICAL_SPAN. But isn't it a horizontal span?
Updated with feedback, PTAL
https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_base.h:55: std::string color; This is not used as far as I can tell. Furthermore, I still would like to separate the data from the visualization details like color. https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:77: "path_effects=[pe.Stroke(linewidth=2, foreground='black'), " Why do we need to do this at all, and why do we only need to do it for step graphs? https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:91: // IntervalSeries If this represents the background, shouldn't it be drawn before the normal time series? https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:96: printf("i%zu = [", i); Maybe a longer, more explicit name for the intervals? "i" sounds like an index. https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:101: for (size_t j = 1; j < interval_list_[i].intervals.size(); j++) { Maybe remove the if statement above, loop from 0 and add if (j > 0) printf(", ") in the loop?
https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_base.h:55: std::string color; On 2017/07/04 12:22:01, terelius wrote: > This is not used as far as I can tell. Furthermore, I still would like to > separate the data from the visualization details like color. Removed. Not sure I understand what you mean by separating data from visualization. Both TimeSeries and Interval is a generalization of different types of data for the sole purpose of visualizing it, so in my mind these classes IS the separation. Of course, if you don't care about the color then it's nice that you don't have to specify it. https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:77: "path_effects=[pe.Stroke(linewidth=2, foreground='black'), " On 2017/07/04 12:22:01, terelius wrote: > Why do we need to do this at all, and why do we only need to do it for step > graphs? I guess it should be configurable by whoever creates the step graph, but in the case of the total bitrate figure it looks much better. https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:91: // IntervalSeries On 2017/07/04 12:22:01, terelius wrote: > If this represents the background, shouldn't it be drawn before the normal time > series? They are always in the background. https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:96: printf("i%zu = [", i); On 2017/07/04 12:22:01, terelius wrote: > Maybe a longer, more explicit name for the intervals? "i" sounds like an index. Renamed to 'ival'. https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:101: for (size_t j = 1; j < interval_list_[i].intervals.size(); j++) { On 2017/07/04 12:22:01, terelius wrote: > Maybe remove the if statement above, loop from 0 and add > if (j > 0) > printf(", ") > in the loop? This is consistent with the style above, if we want to change it I think we should change it in a separate CL.
https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_base.h:55: std::string color; On 2017/07/04 12:55:10, philipel wrote: > On 2017/07/04 12:22:01, terelius wrote: > > This is not used as far as I can tell. Furthermore, I still would like to > > separate the data from the visualization details like color. > > Removed. > > Not sure I understand what you mean by separating data from visualization. Both > TimeSeries and Interval is a generalization of different types of data for the > sole purpose of visualizing it, so in my mind these classes IS the separation. > > Of course, if you don't care about the color then it's nice that you don't have > to specify it. What I mean is that "connect these point with a line" is something that only depends on the data, and can be carried out by any visualization framework. The choice of color is not part of the data and different visualization frameworks may or may not be able to do it. Furthermore, many frameworks choose fairly sophisticated color schemes to make sure that different curves are distinguishable without making the colors clash. If we specify colors for some but not all data, we're going to clash with the color scheme of the visualization framework. On the other hand, specifying colors for all data series would duplicate a lot of the complex logic that's already (and more appropriately) done in the visualization framework. Hence, I don't think we should be able to specify any colors at all. https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... File webrtc/rtc_tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/analyzer.cc:295: : parsed_log_(log), window_duration_(500000), step_(10000) { Changing this will make the curves less noisy, but it also reduces the time resolution. What's the rationale for 500 ms rather than 250 ms? Very low bitrate scenarios where we send few packets? https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/analyzer.cc:963: IntervalSeries::kVertical); Shouldn't this be "Horizontal"? You are trying to highlight a certain interval on the x-axis.
https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... File webrtc/rtc_tools/event_log_visualizer/plot_base.h (right): https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... webrtc/rtc_tools/event_log_visualizer/plot_base.h:55: std::string color; On 2017/07/07 13:42:01, terelius wrote: > On 2017/07/04 12:55:10, philipel wrote: > > On 2017/07/04 12:22:01, terelius wrote: > > > This is not used as far as I can tell. Furthermore, I still would like to > > > separate the data from the visualization details like color. > > > > Removed. > > > > Not sure I understand what you mean by separating data from visualization. > Both > > TimeSeries and Interval is a generalization of different types of data for the > > sole purpose of visualizing it, so in my mind these classes IS the separation. > > > > Of course, if you don't care about the color then it's nice that you don't > have > > to specify it. > > What I mean is that "connect these point with a line" is something that only > depends on the data, and can be carried out by any visualization framework. To represent some data using a LINE_GRAPH, LINE_DOT_GRAPH or a BAR_GRAPH is a choice made by the programmer on how to visualize the data. Choosing some color for that specific data is just another decision about how it should be visualized. > The > choice of color is not part of the data and different visualization frameworks > may or may not be able to do it. Furthermore, many frameworks choose fairly > sophisticated color schemes to make sure that different curves are > distinguishable without making the colors clash. If we specify colors for some > but not all data, we're going to clash with the color scheme of the > visualization framework. On the other hand, specifying colors for all data > series would duplicate a lot of the complex logic that's already (and more > appropriately) done in the visualization framework. > > Hence, I don't think we should be able to specify any colors at all. Not having to specify color for a given graph and having the framework choose it for you is convenient, but for some situations (like this) it will not give you a good result. In this case you want to have lighter background colors that doesn't interfere with the drawn graphs, and it is also a plus to have consistent colors so that overuse is always represented by red and underuse by blue for example. https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... File webrtc/rtc_tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/analyzer.cc:295: : parsed_log_(log), window_duration_(500000), step_(10000) { On 2017/07/07 13:42:01, terelius wrote: > Changing this will make the curves less noisy, but it also reduces the time > resolution. What's the rationale for 500 ms rather than 250 ms? Very low bitrate > scenarios where we send few packets? Oops, this was not meant to be changed, reverted. https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/analyzer.cc:963: IntervalSeries::kVertical); On 2017/07/07 13:42:01, terelius wrote: > Shouldn't this be "Horizontal"? You are trying to highlight a certain interval > on the x-axis. The area span vertically, but only in an interval horizontally.
On 2017/07/11 14:12:35, philipel wrote: > https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... > File webrtc/rtc_tools/event_log_visualizer/plot_base.h (right): > > https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... > webrtc/rtc_tools/event_log_visualizer/plot_base.h:55: std::string color; > On 2017/07/07 13:42:01, terelius wrote: > > On 2017/07/04 12:55:10, philipel wrote: > > > On 2017/07/04 12:22:01, terelius wrote: > > > > This is not used as far as I can tell. Furthermore, I still would like to > > > > separate the data from the visualization details like color. > > > > > > Removed. > > > > > > Not sure I understand what you mean by separating data from visualization. > > Both > > > TimeSeries and Interval is a generalization of different types of data for > the > > > sole purpose of visualizing it, so in my mind these classes IS the > separation. > > > > > > Of course, if you don't care about the color then it's nice that you don't > > have > > > to specify it. > > > > What I mean is that "connect these point with a line" is something that only > > depends on the data, and can be carried out by any visualization framework. > > To represent some data using a LINE_GRAPH, LINE_DOT_GRAPH or a BAR_GRAPH is a > choice made by the programmer on how to visualize the data. Choosing some color > for that specific data is just another decision about how it should be > visualized. Maybe for dots, but the other graph types aren't visualization choices. For example, a LINE_GRAPH represents a continuously varying quantity whereas a STEP_GRAPH represents a setting that retains it's previous value until changed. > > The > > choice of color is not part of the data and different visualization frameworks > > may or may not be able to do it. Furthermore, many frameworks choose fairly > > sophisticated color schemes to make sure that different curves are > > distinguishable without making the colors clash. If we specify colors for some > > but not all data, we're going to clash with the color scheme of the > > visualization framework. On the other hand, specifying colors for all data > > series would duplicate a lot of the complex logic that's already (and more > > appropriately) done in the visualization framework. > > > > Hence, I don't think we should be able to specify any colors at all. > > Not having to specify color for a given graph and having the framework choose it > for you is convenient, but for some situations (like this) it will not give you > a good result. In this case you want to have lighter background colors that > doesn't interfere with the drawn graphs, and it is also a plus to have > consistent colors so that overuse is always represented by red and underuse by > blue for example. But if the visualization chooses poor colors for highlighting intervals, maybe that should be fixed in the visualization framework, i.e. the generated python code?
https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... File webrtc/rtc_tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/analyzer.cc:963: IntervalSeries::kVertical); On 2017/07/11 14:12:35, philipel wrote: > On 2017/07/07 13:42:01, terelius wrote: > > Shouldn't this be "Horizontal"? You are trying to highlight a certain interval > > on the x-axis. > > The area span vertically, but only in an interval horizontally. Yes, but then it is a horizontal interval. Whether or not the interval is visualized by coloring the full vertical span of the background is an implementation detail. You could also highlight the interval by coloring the relevant part of the x-axis for example.
On 2017/07/11 14:45:03, terelius wrote: > On 2017/07/11 14:12:35, philipel wrote: > > > https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... > > File webrtc/rtc_tools/event_log_visualizer/plot_base.h (right): > > > > > https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... > > webrtc/rtc_tools/event_log_visualizer/plot_base.h:55: std::string color; > > On 2017/07/07 13:42:01, terelius wrote: > > > On 2017/07/04 12:55:10, philipel wrote: > > > > On 2017/07/04 12:22:01, terelius wrote: > > > > > This is not used as far as I can tell. Furthermore, I still would like > to > > > > > separate the data from the visualization details like color. > > > > > > > > Removed. > > > > > > > > Not sure I understand what you mean by separating data from visualization. > > > Both > > > > TimeSeries and Interval is a generalization of different types of data for > > the > > > > sole purpose of visualizing it, so in my mind these classes IS the > > separation. > > > > > > > > Of course, if you don't care about the color then it's nice that you don't > > > have > > > > to specify it. > > > > > > What I mean is that "connect these point with a line" is something that only > > > depends on the data, and can be carried out by any visualization framework. > > > > To represent some data using a LINE_GRAPH, LINE_DOT_GRAPH or a BAR_GRAPH is a > > choice made by the programmer on how to visualize the data. Choosing some > color > > for that specific data is just another decision about how it should be > > visualized. > > Maybe for dots, but the other graph types aren't visualization choices. > For example, a LINE_GRAPH represents a continuously varying quantity whereas a > STEP_GRAPH represents a setting that retains it's previous value until changed. The point is that the visualization framework can't decide whether it should use a bar graph, lines or dotted lines to visualize the data in a good way. The same is true for colors (even if it can choose colors well in many cases). > > > The > > > choice of color is not part of the data and different visualization > frameworks > > > may or may not be able to do it. Furthermore, many frameworks choose fairly > > > sophisticated color schemes to make sure that different curves are > > > distinguishable without making the colors clash. If we specify colors for > some > > > but not all data, we're going to clash with the color scheme of the > > > visualization framework. On the other hand, specifying colors for all data > > > series would duplicate a lot of the complex logic that's already (and more > > > appropriately) done in the visualization framework. > > > > > > Hence, I don't think we should be able to specify any colors at all. > > > > Not having to specify color for a given graph and having the framework choose > it > > for you is convenient, but for some situations (like this) it will not give > you > > a good result. In this case you want to have lighter background colors that > > doesn't interfere with the drawn graphs, and it is also a plus to have > > consistent colors so that overuse is always represented by red and underuse by > > blue for example. > > But if the visualization chooses poor colors for highlighting intervals, maybe > that > should be fixed in the visualization framework, i.e. the generated python code? Can we do that in another CL? I'm not sure how we would fix that. I feel like we have a simple solution to a simple problem, and if we really need a more advanced solution (which I'm not sure if we will ever need) we can implement that later on.
https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... File webrtc/rtc_tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2826313004/diff/100001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/analyzer.cc:963: IntervalSeries::kVertical); On 2017/07/11 14:45:11, terelius wrote: > On 2017/07/11 14:12:35, philipel wrote: > > On 2017/07/07 13:42:01, terelius wrote: > > > Shouldn't this be "Horizontal"? You are trying to highlight a certain > interval > > > on the x-axis. > > > > The area span vertically, but only in an interval horizontally. > > Yes, but then it is a horizontal interval. Whether or not the interval is > visualized by coloring the full vertical span of the background is an > implementation detail. You could also highlight the interval by coloring the > relevant part of the x-axis for example. Ah, I see what you mean, fixed.
On 2017/07/11 15:38:37, philipel wrote: > On 2017/07/11 14:45:03, terelius wrote: > > On 2017/07/11 14:12:35, philipel wrote: > > > > > > https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... > > > File webrtc/rtc_tools/event_log_visualizer/plot_base.h (right): > > > > > > > > > https://codereview.webrtc.org/2826313004/diff/80001/webrtc/rtc_tools/event_lo... > > > webrtc/rtc_tools/event_log_visualizer/plot_base.h:55: std::string color; > > > On 2017/07/07 13:42:01, terelius wrote: > > > > On 2017/07/04 12:55:10, philipel wrote: > > > > > On 2017/07/04 12:22:01, terelius wrote: > > > > > > This is not used as far as I can tell. Furthermore, I still would like > > to > > > > > > separate the data from the visualization details like color. > > > > > > > > > > Removed. > > > > > > > > > > Not sure I understand what you mean by separating data from > visualization. > > > > Both > > > > > TimeSeries and Interval is a generalization of different types of data > for > > > the > > > > > sole purpose of visualizing it, so in my mind these classes IS the > > > separation. > > > > > > > > > > Of course, if you don't care about the color then it's nice that you > don't > > > > have > > > > > to specify it. > > > > > > > > What I mean is that "connect these point with a line" is something that > only > > > > depends on the data, and can be carried out by any visualization > framework. > > > > > > To represent some data using a LINE_GRAPH, LINE_DOT_GRAPH or a BAR_GRAPH is > a > > > choice made by the programmer on how to visualize the data. Choosing some > > color > > > for that specific data is just another decision about how it should be > > > visualized. > > > > Maybe for dots, but the other graph types aren't visualization choices. > > For example, a LINE_GRAPH represents a continuously varying quantity whereas a > > > STEP_GRAPH represents a setting that retains it's previous value until > changed. > > The point is that the visualization framework can't decide whether it should use > a bar graph, lines or dotted lines to visualize the data in a good way. The same > is true for colors (even if it can choose colors well in many cases). > > > > > The > > > > choice of color is not part of the data and different visualization > > frameworks > > > > may or may not be able to do it. Furthermore, many frameworks choose > fairly > > > > sophisticated color schemes to make sure that different curves are > > > > distinguishable without making the colors clash. If we specify colors for > > some > > > > but not all data, we're going to clash with the color scheme of the > > > > visualization framework. On the other hand, specifying colors for all data > > > > series would duplicate a lot of the complex logic that's already (and more > > > > appropriately) done in the visualization framework. > > > > > > > > Hence, I don't think we should be able to specify any colors at all. > > > > > > Not having to specify color for a given graph and having the framework > choose > > it > > > for you is convenient, but for some situations (like this) it will not give > > you > > > a good result. In this case you want to have lighter background colors that > > > doesn't interfere with the drawn graphs, and it is also a plus to have > > > consistent colors so that overuse is always represented by red and underuse > by > > > blue for example. > > > > But if the visualization chooses poor colors for highlighting intervals, maybe > > that > > should be fixed in the visualization framework, i.e. the generated python > code? > > Can we do that in another CL? I'm not sure how we would fix that. > > I feel like we have a simple solution to a simple problem, and if we really need > a more advanced solution (which I'm not sure if we will ever need) we can > implement that later on. Maybe, but I really don't think it is harder to move the colors to the python code. Attaching one possibility as comments.
https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... File webrtc/rtc_tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:91: // IntervalSeries printf("interval_colors = ['#ff8e82','#5092fc','#c4ffc4']\n"); RTC_CHECK_LE(interval_list.size(), 3); https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:109: printf( printf("plt.axhspan(ival%zu[i][0], ival%zu[i][1], facecolor=interval_colors[%zu], alpha=0.3)\n", i, i, i); https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:118: } Is there some label that helps the user understand the meaning of the different colors? (You're the only one that knows your convention.) Documentation and example here: https://matplotlib.org/users/legend_guide.html
https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... File webrtc/rtc_tools/event_log_visualizer/plot_python.cc (right): https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:91: // IntervalSeries On 2017/07/11 21:35:52, terelius wrote: > printf("interval_colors = ['#ff8e82','#5092fc','#c4ffc4']\n"); > RTC_CHECK_LE(interval_list.size(), 3); Done. https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:109: printf( On 2017/07/11 21:35:52, terelius wrote: > printf("plt.axhspan(ival%zu[i][0], ival%zu[i][1], > facecolor=interval_colors[%zu], alpha=0.3)\n", i, i, i); Done. https://codereview.webrtc.org/2826313004/diff/140001/webrtc/rtc_tools/event_l... webrtc/rtc_tools/event_log_visualizer/plot_python.cc:118: } On 2017/07/11 21:35:52, terelius wrote: > Is there some label that helps the user understand the meaning of the different > colors? (You're the only one that knows your convention.) > > Documentation and example here: https://matplotlib.org/users/legend_guide.html Done.
lgtm Thanks!
The CQ bit was checked by philipel@webrtc.org
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": 160001, "attempt_start_ts": 1500036583917240,
"parent_rev": "9b1367f2337d147ca7558ab11b7f2a510df12fd7", "commit_rev":
"23c7f2526621ae9283e694a7c329780713dafb25"}
Message was sent while issue was closed.
Description was changed from ========== Added -show_detector_state which show the detector state in the total bitrate graph. BUG=none ========== to ========== Added -show_detector_state which show the detector state in the total bitrate graph. BUG=none Review-Url: https://codereview.webrtc.org/2826313004 Cr-Commit-Position: refs/heads/master@{#19020} Committed: https://chromium.googlesource.com/external/webrtc/+/23c7f2526621ae9283e694a7c... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/23c7f2526621ae9283e694a7c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
