Add statistics gathering for packet loss.

Add statistics gathering for packet loss.

Adds a class used to classify whether packet loss events are a single packet or multiple packets as well as how many packets have been lost. Also exposes a new function in the RtpRtcp interface to retrieve these statistics.

BUG=

Committed: https://crrev.com/30409b4dca3d9cfdb0e714a5932b135becb0f822
Cr-Commit-Position: refs/heads/master@{#9568}

[bcornell, 2015-06-22 22:50:09]

bcornell@google.com changed reviewers:
+ mflodman@webrtc.org, noahric@chromium.org

[noahric, 2015-06-26 00:21:19]

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/inter...
File webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/inter...
webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h:357: struct
RtpPacketLossStats {
Comment for this struct and each field.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:13: static const int
kBufferSize = 100;
Comment or name to point out that this is the size at which automatic pruning
occurs.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:24: // Detect sequence
number wrap around.
There's another way of handling this where you have a wraparound counter that
you increment and use as the high halfword when adding a new packet. You still
need a check in this function to detect wraparound and the packets need to
arrive in order, but you could get rid of the complexities around the two
buffers (except on full wrap).

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:25: if (sequence_number <
0x4000 && !lost_packets_buffer_.empty()
If you lose every ~200 packets or so, this wraparound check would miss it. Maybe
that's unlikely in practice, but even if you don't do my comment from above
(about the wraparound counter), consider checking the distance from the high
sequence number in lost_packets_buffer_ instead of the lowest (i.e. something
with rbegin(), with some distance like 0x8000). I took a quick look through the
webrtc codebase to find how it's handled elsewhere, but I couldn't find
anything. Hopefully other reviewers will know if there's a generally accepted
algorithm for it.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:26: &&
*(lost_packets_buffer_.begin()) > 0xC000) {
&& at the end of the previous line

(you can probably just run 'git cl format' if you haven't yet)

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:64: return;
Consider DCHECK to ensure lost_packets_wrapped_buffer_ is also empty.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:69: for (int i = 0; i < 2;
++i) {
C++11 ftw:

for (auto buffer : { &lost_packets_buffer_, &lost_packets_wrapped_buffer_ }) {

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:71: uint16_t next_num = *it;
I'd say current_num, though I bet it wouldn't fit on line 72 :)

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:91: }
Is there a reasonable way to do some more thorough pruning here as well and move
it into PruneBuffer? Since we're already enumerating the full list, it seems
like you could prune everything but the last packet (group). Assuming it's fast
enough to do in the normal prune case (if it isn't, maybe you could prune more
often).

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:111: while
(!lost_packets_buffer_.empty()
Too bad you can't easily combine this with the block above :-/ Especially since
you could replace the block above with this version (that allows wrappping) and
it'd still work.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats.h (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.h:29: // Query the number of
packets that were lost by themselves, no neighboring
Queries (for the rest too)

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.h:39: // with the event count,
this can be used to determine how large events are.
I'd say explicitly "the average event size", if that's what you mean

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats_unittest.cc (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats_unittest.cc:36:
stats_.AddLostPacket(i & 0xFFFF);
Consider adding a CHECK or DCHECK in AddLostPacket to ensure the value fits in
the normal ssrc range. Otherwise, will weird things happen?

[harryjin, 2015-06-30 18:17:02]

harryjin@google.com changed reviewers:
+ harryjin@google.com

[harryjin, 2015-06-30 18:17:03]

https://codereview.webrtc.org/1198853004/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc (right):

https://codereview.webrtc.org/1198853004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:75: } else if
(sequential_count > 1) {
Just else is good enough? The else if condition is redundant.

[bcornell, 2015-06-30 19:47:46]

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/inter...
File webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/inter...
webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h:357: struct
RtpPacketLossStats {
On 2015/06/26 00:21:19, noahric wrote:
> Comment for this struct and each field.

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:13: static const int
kBufferSize = 100;
On 2015/06/26 00:21:19, noahric wrote:
> Comment or name to point out that this is the size at which automatic pruning
> occurs.

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:24: // Detect sequence
number wrap around.
On 2015/06/26 00:21:19, noahric wrote:
> There's another way of handling this where you have a wraparound counter that
> you increment and use as the high halfword when adding a new packet. You still
> need a check in this function to detect wraparound and the packets need to
> arrive in order, but you could get rid of the complexities around the two
> buffers (except on full wrap).

Indeed, I'm not sure how to handle that approach if the numbers don't come in
order. If we do want to bake in the assumption that they come in order, the
whole buffer is unnecessary and can be replaced by the last lost sequence number
and the count of adjacent lost packets before it.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:25: if (sequence_number <
0x4000 && !lost_packets_buffer_.empty()
On 2015/06/26 00:21:19, noahric wrote:
> If you lose every ~200 packets or so, this wraparound check would miss it.
Maybe
> that's unlikely in practice, but even if you don't do my comment from above
> (about the wraparound counter), consider checking the distance from the high
> sequence number in lost_packets_buffer_ instead of the lowest (i.e. something
> with rbegin(), with some distance like 0x8000). I took a quick look through
the
> webrtc codebase to find how it's handled elsewhere, but I couldn't find
> anything. Hopefully other reviewers will know if there's a generally accepted
> algorithm for it.

True, this did not handle extremely sparse loss very well. I have updated it to
use distance, use the last element in the non-wrapped buffer, and always prune
the entire non-wrapped buffer once the wrapped buffer passes a threshold.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:26: &&
*(lost_packets_buffer_.begin()) > 0xC000) {
On 2015/06/26 00:21:19, noahric wrote:
> && at the end of the previous line
> 
> (you can probably just run 'git cl format' if you haven't yet)

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:64: return;
On 2015/06/26 00:21:19, noahric wrote:
> Consider DCHECK to ensure lost_packets_wrapped_buffer_ is also empty.

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:69: for (int i = 0; i < 2;
++i) {
On 2015/06/26 00:21:19, noahric wrote:
> C++11 ftw:
> 
> for (auto buffer : { &lost_packets_buffer_, &lost_packets_wrapped_buffer_ }) {

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:71: uint16_t next_num = *it;
On 2015/06/26 00:21:19, noahric wrote:
> I'd say current_num, though I bet it wouldn't fit on line 72 :)

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:91: }
On 2015/06/26 00:21:19, noahric wrote:
> Is there a reasonable way to do some more thorough pruning here as well and
move
> it into PruneBuffer? Since we're already enumerating the full list, it seems
> like you could prune everything but the last packet (group). Assuming it's
fast
> enough to do in the normal prune case (if it isn't, maybe you could prune more
> often).

Well, the point of the buffer is to allow reasonably old packet loss to come in
out of order. If we don't care about that, we should just decrease the size of
the buffer or eliminate the buffer entirely and require packets to come in
order.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:111: while
(!lost_packets_buffer_.empty()
On 2015/06/26 00:21:19, noahric wrote:
> Too bad you can't easily combine this with the block above :-/ Especially
since
> you could replace the block above with this version (that allows wrappping)
and
> it'd still work.

Don't tell me what I can't combine :-p

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats.h (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.h:29: // Query the number of
packets that were lost by themselves, no neighboring
On 2015/06/26 00:21:19, noahric wrote:
> Queries (for the rest too)

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.h:39: // with the event count,
this can be used to determine how large events are.
On 2015/06/26 00:21:19, noahric wrote:
> I'd say explicitly "the average event size", if that's what you mean

Done.

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats_unittest.cc (right):

https://codereview.webrtc.org/1198853004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/packet_loss_stats_unittest.cc:36:
stats_.AddLostPacket(i & 0xFFFF);
On 2015/06/26 00:21:19, noahric wrote:
> Consider adding a CHECK or DCHECK in AddLostPacket to ensure the value fits in
> the normal ssrc range. Otherwise, will weird things happen?

AddLostPacket takes a uint16_t, I just like to be explicit with these tests
using ints that it is being masked into a 16 bit type.

https://codereview.webrtc.org/1198853004/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc (right):

https://codereview.webrtc.org/1198853004/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/packet_loss_stats.cc:75: } else if
(sequential_count > 1) {
On 2015/06/30 18:17:03, harryjin wrote:
> Just else is good enough? The else if condition is redundant.

Done.

[mflodman, 2015-07-03 13:14:51]

mflodman@webrtc.org changed reviewers:
+ asapersson@webrtc.org
- mflodman@webrtc.org

[mflodman, 2015-07-03 13:15:50]

Sorry for the delayed response when working through my review backlog. I'll be
OOO the coming two weeks, so I'll add Åsa as a reviewer instead of me.

[åsapersson, 2015-07-06 13:57:24]

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:720: auto it =
receive_loss_stats_.find(remote_ssrc);
The remote_ssrc is the SSRC of the received media stream on this module (which
will stay the same throughout a call?).

Is the intention to handle received NACK requests from multiple receivers for
one sent stream? If so, please see RTCPReceiver::HandleNACK and
rtcpPacket.NACK.SenderSSRC which shows who a NACK request is from (and could be
used as the key for the map?).

[stefan-webrtc, 2015-07-06 14:13:35]

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

[stefan-webrtc, 2015-07-06 14:13:36]

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:730: }
Is this new functionality supposed to track how often a packet is nacked, or to
track loss rates? We have a separate ReceiveStatistics class
(webrtc/modules/rtp_rtcp/interface/receive_statistics.h) which is used for this
very purpose, so this looks a bit misplaced.

[bcornell, 2015-07-08 18:28:12]

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:720: auto it =
receive_loss_stats_.find(remote_ssrc);
On 2015/07/06 13:57:24, asapersson wrote:
> The remote_ssrc is the SSRC of the received media stream on this module (which
> will stay the same throughout a call?).
> 
> Is the intention to handle received NACK requests from multiple receivers for
> one sent stream? If so, please see RTCPReceiver::HandleNACK and
> rtcpPacket.NACK.SenderSSRC which shows who a NACK request is from (and could
be
> used as the key for the map?).

You're right, I had confused myself into thinking that you could get multiple
receive streams here, but you can't, so there's no need for multiple packet loss
counters.

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:730: }
On 2015/07/06 14:13:36, stefan-webrtc (holmer) wrote:
> Is this new functionality supposed to track how often a packet is nacked, or
to
> track loss rates? We have a separate ReceiveStatistics class
> (webrtc/modules/rtp_rtcp/interface/receive_statistics.h) which is used for
this
> very purpose, so this looks a bit misplaced.

It uses how often a packet is NACKed as an approximation of loss rates for both
receive and send. If it were purely for receive, ReceiveStatistics would make
sense, but this also tracks packet loss on the outgoing stream (approximated by
which packets it receives NACKs for). As such I thought it made more sense to
put it in a common location.

[åsapersson, 2015-07-09 06:26:54]

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/1198853004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:730: }
On 2015/07/08 18:28:12, bcornell wrote:
> On 2015/07/06 14:13:36, stefan-webrtc (holmer) wrote:
> > Is this new functionality supposed to track how often a packet is nacked, or
> to
> > track loss rates? We have a separate ReceiveStatistics class
> > (webrtc/modules/rtp_rtcp/interface/receive_statistics.h) which is used for
> this
> > very purpose, so this looks a bit misplaced.
> 
> It uses how often a packet is NACKed as an approximation of loss rates for
both
> receive and send. If it were purely for receive, ReceiveStatistics would make
> sense, but this also tracks packet loss on the outgoing stream (approximated
by
> which packets it receives NACKs for). As such I thought it made more sense to
> put it in a common location.
 
If this is for tracking packet loss, there is a class ReportBlockStats which is
used in ViE for packet loss statistics for sent/received streams based on
received/sent RTCP report blocks.

Maybe this class could be used?

[åsapersson, 2015-07-10 13:48:34]

Note that NACK requests may not necessarily be sent for all lost packets. If
losing a non-base layer temporal layer, there may be a skip forward to the next
base layer and the non-base layer packets may not be NACKed. This might not be
an issue, if so I am fine with this, lgtm.

[bcornell, 2015-07-10 21:09:55]

The CQ bit was checked by bcornell@google.com

[bcornell, 2015-07-10 21:09:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1198853004/#ps180001 (title:
"Another hopeful Mac build fix")

[commit-bot: I haz the power, 2015-07-10 21:10:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198853004/180001

[commit-bot: I haz the power, 2015-07-10 21:59:46]

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

[commit-bot: I haz the power, 2015-07-10 21:59:47]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[noahric, 2015-07-10 23:59:42]

The CQ bit was checked by noahric@chromium.org

[commit-bot: I haz the power, 2015-07-10 23:59:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198853004/180001

[commit-bot: I haz the power, 2015-07-11 01:10:08]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-11 01:10:18]

Patchset 10 (id:??) landed as
https://crrev.com/30409b4dca3d9cfdb0e714a5932b135becb0f822
Cr-Commit-Position: refs/heads/master@{#9568}