BWE Simulation Framework: Standard plot logging

BWE Simulation Framework: Standard plot logging

-- Made plot logging from MetricRecorder and from RateCounterFilter/PacketReceiver standard to fit python and shell plotting scripts likewise.

-- RateCounterFilter is initialized with algorithm name.
-- Removed spare commas and duplicated flow ids from RateCounterFilter name.
-- Added optional plot_delay and plot_loss in MetricRecorder.
-- PacketReceiver can plot directly plot delay if there is no metric_recorder_.
-- Added comments to plot scripts.

R=stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/d55ce2ddb9eb67e237725820b751f404df6dba0d

[magalhaesc, 2015-07-23 19:00:09]

magalhaesc@google.com changed reviewers:
+ stefan@webrtc.org

[magalhaesc, 2015-07-23 19:03:09]

Added missing comma

[magalhaesc, 2015-07-23 19:10:51]

Standard plot logging - matching python and shell scripts

[magalhaesc, 2015-07-23 19:10:52]

Standard plot logging - matching python and shell scripts

[magalhaesc, 2015-07-23 19:13:19]

Check why plot_bars.sh is empty

[stefan-webrtc, 2015-07-24 09:30:43]

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:500: bwe_type, false,
false,
Who is responsible for plotting delays in this test now?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:271:
name_(std::string(name) + "_A"),  // All flow ids.
I don't fully understand this. What if a RateCounterFilter is created only to
cover a subset of flows?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:304:
BWE_TEST_LOGGING_PLOT(0, "Throughput_kbps#2", timestamp_ms, plot_kbps);
Isn't #1 the bitrate axis and #2 the delay axis?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:38:
delay_prefix_("Delay_ms#d") {
What is #d?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:43:
prefixes.push_back("Throughput_kbps#2");      // Throughput.
I find it odd that this is on #2 and not #1. Could you elaborate? I kind of
would like to keep the flow_id in the name as well if possible, so that it's
easy to see what flow has what rate. We can discuss if this is hard to get
right.

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:44:
prefixes.push_back("Delay_ms_#2");            // Delay.
Should Delay_ms_#2 be the same format as Delay_ms#d?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.sh (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.sh:44: if ((
"${#delay_line}" > "0" )); then
Previously, this checked if we had more than a single y-axis, that is, if
linetypes contained only "1", that meant a single axis, while if it contained
both "1" and "2" (or was it 0 and 1?) we had two axis, which we assumed to be
bitrate and delay. How has this been changed now?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.sh:63: echo -n "axes
x1y2 "
Previously, this checked if we had more than a single y-axis, that is, if
linetypes contained only "1", that meant a single axis, while if it contained
both "1" and "2" (or was it 0 and 1?) we had two axis, which we assumed to be
bitrate and delay.

Are you now proposing to have #d for delay? Should we in that case have #b for
bitrate?

[magalhaesc, 2015-07-24 11:46:40]

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:500: bwe_type, false,
false,
On 2015/07/24 09:30:42, stefan-webrtc (holmer) wrote:
> Who is responsible for plotting delays in this test now?

The delay is being plotted by the metric_recorder.
I will fix this since the metric_recorder is actually called by the
packet_receiver

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:271:
name_(std::string(name) + "_A"),  // All flow ids.
On 2015/07/24 09:30:42, stefan-webrtc (holmer) wrote:
> I don't fully understand this. What if a RateCounterFilter is created only to
> cover a subset of flows?

Right, I didn't consider that.
Replaced back with all flow ids.

[magalhaesc, 2015-07-24 13:50:33]

Comments addressed

[magalhaesc, 2015-07-24 13:51:40]

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc:304:
BWE_TEST_LOGGING_PLOT(0, "Throughput_kbps#2", timestamp_ms, plot_kbps);
On 2015/07/24 09:30:42, stefan-webrtc (holmer) wrote:
> Isn't #1 the bitrate axis and #2 the delay axis?

