Add probe logging to RtcEventLog.

Add probe logging to RtcEventLog.

In this CL:
 - Add message BweProbeCluster and BweProbeResult to rtc_event_log.proto.
 - Add corresponding log functions to RtcEventLog.
 - Add optional field |probe_cluster_id| to RtpPacket message and added
   an overload function to log with this information.
 - Propagate the probe_cluster_id to where RTP packets are logged.

BUG=webrtc:6984
Review-Url: https://codereview.webrtc.org/2666533002
Cr-Commit-Position: refs/heads/master@{#16857}
Committed: https://chromium.googlesource.com/external/webrtc/+/32d0010d8650060aae384106f23cd3593b1a320c

[philipel, 2017-01-30 10:47:32]

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

[terelius, 2017-02-01 13:38:19]

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log.cc (right):

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:72: void
LogRtpHeader(PacketDirection direction,
Assuming we always log outgoing packets by calling this function, could we
rename the two functions to LogIncomingRtpHeader and LogOutgoingRtpHeader and
drop the PacketDirection?

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:488: void
RtcEventLogImpl::LogProbeClusterCreated(int id,
Please add unit test.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:504: void
RtcEventLogImpl::LogProbeResultSuccess(int id, int bitrate_bps) {
Please add unit test.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:508: void
RtcEventLogImpl::LogProbeResultFailure(int id,
Please add unit test.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.h:109: // Save as above but used on
the senderside to log packets that is part of
nit: senderside -> sender side,  is -> are

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.h:149: // virtual void
LogProbeResult() = 0;
This declaration doesn't seem to match any implementation in the cc file. (The
implementation takes 3 arguments.)

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.h:149: // virtual void
LogProbeResult() = 0;
Assuming that people are meant to call LogProbeResultSuccess or
LogProbeResultFailure, this helper function should be private and probably not a
part of the interface.

[philipel, 2017-02-17 15:15:02]

PTAL

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log.cc (right):

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:72: void
LogRtpHeader(PacketDirection direction,
On 2017/02/01 13:38:19, terelius wrote:
> Assuming we always log outgoing packets by calling this function, could we
> rename the two functions to LogIncomingRtpHeader and LogOutgoingRtpHeader and
> drop the PacketDirection?

We could do that, but we will still log outgoing packets that don't have a
cluster id (RTX for example), but I guess you could just use the constant
PacketInfo::kNotAProbe in that case.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:488: void
RtcEventLogImpl::LogProbeClusterCreated(int id,
On 2017/02/01 13:38:19, terelius wrote:
> Please add unit test.

Done.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:504: void
RtcEventLogImpl::LogProbeResultSuccess(int id, int bitrate_bps) {
On 2017/02/01 13:38:19, terelius wrote:
> Please add unit test.

Done.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:508: void
RtcEventLogImpl::LogProbeResultFailure(int id,
On 2017/02/01 13:38:19, terelius wrote:
> Please add unit test.

Done.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.h:109: // Save as above but used on
the senderside to log packets that is part of
On 2017/02/01 13:38:19, terelius wrote:
> nit: senderside -> sender side,  is -> are

Done.

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.h:149: // virtual void
LogProbeResult() = 0;
On 2017/02/01 13:38:19, terelius wrote:
> Assuming that people are meant to call LogProbeResultSuccess or
> LogProbeResultFailure, this helper function should be private and probably not
a
> part of the interface.

This was a leftover as I outlined which functions I wanted to implement,
removed.

[terelius, 2017-02-17 15:21:47]

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

https://codereview.webrtc.org/2666533002/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.proto:274: // required - The minimum
number of probes used to probe the given bitrate.
should this be packets?

[terelius, 2017-02-17 15:50:43]

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log.cc (right):

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:72: void
LogRtpHeader(PacketDirection direction,
On 2017/02/17 15:15:02, philipel wrote:
> On 2017/02/01 13:38:19, terelius wrote:
> > Assuming we always log outgoing packets by calling this function, could we
> > rename the two functions to LogIncomingRtpHeader and LogOutgoingRtpHeader
and
> > drop the PacketDirection?
> 
> We could do that, but we will still log outgoing packets that don't have a
> cluster id (RTX for example), but I guess you could just use the constant
> PacketInfo::kNotAProbe in that case.

As far as I can tell, all outgoing packets will have a cluster id with  this CL.
The only call to LogRtpHeader(kOutgoingPacket, ...) is in
RTPSender::SendPacketToNetwork.

[philipel, 2017-02-20 09:46:47]

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log.cc (right):

https://codereview.webrtc.org/2666533002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log.cc:72: void
LogRtpHeader(PacketDirection direction,
On 2017/02/17 15:50:43, terelius wrote:
> On 2017/02/17 15:15:02, philipel wrote:
> > On 2017/02/01 13:38:19, terelius wrote:
> > > Assuming we always log outgoing packets by calling this function, could we
> > > rename the two functions to LogIncomingRtpHeader and LogOutgoingRtpHeader
> and
> > > drop the PacketDirection?
> > 
> > We could do that, but we will still log outgoing packets that don't have a
> > cluster id (RTX for example), but I guess you could just use the constant
> > PacketInfo::kNotAProbe in that case.
> 
> As far as I can tell, all outgoing packets will have a cluster id with  this
CL.
> The only call to LogRtpHeader(kOutgoingPacket, ...) is in
> RTPSender::SendPacketToNetwork. 

If we want to do this change I think we should do it in a separate CL.

[terelius, 2017-02-20 12:47:55]

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

https://codereview.webrtc.org/2666533002/diff/40001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.proto:275: optional uint32 min_probes
= 3;
probes -> packets here too?

[philipel, 2017-02-20 12:54:31]

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

https://codereview.webrtc.org/2666533002/diff/40001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.proto:275: optional uint32 min_probes
= 3;
On 2017/02/20 12:47:55, terelius wrote:
> probes -> packets here too?

Done.

[terelius, 2017-02-20 14:00:31]

Renaming things in the event log is hard. :-/

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h (right):

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h:72: void(int id, int
bitrate_bps, int min_probes, int min_bytes));
probes -> packets

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log.cc (right):

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.cc:89: int min_probes,
probes -> packets

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

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.h:138: int min_probes,
probes -> packets

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log.h:198: int min_probes,
probes -> packets

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc:509: int min_probes0 =
prng.Rand(0, 100);
probes -> packets

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.cc (right):

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.cc:528: uint32_t
min_probes,
probes -> packets

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.h (right):

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.h:73: uint32_t
min_probes,
probes -> packets

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2666533002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:162: int min_probes,
probes -> packets

[philipel, 2017-02-20 14:15:00]

The problem is that "probes" has been used throughout the rest of the code to
refer to a packet that is part of a probing cluster.

It is something I would like to change, but in a separate refactoring CL.

Sorry, should have told you earlier :P

[terelius, 2017-02-21 12:01:53]

lgtm

[philipel, 2017-02-21 12:02:31]

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

[commit-bot: I haz the power, 2017-02-21 12:02:43]

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

[philipel, 2017-02-21 12:04:43]

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

[philipel, 2017-02-21 12:04:43]

Stefan, PTAL in webrtc/rtp_rtcp and webrtc/voice_engine.

[commit-bot: I haz the power, 2017-02-21 12:05:15]

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

[commit-bot: I haz the power, 2017-02-21 12:05:16]

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...)

[philipel, 2017-02-21 12:54:34]

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

[commit-bot: I haz the power, 2017-02-21 12:54:38]

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-21 14:44:25]

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

[commit-bot: I haz the power, 2017-02-21 14:44:27]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-02-23 11:51:28]

ping

[philipel, 2017-02-24 09:58:02]

ping Stefan

[stefan-webrtc, 2017-02-24 12:15:35]

lgtm

[philipel, 2017-02-24 12:16:41]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-02-24 12:16:42]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2666533002/#ps80001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-02-24 12:16:48]

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-24 12:19:57]

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

[commit-bot: I haz the power, 2017-02-24 12:19:59]

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

[philipel, 2017-02-24 14:40:20]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-02-24 14:40:21]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2666533002/#ps100001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-02-24 14:40: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, 2017-02-24 14:43:57]

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

[commit-bot: I haz the power, 2017-02-24 14:43:58]

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

[philipel, 2017-02-24 14:58:09]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-02-24 14:58:10]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2666533002/#ps120001 (title:
"Rebase + format")

[commit-bot: I haz the power, 2017-02-24 14:58:19]

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-24 15:08:59]

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

