rtt calculation handles time go backwards

rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRttToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=asapersson@webrtc.org, stefan@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/c1e55c7136ceaf348d4975df7ac92b480c9f7b6d

[danilchap, 2016-03-04 13:30:42]

Alternative (to https://codereview.webrtc.org/1761483003/), way to mitigate ntp
clock go backward issue when caclulating rtt.
Larger but neater.

[stefan-webrtc, 2016-03-04 14:07:33]

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

[stefan-webrtc, 2016-03-04 14:07:34]

Åsa, can you help review the details of this since you have done some rtt work
before?

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

https://codereview.webrtc.org/1763823003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/time_util.cc:27: value =
DivideRoundToNearest(value * 1000, 1 << 16);
I think you should comment on how and why this division can be negative. I
assume compact_ntp_interval can't be negative?

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

https://codereview.webrtc.org/1763823003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/time_util.h:42: // This interval can be upto ~9.1
hours (2^15 seconds).
up to

[danilchap, 2016-03-04 15:05:50]

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

https://codereview.webrtc.org/1763823003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/time_util.cc:27: value =
DivideRoundToNearest(value * 1000, 1 << 16);
On 2016/03/04 14:07:33, stefan-webrtc (holmer) wrote:
> I think you should comment on how and why this division can be negative. I
> assume compact_ntp_interval can't be negative?
negative case handled before division (otherwise division becomes unnecessary
complicated.) Added comments to clarify this (and other lines in this function,
since all of them might be unobvious)

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

https://codereview.webrtc.org/1763823003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/time_util.h:42: // This interval can be upto ~9.1
hours (2^15 seconds).
On 2016/03/04 14:07:33, stefan-webrtc (holmer) wrote:
> up to

Done.

[åsapersson, 2016-03-08 12:59:44]

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

https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:532: rtt =
CompactNtpRttToMs(rtt_ntp);
I think this will now also result in RTT estimates for receive streams (earlier
prevented by _rtpRtcp.SendTimeOfSendReport > 0) and accounted for several times
in the CallStats class? Maybe add a check that this is not a receive_only
channel (and the opposite for XR rtt)?

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

https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util.cc:39: // Rtt value 0 consideres too
good to be true and increases to 1.
considered

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

https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util_unittest.cc:68: // Ntp difference close
to 2^16 seconds should be treatet as negative.
treated

[danilchap, 2016-03-08 15:02:02]

I'm not sure I understood the issue with CallStats.
Did CallStats got non-zero rtt in non-test setup for receive only stream?

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

https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:532: rtt =
CompactNtpRttToMs(rtt_ntp);
On 2016/03/08 12:59:44, åsapersson wrote:
> I think this will now also result in RTT estimates for receive streams
(earlier
> prevented by _rtpRtcp.SendTimeOfSendReport > 0) and accounted for several
times
> in the CallStats class? Maybe add a check that this is not a receive_only
> channel (and the opposite for XR rtt)?

It shouldn't: send_time should be set to 0 to indicate there is no send time.
That is checked instead of previous _rtpRtcp.SendTimeOfSendReport > 0. Same is
for XR rtt.
Added comment for SR, fixed comment for XR.

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

https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util.cc:39: // Rtt value 0 consideres too
good to be true and increases to 1.
On 2016/03/08 12:59:44, åsapersson wrote:
> considered

Done.

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

https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util_unittest.cc:68: // Ntp difference close
to 2^16 seconds should be treatet as negative.
On 2016/03/08 12:59:44, åsapersson wrote:
> treated

Done.

[åsapersson, 2016-03-08 15:22:30]

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

https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:532: rtt =
CompactNtpRttToMs(rtt_ntp);
On 2016/03/08 15:02:02, danilchap wrote:
> On 2016/03/08 12:59:44, åsapersson wrote:
> > I think this will now also result in RTT estimates for receive streams
> (earlier
> > prevented by _rtpRtcp.SendTimeOfSendReport > 0) and accounted for several
> times
> > in the CallStats class? Maybe add a check that this is not a receive_only
> > channel (and the opposite for XR rtt)?
> 
> It shouldn't: send_time should be set to 0 to indicate there is no send time.
> That is checked instead of previous _rtpRtcp.SendTimeOfSendReport > 0. Same is
> for XR rtt.
> Added comment for SR, fixed comment for XR.

The receive stream(s) has the same main_ssrc_ as the send stream. The received
RTCP packets are input to all streams, therefore i think receive streams also
will get an RTT estimate (which is picked up the the CallStats class).

[danilchap, 2016-03-08 16:16:08]

On 2016/03/08 15:22:30, åsapersson wrote:
>
https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
> File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):
> 
>
https://codereview.webrtc.org/1763823003/diff/20001/webrtc/modules/rtp_rtcp/s...
> webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:532: rtt =
> CompactNtpRttToMs(rtt_ntp);
> On 2016/03/08 15:02:02, danilchap wrote:
> > On 2016/03/08 12:59:44, åsapersson wrote:
> > > I think this will now also result in RTT estimates for receive streams
> > (earlier
> > > prevented by _rtpRtcp.SendTimeOfSendReport > 0) and accounted for several
> > times
> > > in the CallStats class? Maybe add a check that this is not a receive_only
> > > channel (and the opposite for XR rtt)?
> > 
> > It shouldn't: send_time should be set to 0 to indicate there is no send
time.
> > That is checked instead of previous _rtpRtcp.SendTimeOfSendReport > 0. Same
is
> > for XR rtt.
> > Added comment for SR, fixed comment for XR.
> 
> The receive stream(s) has the same main_ssrc_ as the send stream. The received
> RTCP packets are input to all streams, therefore i think receive streams also
> will get an RTT estimate (which is picked up the the CallStats class).

