Add loss-based BWE estimate to the outgoing bitrate plot.

Add loss-based BWE estimate to the outgoing bitrate plot.

Committed: https://crrev.com/8058e58d8fb35b4aba8330d19ffb0e0ef4b9d0cf
Cr-Commit-Position: refs/heads/master@{#13517}

[terelius, 2016-07-19 09:13:37]

terelius@webrtc.org changed reviewers:
+ ivoc@webrtc.org, stefan@webrtc.org

[terelius, 2016-07-19 09:13:37]

Please take a look.

[ivoc, 2016-07-19 09:42:48]

LGTM, see some minor remarks below.

https://codereview.webrtc.org/2165523002/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2165523002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:213:
bwe_loss_updates_.push_back(BwePacketLossEvent(
You could use emplace_back here to construct the object directly within the
container.

https://codereview.webrtc.org/2165523002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:576:
plot->series.back().points.push_back(TimeSeriesPoint(x, y));
Another opportunity for emplace_back.

[stefan-webrtc, 2016-07-19 09:49:34]

lgtm

One thing that you may want to address at somepoint is to consider the
testability of this. Could you perhaps send in a test proto and verify that the
output python code is as expected?

[terelius, 2016-07-19 13:06:18]

https://codereview.webrtc.org/2165523002/diff/1/webrtc/tools/event_log_visual...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2165523002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:213:
bwe_loss_updates_.push_back(BwePacketLossEvent(
On 2016/07/19 09:42:48, ivoc wrote:
> You could use emplace_back here to construct the object directly within the
> container.

Changed the GetBwePacketLossEvent to write the directly to the
BwePacketLossEvent object instead. While it doesn't use emplace_back, I think
this is about as efficient and it is a few lines shorter. Wdyt?

https://codereview.webrtc.org/2165523002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:576:
plot->series.back().points.push_back(TimeSeriesPoint(x, y));
On 2016/07/19 09:42:48, ivoc wrote:
> Another opportunity for emplace_back.

Done.

[terelius, 2016-07-19 13:16:45]

On 2016/07/19 09:49:34, stefan-webrtc (holmer) wrote:
> lgtm
> 
> One thing that you may want to address at somepoint is to consider the
> testability of this. Could you perhaps send in a test proto and verify that
the
> output python code is as expected?

Doing a naive text comparison of the python file would give a lot of false
positives. Almost any change to the analysis tool would be expected to change
the python output in some way.

It might be better to store the expected data series in a file with one file per
plot. Then we could import the data series and compare with the generated
TimeSeries objects. That way we could have one test per plot, so it would be
easier to spot if a change accidentally affects other plots than the intended
one.

[stefan-webrtc, 2016-07-19 13:22:32]

On 2016/07/19 13:16:45, terelius wrote:
> On 2016/07/19 09:49:34, stefan-webrtc (holmer) wrote:
> > lgtm
> > 
> > One thing that you may want to address at somepoint is to consider the
> > testability of this. Could you perhaps send in a test proto and verify that
> the
> > output python code is as expected?
> 
> Doing a naive text comparison of the python file would give a lot of false
> positives. Almost any change to the analysis tool would be expected to change
> the python output in some way.
> 
> It might be better to store the expected data series in a file with one file
per
> plot. Then we could import the data series and compare with the generated
> TimeSeries objects. That way we could have one test per plot, so it would be
> easier to spot if a change accidentally affects other plots than the intended
> one.

Sounds better, yes. I think it'd be enough with a few data points per time
series we test, so we may not even need to store them in separate files.

[terelius, 2016-07-20 09:16:05]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-07-20 09:16:05]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2165523002/#ps20001 (title:
"Use emplace_back")

[commit-bot: I haz the power, 2016-07-20 09:16:13]

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-07-20 09:19:04]

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

[commit-bot: I haz the power, 2016-07-20 09:19:05]

Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/12750)

[terelius, 2016-07-20 09:31:08]

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

[commit-bot: I haz the power, 2016-07-20 09:31:32]

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-07-20 10:31:55]

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

[commit-bot: I haz the power, 2016-07-20 10:31:55]

Dry run: Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/15058)

[terelius, 2016-07-21 07:52:37]

terelius@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[terelius, 2016-07-21 07:53:07]

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

[commit-bot: I haz the power, 2016-07-21 07:53:14]

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

[phoglund, 2016-07-21 08:02:42]

rs lgtm

[commit-bot: I haz the power, 2016-07-21 09:55:30]

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

[commit-bot: I haz the power, 2016-07-21 09:55:31]

Dry run: Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[terelius, 2016-07-21 12:03:20]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-07-21 12:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2165523002/#ps40001 (title:
"Use default constructor for POD type.")

[commit-bot: I haz the power, 2016-07-21 12:03:28]

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-07-21 12:11:57]

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

[commit-bot: I haz the power, 2016-07-21 12:11:58]

Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[terelius, 2016-07-25 07:53:36]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2016-07-25 07:53:39]

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-07-25 08:32:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-25 08:38:01]

Description was changed from

==========
Add loss-based BWE estimate to the outgoing bitrate plot.
==========

to

==========
Add loss-based BWE estimate to the outgoing bitrate plot.

Committed: https://crrev.com/8058e58d8fb35b4aba8330d19ffb0e0ef4b9d0cf
Cr-Commit-Position: refs/heads/master@{#13517}
==========

[commit-bot: I haz the power, 2016-07-25 08:38:02]

Patchset 3 (id:??) landed as
https://crrev.com/8058e58d8fb35b4aba8330d19ffb0e0ef4b9d0cf
Cr-Commit-Position: refs/heads/master@{#13517}