Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

The new format for plot lines is PLOT <plot_no> <var_name>:<ssrc>@<alg_name> <time> <value>. The var_name is no longer prefixed by the context/tag (which most of the time was just the same as the test name.)

Update plot_dynamics.py script (which didn't work) to visualize the new BWE_TEST_LOGGING_PLOT lines.

BUG=webrtc:6621
Committed: https://crrev.com/f94ba462bc80ce8adfc566b1fc56d8a66c7fcfec
Cr-Commit-Position: refs/heads/master@{#14983}

[terelius, 2016-10-28 14:18:28]

terelius@webrtc.org changed reviewers:
+ gaetano.carlucci@gmail.com, stefan@webrtc.org

[terelius, 2016-10-28 14:18:29]

Please take a look.

[stefan-webrtc, 2016-10-31 10:41:41]

Should we also delete plot_dynamics.sh?

[Gaetano Carlucci, 2016-10-31 12:10:59]

From a rapid look the log format looks better now. Thanks!

It seems that the ninja target for the simulations now is bwe_simulator. Is it
documented somewhere?

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py (right):

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:31: raise
ParsePlotLineException("Expected 5 aegumesnts on line", line)
aegumesnts ->  arguments

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:110:
receiver.addSubplot("Time (s)", "One-way Delay (ms)", ['Delay_ms'])
Delay_ms -> Delay_ms_
It seems that an extra _ is added. Could you please check that?

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:111:
receiver.addSubplot("Time (s)", "Packet Loss Ratio", ['Packet_Loss'])
Packet_Loss -> Packet_Loss_

[terelius, 2016-11-02 09:37:16]

Was the old BWE simulator documented anywhere publicly? If so, I agree that it
needs to be updated now that the simulator is it's own binary.

Removed plot_dynamics.sh

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py (right):

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:31: raise
ParsePlotLineException("Expected 5 aegumesnts on line", line)
On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> aegumesnts ->  arguments

Done.

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:110:
receiver.addSubplot("Time (s)", "One-way Delay (ms)", ['Delay_ms'])
On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> Delay_ms -> Delay_ms_
> It seems that an extra _ is added. Could you please check that?

There are actually two variables, one with underscore and one without. Both
appear to be used, but in different circumstances. (Delay_ms is used in the BWE
simulations.) Since I don't know what the difference is, I'll plot both for the
time being.

https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:111:
receiver.addSubplot("Time (s)", "Packet Loss Ratio", ['Packet_Loss'])
On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> Packet_Loss -> Packet_Loss_

Done.

[Gaetano Carlucci, 2016-11-02 17:55:34]

On 2016/11/02 09:37:16, terelius wrote:
> Was the old BWE simulator documented anywhere publicly? If so, I agree that it
> needs to be updated now that the simulator is it's own binary.
> 
> Removed plot_dynamics.sh
> 
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py (right):
> 
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:31: raise
> ParsePlotLineException("Expected 5 aegumesnts on line", line)
> On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> > aegumesnts ->  arguments
> 
> Done.
> 
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:110:
> receiver.addSubplot("Time (s)", "One-way Delay (ms)", ['Delay_ms'])
> On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> > Delay_ms -> Delay_ms_
> > It seems that an extra _ is added. Could you please check that?
> 
> There are actually two variables, one with underscore and one without. Both
> appear to be used, but in different circumstances. (Delay_ms is used in the
BWE
> simulations.) Since I don't know what the difference is, I'll plot both for
the
> time being.
> 
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:111:
> receiver.addSubplot("Time (s)", "Packet Loss Ratio", ['Packet_Loss'])
> On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> > Packet_Loss -> Packet_Loss_
> 
> Done.

I am not aware of a public document for the simulator. In the case exists it
should be documented.
I think that the extra _ should be addressed in metric_recorder.cc.

Probably in the new plot_dynamics.py we do not distinguish concurrent flows. 
For example, if we run:

./bwe_simulator --gtest_filter=*BweSimulation.Evaluation4/1: 

it seems that the py script aggregates all the concurrent flows in one. Could
you please check this?
Thanks

[terelius, 2016-11-07 08:13:22]

On 2016/11/02 17:55:34, Gaetano Carlucci wrote:
> On 2016/11/02 09:37:16, terelius wrote:
> > Was the old BWE simulator documented anywhere publicly? If so, I agree that
it
> > needs to be updated now that the simulator is it's own binary.
> > 
> > Removed plot_dynamics.sh
> > 
> >
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> > File webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py (right):
> > 
> >
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> > webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:31: raise
> > ParsePlotLineException("Expected 5 aegumesnts on line", line)
> > On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> > > aegumesnts ->  arguments
> > 
> > Done.
> > 
> >
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> > webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:110:
> > receiver.addSubplot("Time (s)", "One-way Delay (ms)", ['Delay_ms'])
> > On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> > > Delay_ms -> Delay_ms_
> > > It seems that an extra _ is added. Could you please check that?
> > 
> > There are actually two variables, one with underscore and one without. Both
> > appear to be used, but in different circumstances. (Delay_ms is used in the
> BWE
> > simulations.) Since I don't know what the difference is, I'll plot both for
> the
> > time being.
> > 
> >
>
https://codereview.webrtc.org/2456373002/diff/1/webrtc/modules/remote_bitrate...
> > webrtc/modules/remote_bitrate_estimator/test/plot_dynamics.py:111:
> > receiver.addSubplot("Time (s)", "Packet Loss Ratio", ['Packet_Loss'])
> > On 2016/10/31 12:10:58, Gaetano Carlucci wrote:
> > > Packet_Loss -> Packet_Loss_
> > 
> > Done.
> 
> I am not aware of a public document for the simulator. In the case exists it
> should be documented.
> I think that the extra _ should be addressed in metric_recorder.cc.
> 
> Probably in the new plot_dynamics.py we do not distinguish concurrent flows. 
> For example, if we run:
> 
> ./bwe_simulator --gtest_filter=*BweSimulation.Evaluation4/1: 
> 
> it seems that the py script aggregates all the concurrent flows in one. Could
> you please check this?
> Thanks

I printed the flow id as an SSRC. (I'm assuming that you don't need both
multiple flows and multiple ssrcs at the same time, right?) Also fixed some
strange behavior caused by several plot statements that were outputting the same
variable name.

Please take another look.

[terelius, 2016-11-08 14:01:51]

ping

It would be very helpful if we could land some version of this script. Otherwise
I have to rebase every branch on top of this, and redo the work every time I
pull in new changes from master.

[stefan-webrtc, 2016-11-08 14:21:43]

Does the fairness plots work with this now, so that we plot the throughput of
every flow?

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

https://codereview.webrtc.org/2456373002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:360: // it
hasn't been kept up to data with the plot format. Remove or fix?
up to date

[Gaetano Carlucci, 2016-11-08 14:55:25]

On 2016/11/08 14:21:43, stefan-webrtc (holmer) wrote:
> Does the fairness plots work with this now, so that we plot the throughput of
> every flow?
> 
>
https://codereview.webrtc.org/2456373002/diff/80001/webrtc/modules/remote_bit...
> File webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc (right):
> 
>
https://codereview.webrtc.org/2456373002/diff/80001/webrtc/modules/remote_bit...
> webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:360: // it
> hasn't been kept up to data with the plot format. Remove or fix?
> up to date

I have run some tests. Some results here
https://drive.google.com/open?id=0Bxpn5mZJzQcJelc5aFJXMU5IMmc
Overall it looks good to me!

As a further improvement, we should consider that some variables, i.e. gamma,
offset... belong to a particular flow.

[terelius, 2016-11-08 15:04:43]

On 2016/11/08 14:21:43, stefan-webrtc (holmer) wrote:
> Does the fairness plots work with this now, so that we plot the throughput of
> every flow?
> 

Yes. In some cases it even plots the throughput of each flow twice. In fact, it
plots so many flows that you can't see the throughput because the entire window
is covered by the labels.

[Gaetano Carlucci, 2016-11-08 15:11:50]

lgtm

The fact that plots the throughput of each flow twice depends on the variables
that are selected in ploy_dynamics.p. 

It should be ok.

[stefan-webrtc, 2016-11-08 15:20:15]

lgtm

[terelius, 2016-11-08 16:06:35]

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

https://codereview.webrtc.org/2456373002/diff/80001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/metric_recorder.cc:360: // it
hasn't been kept up to data with the plot format. Remove or fix?
On 2016/11/08 14:21:43, stefan-webrtc (holmer) wrote:
> up to date

Done.

[terelius, 2016-11-08 16:07:23]

Description was changed from

==========
Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

The new format for plot lines is PLOT <plot_no> <var_name>:<ssrc>@<alg_name>
<time> <value>. The var_name no longer is no longer prefixed by the context/tag
(which most of the time was just the same as the test name.)

Update plot_dynamics.py script (which didn't work) to visualize the new
BWE_TEST_LOGGING_PLOT lines.

BUG=webrtc:6621
==========

to

==========
Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

The new format for plot lines is PLOT <plot_no> <var_name>:<ssrc>@<alg_name>
<time> <value>. The var_name is no longer prefixed by the context/tag (which
most of the time was just the same as the test name.)

Update plot_dynamics.py script (which didn't work) to visualize the new
BWE_TEST_LOGGING_PLOT lines.

BUG=webrtc:6621
==========

[terelius, 2016-11-08 16:07:34]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 16:07:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-08 16:09:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 16:09:53]

Dry run: Try jobs failed on following builders:
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12312)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14651)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19882)
  linux_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/19977)

