Add logging of delay-based bandwidth estimate.

Add logging of delay-based bandwidth estimate.

BUG=webrtc:6423
Review-Url: https://codereview.webrtc.org/2695923004
Cr-Commit-Position: refs/heads/master@{#16663}
Committed: https://chromium.googlesource.com/external/webrtc/+/0baf55d23b6986c210006393e0fb1f8564204291

[terelius, 2017-02-14 18:05:57]

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

[commit-bot: I haz the power, 2017-02-14 18:06:06]

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

[terelius, 2017-02-14 18:07:02]

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

[commit-bot: I haz the power, 2017-02-14 18:07:54]

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

[commit-bot: I haz the power, 2017-02-14 18:07:55]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17498)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17319)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22754)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21521)

[stefan-webrtc, 2017-02-14 18:13:21]

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:394: if (event_log_) {
Should we pass a null object instead?

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.h (right):

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.h:96: RtcEventLog*
event_log_;
const

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right):

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/transport_feedback_adapter.h:70:
RtcEventLog* event_log_;
const

[terelius, 2017-02-15 12:49:23]

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

[terelius, 2017-02-15 12:49:24]

+solenberg for voice_engine/

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:394: if (event_log_) {
On 2017/02/14 18:13:20, stefan-webrtc wrote:
> Should we pass a null object instead?

I don't know. We use a null implementation in some places, but the RPT and RTCP
senders explicitly check for null pointers. WDYT?

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.h (right):

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.h:96: RtcEventLog*
event_log_;
On 2017/02/14 18:13:20, stefan-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right):

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/transport_feedback_adapter.h:70:
RtcEventLog* event_log_;
On 2017/02/14 18:13:20, stefan-webrtc wrote:
> const

Done.

[stefan-webrtc, 2017-02-16 10:22:28]

lgtm

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2695923004/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/delay_based_bwe.cc:394: if (event_log_) {
On 2017/02/15 12:49:24, terelius wrote:
> On 2017/02/14 18:13:20, stefan-webrtc wrote:
> > Should we pass a null object instead?
> 
> I don't know. We use a null implementation in some places, but the RPT and
RTCP
> senders explicitly check for null pointers. WDYT?

I tend to prefer null objects, but I don't feel that strongly about it.

[philipel, 2017-02-16 10:37:56]

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.h:120: virtual void
LogBwePacketDelayEvent(int32_t bitrate,
Don't you think LogDelayBasedBweUpdateEvent might be a better name?

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log.proto (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.proto:131: optional int32 bitrate =
1;
bitrate_bps

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:395:
event_log_->LogBwePacketDelayEvent(result.target_bitrate_bps,
In the interest of saving space in the log, shouldn't we only log the detector
state if the detector state is different since last time we logged a change in
the delay based bwe?

[the sun, 2017-02-16 13:11:17]

lgtm for voice_engine/

[terelius, 2017-02-16 21:54:37]

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

[commit-bot: I haz the power, 2017-02-16 21:54:48]

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

[terelius, 2017-02-16 23:15:10]

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.h:120: virtual void
LogBwePacketDelayEvent(int32_t bitrate,
On 2017/02/16 10:37:56, philipel wrote:
> Don't you think LogDelayBasedBweUpdateEvent might be a better name?

Done. However, maintaining consistent names forced changes to ~100 lines across
14 different files, so I've broken it out to a separate CL.

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log.proto (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.proto:131: optional int32 bitrate =
1;
On 2017/02/16 10:37:56, philipel wrote:
> bitrate_bps

Done in separate CL: https://codereview.webrtc.org/2693213005/

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:395:
event_log_->LogBwePacketDelayEvent(result.target_bitrate_bps,
On 2017/02/16 10:37:56, philipel wrote:
> In the interest of saving space in the log, shouldn't we only log the detector
> state if the detector state is different since last time we logged a change in
> the delay based bwe?

Changed to only log update if bitrate or detector state has changed. I don't
think this makes a huge difference since we will always change the bitrate
except in application limited, or when underusing.

If you meant logging partial updates by omitting the detector state if that
hasn't changed, that would make it harder to parse the log since we would have
to handle missing data caused by the limited history buffer.

[terelius, 2017-02-16 23:16:40]

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

[commit-bot: I haz the power, 2017-02-16 23:16:43]

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, 2017-02-17 00:20:22]

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

[commit-bot: I haz the power, 2017-02-17 00:20:23]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-02-17 10:29:08]

lgtm

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.h:120: virtual void
LogBwePacketDelayEvent(int32_t bitrate,
On 2017/02/16 23:15:10, terelius wrote:
> On 2017/02/16 10:37:56, philipel wrote:
> > Don't you think LogDelayBasedBweUpdateEvent might be a better name?
> 
> Done. However, maintaining consistent names forced changes to ~100 lines
across
> 14 different files, so I've broken it out to a separate CL.

Acknowledged.

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe.cc (right):

https://codereview.webrtc.org/2695923004/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe.cc:395:
event_log_->LogBwePacketDelayEvent(result.target_bitrate_bps,
On 2017/02/16 23:15:10, terelius wrote:
> On 2017/02/16 10:37:56, philipel wrote:
> > In the interest of saving space in the log, shouldn't we only log the
detector
> > state if the detector state is different since last time we logged a change
in
> > the delay based bwe?
> 
> Changed to only log update if bitrate or detector state has changed. I don't
> think this makes a huge difference since we will always change the bitrate
> except in application limited, or when underusing.
> 
> If you meant logging partial updates by omitting the detector state if that
> hasn't changed, that would make it harder to parse the log since we would have
> to handle missing data caused by the limited history buffer.

I meant the partial update thing :)

[terelius, 2017-02-17 11:35:48]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2017-02-17 11:35:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2695923004/#ps60001 (title:
"Only log BWE update if bitrate or state has changed.")

[commit-bot: I haz the power, 2017-02-17 11:35:54]

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, 2017-02-17 11:38:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487331348769020,
"parent_rev": "9c997a3b9e33e24f3b23ae69024d7436660c364d", "commit_rev":
"0baf55d23b6986c210006393e0fb1f8564204291"}

[commit-bot: I haz the power, 2017-02-17 11:38:33]

Description was changed from

==========
Add logging of delay-based bandwidth estimate.

BUG=webrtc:6423
==========

to

==========
Add logging of delay-based bandwidth estimate.

BUG=webrtc:6423

Review-Url: https://codereview.webrtc.org/2695923004
Cr-Commit-Position: refs/heads/master@{#16663}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0baf55d23b6986c210006393e...
==========

[commit-bot: I haz the power, 2017-02-17 11:38:36]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/0baf55d23b6986c210006393e...