Yes.
I was using d for delay, will go back to the standard:
-- 1 if the alignment is with the left axis.
-- 2 if the alignment is with the right axis.

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:38:
delay_prefix_("Delay_ms#d") {
On 2015/07/24 09:30:43, stefan-webrtc (holmer) wrote:
> What is #d?

Replaced with #2, alignment with the right axis.

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:43:
prefixes.push_back("Throughput_kbps#2");      // Throughput.
On 2015/07/24 09:30:42, stefan-webrtc (holmer) wrote:
> I find it odd that this is on #2 and not #1. Could you elaborate? I kind of
> would like to keep the flow_id in the name as well if possible, so that it's
> easy to see what flow has what rate. We can discuss if this is hard to get
> right.

Done, Adopting #1 for left axis alignment.

The flow_id was being printed twice in every line. So now we kept the
BWE_TEST_LOGGING_CONTEXT(*flow_ids().begin());

What was Receiver_FLOWID_Throughput_FLOWID#1
is now Receiver_FLOWID_Throughput#1

Is that ok?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:44:
prefixes.push_back("Delay_ms_#2");            // Delay.
On 2015/07/24 09:30:42, stefan-webrtc (holmer) wrote:
> Should Delay_ms_#2 be the same format as Delay_ms#d?

The metric recorder uses separate figure_id for each plot (which turns out into
separate boxes in plot_dynamics.py and separated windows in plot_dynamics.sh).

In this case we will need only one axis label, so the delay will be alignment
with the left side.

WDYT?

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.sh (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.sh:44: if ((
"${#delay_line}" > "0" )); then
On 2015/07/24 09:30:43, stefan-webrtc (holmer) wrote:
> Previously, this checked if we had more than a single y-axis, that is, if
> linetypes contained only "1", that meant a single axis, while if it contained
> both "1" and "2" (or was it 0 and 1?) we had two axis, which we assumed to be
> bitrate and delay. How has this been changed now?

Adopting back the convention:
1 - left side axis
2 - right side axis

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.sh:63: echo -n "axes
x1y2 "
On 2015/07/24 09:30:43, stefan-webrtc (holmer) wrote:
> Previously, this checked if we had more than a single y-axis, that is, if
> linetypes contained only "1", that meant a single axis, while if it contained
> both "1" and "2" (or was it 0 and 1?) we had two axis, which we assumed to be
> bitrate and delay.
> 
> Are you now proposing to have #d for delay? Should we in that case have #b for
> bitrate?

Adopting back the convention:
1 - left side axis
2 - right side axis

[magalhaesc, 2015-07-24 13:52:01]

Thanks, comments addressed

[stefan-webrtc, 2015-07-24 14:06:55]

lgtm

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc (right):

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:43:
prefixes.push_back("Throughput_kbps#2");      // Throughput.
On 2015/07/24 13:51:40, magalhaesc wrote:
> On 2015/07/24 09:30:42, stefan-webrtc (holmer) wrote:
> > I find it odd that this is on #2 and not #1. Could you elaborate? I kind of
> > would like to keep the flow_id in the name as well if possible, so that it's
> > easy to see what flow has what rate. We can discuss if this is hard to get
> > right.
> 
> Done, Adopting #1 for left axis alignment.
> 
> The flow_id was being printed twice in every line. So now we kept the
> BWE_TEST_LOGGING_CONTEXT(*flow_ids().begin());
> 
> What was Receiver_FLOWID_Throughput_FLOWID#1
> is now Receiver_FLOWID_Throughput#1
> 
> Is that ok?

Sounds good

https://codereview.webrtc.org/1253473004/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet_receiver.cc:44:
prefixes.push_back("Delay_ms_#2");            // Delay.
On 2015/07/24 13:51:40, magalhaesc wrote:
> On 2015/07/24 09:30:42, stefan-webrtc (holmer) wrote:
> > Should Delay_ms_#2 be the same format as Delay_ms#d?
> 
> The metric recorder uses separate figure_id for each plot (which turns out
into
> separate boxes in plot_dynamics.py and separated windows in plot_dynamics.sh).
> 
> In this case we will need only one axis label, so the delay will be alignment
> with the left side.
> 
> WDYT?

Sounds good.

https://codereview.webrtc.org/1253473004/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc (right):

https://codereview.webrtc.org/1253473004/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:134: bool
plot_delay) {
This should probably be called plot_loss_and_delay

https://codereview.webrtc.org/1253473004/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:153: case kLoss:
 // Plot loss for flows of which delay is ploted.
plotted

[magalhaesc, 2015-07-25 22:22:37]

Comments addressed[2]

[magalhaesc, 2015-07-25 22:27:40]

Comments addressed [2]

[magalhaesc, 2015-07-25 22:27:48]

Patchset #4 (id:60001) has been deleted

[magalhaesc, 2015-07-25 22:35:31]

https://codereview.webrtc.org/1253473004/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc (right):

https://codereview.webrtc.org/1253473004/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:134: bool
plot_delay) {
On 2015/07/24 14:06:55, stefan-webrtc (holmer) wrote:
> This should probably be called plot_loss_and_delay

Done.

https://codereview.webrtc.org/1253473004/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:153: case kLoss:
 // Plot loss for flows of which delay is ploted.
On 2015/07/24 14:06:55, stefan-webrtc (holmer) wrote:
> plotted

Done.

[magalhaesc, 2015-07-25 23:45:14]

Committed patchset #4 (id:80001) manually as
d55ce2ddb9eb67e237725820b751f404df6dba0d.