[terelius, 2016-11-08 17:25:04]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 17:25:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-08 17:47:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 17:47:03]

Dry run: This issue passed the CQ dry run.

[terelius, 2016-11-08 17:48:55]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-11-08 17:48:55]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
gaetano.carlucci@gmail.com
Link to the patchset: https://codereview.webrtc.org/2456373002/#ps120001 (title:
"Suppress warnings about unused variable.")

[commit-bot: I haz the power, 2016-11-08 17:49:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-08 17:52:02]

Description was changed from

==========
Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

The new format for plot lines is PLOT <plot_no> <var_name>:<ssrc>@<alg_name>
<time> <value>. The var_name is no longer prefixed by the context/tag (which
most of the time was just the same as the test name.)

Update plot_dynamics.py script (which didn't work) to visualize the new
BWE_TEST_LOGGING_PLOT lines.

BUG=webrtc:6621
==========

to

==========
Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

The new format for plot lines is PLOT <plot_no> <var_name>:<ssrc>@<alg_name>
<time> <value>. The var_name is no longer prefixed by the context/tag (which
most of the time was just the same as the test name.)

Update plot_dynamics.py script (which didn't work) to visualize the new
BWE_TEST_LOGGING_PLOT lines.

BUG=webrtc:6621
==========

[commit-bot: I haz the power, 2016-11-08 17:52:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-08 18:07:39]

Description was changed from

==========
Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

The new format for plot lines is PLOT <plot_no> <var_name>:<ssrc>@<alg_name>
<time> <value>. The var_name is no longer prefixed by the context/tag (which
most of the time was just the same as the test name.)

Update plot_dynamics.py script (which didn't work) to visualize the new
BWE_TEST_LOGGING_PLOT lines.

BUG=webrtc:6621
==========

to

==========
Update BWE_TEST_LOGGING_PLOT output format, and fix plot_dynamics.py script.

The new format for plot lines is PLOT <plot_no> <var_name>:<ssrc>@<alg_name>
<time> <value>. The var_name is no longer prefixed by the context/tag (which
most of the time was just the same as the test name.)

Update plot_dynamics.py script (which didn't work) to visualize the new
BWE_TEST_LOGGING_PLOT lines.

BUG=webrtc:6621

Committed: https://crrev.com/f94ba462bc80ce8adfc566b1fc56d8a66c7fcfec
Cr-Commit-Position: refs/heads/master@{#14983}
==========

[commit-bot: I haz the power, 2016-11-08 18:07:40]

Patchset 7 (id:??) landed as
https://crrev.com/f94ba462bc80ce8adfc566b1fc56d8a66c7fcfec
Cr-Commit-Position: refs/heads/master@{#14983}