[commit-bot: I haz the power, 2017-02-24 15:09:00]

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

[philipel, 2017-02-27 09:39:33]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2017-02-27 09:39:42]

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-27 10:18:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488188373222660,
"parent_rev": "f284b7ff5f265016203f9c4c3f21b22cedd7593e", "commit_rev":
"32d0010d8650060aae384106f23cd3593b1a320c"}

[commit-bot: I haz the power, 2017-02-27 10:18:49]

Description was changed from

==========
Add probe logging to RtcEventLog.

In this CL:
 - Add message BweProbeCluster and BweProbeResult to rtc_event_log.proto.
 - Add corresponding log functions to RtcEventLog.
 - Add optional field |probe_cluster_id| to RtpPacket message and added
   an overload function to log with this information.
 - Propagate the probe_cluster_id to where RTP packets are logged.

BUG=webrtc:6984
==========

to

==========
Add probe logging to RtcEventLog.

In this CL:
 - Add message BweProbeCluster and BweProbeResult to rtc_event_log.proto.
 - Add corresponding log functions to RtcEventLog.
 - Add optional field |probe_cluster_id| to RtpPacket message and added
   an overload function to log with this information.
 - Propagate the probe_cluster_id to where RTP packets are logged.

BUG=webrtc:6984

Review-Url: https://codereview.webrtc.org/2666533002
Cr-Commit-Position: refs/heads/master@{#16857}
Committed:
https://chromium.googlesource.com/external/webrtc/+/32d0010d8650060aae384106f...
==========

[commit-bot: I haz the power, 2017-02-27 10:18:51]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/32d0010d8650060aae384106f...