Compute packet loss for event log visualization similar to how it is defined in RFC 3550.

Compute packet loss for event log visualization similar to how it is defined in RFC 3550.

The main difference to the old computation is that the expected number of packets during an interval is now computed as the change in highest sequence number encountered, rather than the sequence number difference between the first and last packet in the interval.

BUG=webrtc:7046
Review-Url: https://codereview.webrtc.org/2656333002
Cr-Commit-Position: refs/heads/master@{#16361}
Committed: https://chromium.googlesource.com/external/webrtc/+/4c9b4af53a47b09ca3be2e5a862af49254bc219a

[terelius, 2017-01-27 11:59:19]

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

[stefan-webrtc, 2017-01-27 13:02:12]

lg, just one question

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

https://codereview.webrtc.org/2656333002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:728: int64_t received_packets =
window_index_end - window_index_begin;
Is there any chance that this diff becomes zero even though we have received one
packet in the window?

[terelius, 2017-01-27 14:03:11]

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

https://codereview.webrtc.org/2656333002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:728: int64_t received_packets =
window_index_end - window_index_begin;
On 2017/01/27 13:02:11, stefan-webrtc wrote:
> Is there any chance that this diff becomes zero even though we have received
one
> packet in the window?

The loop on line 700 advances the window_index_end to the first packet past the
window. The loop on line 717 advances window_index_begin to the first packet in
the window (assuming there is a packet in the window). Thus, if there are
packets in the window, window_index_end > window_index_begin.

https://codereview.webrtc.org/2656333002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:729: int64_t lost_packets =
expected_packets - received_packets;
Note, reordering or duplication can cause expected_packets to be lower than the
number of received packets in the window. This would still cause the fraction
loss to be negative.

[stefan-webrtc, 2017-01-27 15:19:00]

lgtm

[minyue-webrtc, 2017-01-30 08:22:34]

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

https://codereview.webrtc.org/2656333002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:729: int64_t lost_packets =
expected_packets - received_packets;
On 2017/01/27 14:03:11, terelius wrote:
> Note, reordering or duplication can cause expected_packets to be lower than
the
> number of received packets in the window. This would still cause the fraction
> loss to be negative.

Is this a flaw in RFC 3550?

[terelius, 2017-01-30 10:03:05]

minyue, please try this version on the log that was causing you problems.

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

https://codereview.webrtc.org/2656333002/diff/1/webrtc/tools/event_log_visual...
webrtc/tools/event_log_visualizer/analyzer.cc:729: int64_t lost_packets =
expected_packets - received_packets;
On 2017/01/30 08:22:34, minyue-webrtc wrote:
> On 2017/01/27 14:03:11, terelius wrote:
> > Note, reordering or duplication can cause expected_packets to be lower than
> the
> > number of received packets in the window. This would still cause the
fraction
> > loss to be negative.
> 
> Is this a flaw in RFC 3550?

They are aware that the computation can give negative results. In the spec, they
use clipping to replace all negative fractions by zero. Clipping helps them save
bits in the RTCP but also reduces the amount of information contained in the
signal.

For our purposes, I think allowing negative fraction losses is the right thing
to do. For example, if you've previously seen packet number 4 and now receive
packets 5, 6, 6, 7 and 8, I think it is reasonable say that the packet loss is
-25% since you have received 25% more packets than you expected. Or do you think
that is too confusing?

[minyue-webrtc, 2017-01-30 12:05:44]

One comment. BTW, I suggest adding some note on the improvement of new method
over the old one in the CL description.

https://codereview.webrtc.org/2656333002/diff/20001/webrtc/tools/event_log_vi...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2656333002/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/analyzer.cc:703: int64_t highest_seq_number =
should it be more correct to placed after 708, and changed to

highest_seq_number =
unwrapper_.Unwrap(packet_stream[window_index_end].header.sequenceNumber) - 1;

https://codereview.webrtc.org/2656333002/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/analyzer.cc:705: int64_t
highest_prior_seq_number =
and this maybe after 715

[terelius, 2017-01-30 14:49:34]

https://codereview.webrtc.org/2656333002/diff/20001/webrtc/tools/event_log_vi...
File webrtc/tools/event_log_visualizer/analyzer.cc (right):

https://codereview.webrtc.org/2656333002/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/analyzer.cc:703: int64_t highest_seq_number =
On 2017/01/30 12:05:44, minyue-webrtc wrote:
> should it be more correct to placed after 708, and changed to
> 
> highest_seq_number =
> unwrapper_.Unwrap(packet_stream[window_index_end].header.sequenceNumber) - 1;
> 

Without these lines, highest_prior_seq_number won't be assigned a value during
the first interval. That causes lines 725 and 728 to consider almost all packets
to be lost at the beginning of the call.

https://codereview.webrtc.org/2656333002/diff/20001/webrtc/tools/event_log_vi...
webrtc/tools/event_log_visualizer/analyzer.cc:705: int64_t
highest_prior_seq_number =
On 2017/01/30 12:05:44, minyue-webrtc wrote:
> and this maybe after 715

We could track the highest sequence number reported during the previous window
rather than the highest over the entire call. However, there will only be a
difference if the packets are reordering across more than a full window
(currently 1 second). This is not likely to occur in practice, and if it does
happen, I think a weird-looking graph might help draw attention to the problem.

[terelius, 2017-01-30 14:56:46]

Description was changed from

==========
Compute packet loss similar to how it is defined in RFC 3550.

BUG=webrtc:7046
==========

to

==========
Compute packet loss similar to how it is defined in RFC 3550.

The largest difference to the old computation is that the expected number of
packets during an interval is now computed as the change in highest sequence
number encountered, rather than the sequence number difference between the first
and last packet in the interval.

BUG=webrtc:7046
==========

[terelius, 2017-01-30 14:58:34]

Description was changed from

==========
Compute packet loss similar to how it is defined in RFC 3550.

The largest difference to the old computation is that the expected number of
packets during an interval is now computed as the change in highest sequence
number encountered, rather than the sequence number difference between the first
and last packet in the interval.

BUG=webrtc:7046
==========

to

==========
Compute packet loss for event log visualization similar to how it is defined in
RFC 3550.

The main difference to the old computation is that the expected number of
packets during an interval is now computed as the change in highest sequence
number encountered, rather than the sequence number difference between the first
and last packet in the interval.

BUG=webrtc:7046
==========

[minyue-webrtc, 2017-01-30 15:16:28]

lgtm

[terelius, 2017-01-30 16:07:30]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2017-01-30 16:07:30]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2656333002/#ps20001 (title:
"Fix incorrect loss rate for first sample.")

[commit-bot: I haz the power, 2017-01-30 16:07:45]

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-01-30 16:44:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485792450022390,
"parent_rev": "aa4b0775aa9877410fb45fe406e786aafef6f934", "commit_rev":
"4c9b4af53a47b09ca3be2e5a862af49254bc219a"}

[commit-bot: I haz the power, 2017-01-30 16:44:54]

Description was changed from

==========
Compute packet loss for event log visualization similar to how it is defined in
RFC 3550.

The main difference to the old computation is that the expected number of
packets during an interval is now computed as the change in highest sequence
number encountered, rather than the sequence number difference between the first
and last packet in the interval.

BUG=webrtc:7046
==========

to

==========
Compute packet loss for event log visualization similar to how it is defined in
RFC 3550.

The main difference to the old computation is that the expected number of
packets during an interval is now computed as the change in highest sequence
number encountered, rather than the sequence number difference between the first
and last packet in the interval.

BUG=webrtc:7046

Review-Url: https://codereview.webrtc.org/2656333002
Cr-Commit-Position: refs/heads/master@{#16361}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4c9b4af53a47b09ca3be2e5a8...
==========

[commit-bot: I haz the power, 2017-01-30 16:44:55]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/4c9b4af53a47b09ca3be2e5a8...