Thank you, now I understand where unexpected behavior is.
Added check of receiver_only_ as an indication if SenderReport were sent,
for ExtendedReports added a special flag that mimic flag in rtcp_sender
responsible for using 
XR for calculating rtt. Will think about neater solution outside of this CL.

[åsapersson, 2016-03-09 09:00:14]

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:193: void
RTCPReceiver::SetRtcpXrRrtrStatus(bool enable) {
should xr_rrtr_status_ be GUARDED_BY(_criticalSectionRTCPReceiver)?

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:486: 
maybe we should move the check earlier and not store the report block
information. could maybe be done in a later CL.

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:531: // Sender Reports even if
it accidentely can.
nit: accidentally

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:924: // Caller should
explicetely enable rtt calculation using extended reports.
nit: explicitly

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:925: if (!xr_rrtr_status_)
move check to line 921

[danilchap, 2016-03-09 09:52:53]

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:193: void
RTCPReceiver::SetRtcpXrRrtrStatus(bool enable) {
On 2016/03/09 09:00:13, åsapersson wrote:
> should xr_rrtr_status_ be GUARDED_BY(_criticalSectionRTCPReceiver)?

Done. Do not think it should, but it doesn't hurt either. Better be safe.

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:486: 
On 2016/03/09 09:00:13, åsapersson wrote:
> maybe we should move the check earlier and not store the report block
> information. could maybe be done in a later CL.

May be, but not in this CL. (I have CL that changes report block handling:
https://codereview.webrtc.org/1540383002/ - probably should incorporate this
change there).

This CL is intended to be merged into M51, so would prefer to have fixes, but
not improvements here.

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:531: // Sender Reports even if
it accidentely can.
On 2016/03/09 09:00:13, åsapersson wrote:
> nit: accidentally

Done.

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:924: // Caller should
explicetely enable rtt calculation using extended reports.
On 2016/03/09 09:00:13, åsapersson wrote:
> nit: explicitly

Done.

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:925: if (!xr_rrtr_status_)
On 2016/03/09 09:00:13, åsapersson wrote:
> move check to line 921
Would rather not in this CL:
xr_dlrr_item flag is used in tests only, and the only impact of moving check
before setting the flag is breaking some tests. (Tests explicitly expect this
flag to be set even when rtt can't be calculated)

[åsapersson, 2016-03-09 10:06:47]

lgtm

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1763823003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:486: 
On 2016/03/09 09:52:53, danilchap wrote:
> On 2016/03/09 09:00:13, åsapersson wrote:
> > maybe we should move the check earlier and not store the report block
> > information. could maybe be done in a later CL.
> 
> May be, but not in this CL. (I have CL that changes report block handling:
> https://codereview.webrtc.org/1540383002/ - probably should incorporate this
> change there).
> 
> This CL is intended to be merged into M51, so would prefer to have fixes, but
> not improvements here.

Acknowledged.

[danilchap, 2016-03-09 11:05:36]

Description was changed from

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRtpToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=stefan@webrtc.org
==========

to

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRtpToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=stefan@webrtc.org
==========

[danilchap, 2016-03-09 11:05:43]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-03-09 11:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1763823003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1763823003/80001

[danilchap, 2016-03-09 11:06:12]

Description was changed from

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRtpToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=stefan@webrtc.org
==========

to

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRttToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=stefan@webrtc.org
==========

[commit-bot: I haz the power, 2016-03-09 12:41:36]

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

[commit-bot: I haz the power, 2016-03-09 12:41:37]

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

[danilchap, 2016-03-09 14:14:51]

Description was changed from

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRttToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=stefan@webrtc.org
==========

to

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRttToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=asapersson@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/c1e55c7136ceaf348d4975df7...
==========

[commit-bot: I haz the power, 2016-03-09 14:14:51]

Description was changed from

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRttToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=stefan@webrtc.org
==========

to

==========
rtt calculation handles time go backwards

CompactNtpIntervalToMs renamed to CompactNtpRttToMs and handle special cases:
large values consider negative/invalid and result in value of 1.
0 result consider too small and increases to 1.

BUG=590996
R=asapersson@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/c1e55c7136ceaf348d4975df7ac92b480c9f7b6d
Cr-Commit-Position: refs/heads/master@{#11928}
==========

[commit-bot: I haz the power, 2016-03-09 14:14:52]

Patchset 5 (id:??) landed as
https://crrev.com/c1e55c7136ceaf348d4975df7ac92b480c9f7b6d
Cr-Commit-Position: refs/heads/master@{#11928}

[danilchap, 2016-03-09 14:14:52]

Committed patchset #5 (id:80001) manually as
c1e55c7136ceaf348d4975df7ac92b480c9f7b6d (presubmit successful).