Added -show_detector_state which show the detector state in the total bitrate graph.

webrtc/tools/event_log_visualizer/analyzer.cc

Index: webrtc/tools/event_log_visualizer/analyzer.cc
diff --git a/webrtc/tools/event_log_visualizer/analyzer.cc b/webrtc/tools/event_log_visualizer/analyzer.cc
index 88ca33935f44a11a10afd67497cd7ef4f6a98fc5..94f02aa0518bd0c58e01e0db2e3fb0cf8b99b4c6 100644
--- a/webrtc/tools/event_log_visualizer/analyzer.cc
+++ b/webrtc/tools/event_log_visualizer/analyzer.cc
@@ -883,7 +883,8 @@ void EventLogAnalyzer::CreateFractionLossGraph(Plot* plot) {
 // Plot the total bandwidth used by all RTP streams.
 void EventLogAnalyzer::CreateTotalBitrateGraph(
     PacketDirection desired_direction,
-    Plot* plot) {
+    Plot* plot,
+    bool show_detector_state) {
   struct TimestampSize {
     TimestampSize(uint64_t t, size_t s) : timestamp(t), size(s) {}
     uint64_t timestamp;
@@ -943,14 +944,58 @@ void EventLogAnalyzer::CreateTotalBitrateGraph(
       loss_series.points.emplace_back(x, y);
     }
 
+    TimeSeries overusing_series("Overusing", VSPAN_GRAPH);
+    overusing_series.color = "#ff8e82";

[terelius, 2017/04/24 11:58:06]

Color seems like a UI choice. I'd  prefer to not include that in the TimeSeries
object.

[philipel, 2017/04/26 12:56:28]

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.

[terelius, 2017/04/26 13:40:09]

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?

[philipel, 2017/04/26 14:08:01]

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.

[terelius, 2017/04/27 11:30:59]

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.

+    TimeSeries underusing_series("Underusing", VSPAN_GRAPH);
+    underusing_series.color = "#5092fc";
+    TimeSeries normal_series("Normal", VSPAN_GRAPH);
+    normal_series.color = "#c4ffc4";
+    normal_series.points.emplace_back(0, 0);
     TimeSeries delay_series("Delay-based estimate", LINE_STEP_GRAPH);
+    TimeSeries* last_series = &normal_series;
+    BandwidthUsage detector_state = BandwidthUsage::kBwNormal;

[terelius, 2017/04/24 11:58:06]

last_detector_state to make it correspond to last_series?

[philipel, 2017/04/26 12:56:28]

Having a detector state makes things so much easier to detect when the state has
changed (line 963).

[terelius, 2017/04/26 13:40:09]

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.

[philipel, 2017/04/26 14:08:01]

Ah... Done :)

+
     for (auto& delay_update : bwe_delay_updates_) {
       float x =
           static_cast<float>(delay_update.timestamp - begin_time_) / 1000000;
       float y = static_cast<float>(delay_update.bitrate_bps) / 1000;
+
+      if (detector_state != delay_update.detector_state) {
+        RTC_CHECK(last_series);
+        last_series->points.emplace_back(x, 0);
+        last_series = nullptr;
+      }
+
+      switch (delay_update.detector_state) {
+        case BandwidthUsage::kBwNormal:
+          if (detector_state != BandwidthUsage::kBwNormal) {
+            normal_series.points.emplace_back(x, 0);
+            last_series = &normal_series;
+            detector_state = delay_update.detector_state;
+          }
+          break;
+        case BandwidthUsage::kBwUnderusing:
+          if (detector_state != BandwidthUsage::kBwUnderusing) {
+            underusing_series.points.emplace_back(x, 0);
+            last_series = &underusing_series;
+            detector_state = delay_update.detector_state;
+          }
+          break;
+        case BandwidthUsage::kBwOverusing:
+          if (detector_state != BandwidthUsage::kBwOverusing) {
+            overusing_series.points.emplace_back(x, 0);
+            last_series = &overusing_series;
+            detector_state = delay_update.detector_state;
+          }
+          break;
+      }
+
       delay_series.points.emplace_back(x, y);
     }
 
+    RTC_CHECK(last_series);
+    last_series->points.emplace_back(end_time_, 0);
+
     TimeSeries created_series("Probe cluster created.", DOT_GRAPH);
     for (auto& cluster : bwe_probe_cluster_created_events_) {
       float x = static_cast<float>(cluster.timestamp - begin_time_) / 1000000;
@@ -966,6 +1011,14 @@ void EventLogAnalyzer::CreateTotalBitrateGraph(
         result_series.points.emplace_back(x, y);
       }
     }
+
+    if (show_detector_state) {
+      plot->AppendTimeSeries(std::move(underusing_series));
+      plot->AppendTimeSeries(std::move(normal_series));
+      plot->AppendTimeSeries(std::move(overusing_series));
+    }
+
+    plot->AppendTimeSeries(std::move(bitrate_series));
     plot->AppendTimeSeries(std::move(loss_series));
     plot->AppendTimeSeries(std::move(delay_series));
     plot->AppendTimeSeries(std::move(created_series));