[rtp_rtcp] RTT intermidiate calculation use ntp time instead of milliseconds.

RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp presentation and milliseconds in the RTCPSender.

BUG=webrtc:5565
R=stefan@webrtc.org

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

[danilchap, 2015-12-29 10:02:29]

Description was changed from

==========
RTT intermidiate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT.
  Until plans to use RTP over IPoAC (RFC1149) compact NTP time range should be
enough to calculate RTT.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5260
R=åsapersson,språng
==========

to

==========
RTT intermidiate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT.
  Until plans to use RTP over IPoAC (RFC1149) compact NTP time range should be
enough to calculate RTT.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5260
R=åsapersson,språng
==========

[danilchap, 2015-12-29 10:02:30]

danilchap@webrtc.org changed reviewers:
- sprang@webrtc.org

[danilchap, 2016-02-01 13:33:30]

Description was changed from

==========
RTT intermidiate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT.
  Until plans to use RTP over IPoAC (RFC1149) compact NTP time range should be
enough to calculate RTT.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5260
R=åsapersson,språng
==========

to

==========
RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.
==========

[danilchap, 2016-02-01 13:33:30]

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

[stefan-webrtc, 2016-02-03 10:03:11]

Seems like a nice improvement!

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

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:525: uint32_t RTT = 0;
rtt

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:920: // The send_time and
delay_rr fields are in units of 1/65536 sec.
1/2^16 as above?

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:932: xr_rr_rtt_ms_ =
std::max<uint32_t>(rtt_ms, 1);
Maybe we should break this out into a method and reuse here and above?

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

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:217:
EXPECT_NEAR(kRttMs, rtt_ms, 1);
What's the reason we can't get an exact rtt here?

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:703:
dlrr.WithDlrrItem(kSourceSsrc, 0, 0x67890);
Could you explain these changes to me?

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:820: }
Maybe we can reuse a few lines from the other test you added? See that as
optional if it turns out to be non trivial.

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

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:352: EXPECT_NEAR(2 *
kOneWayNetworkDelayMs, receiver_.impl_->rtt_ms(), 1);
Here too, it'd be nice to know why this isn't as exact any longer?

[danilchap, 2016-02-03 13:27:11]

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

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:525: uint32_t RTT = 0;
On 2016/02/03 10:03:10, stefan-webrtc (holmer) wrote:
> rtt

Done.

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:920: // The send_time and
delay_rr fields are in units of 1/65536 sec.
On 2016/02/03 10:03:10, stefan-webrtc (holmer) wrote:
> 1/2^16 as above?

Done.

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:932: xr_rr_rtt_ms_ =
std::max<uint32_t>(rtt_ms, 1);
On 2016/02/03 10:03:10, stefan-webrtc (holmer) wrote:
> Maybe we should break this out into a method and reuse here and above?

I would like to move one of this blocks (the other one) into different file
(https://codereview.webrtc.org/1540383002/)
It would be harder to reuse then.

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

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:217:
EXPECT_NEAR(kRttMs, rtt_ms, 1);
On 2016/02/03 10:03:11, stefan-webrtc (holmer) wrote:
> What's the reason we can't get an exact rtt here?

Rounding.
if rtt would be stored and returned in 1/2^16 seconds, the result would be
exact.

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:703:
dlrr.WithDlrrItem(kSourceSsrc, 0, 0x67890);
On 2016/02/03 10:03:11, stefan-webrtc (holmer) wrote:
> Could you explain these changes to me?

changed field is sent time.
Before this cl it wasn't possible to estimate RTT because rtcp_sender associated
with rtcp_receiver should have send Rrtr Report at that send time.
This CL removes this complicated setup.
sent time 0 indicate there is no sent time and RTT shouldn't be estimated.

This test is not about RTT.
Though it is better to update the comment since RTT calculation is not checked.

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:742:
dlrr.WithDlrrItem(kSourceSsrc, 0, 0x67890);
This test does check rtt wasn't estimated. So need to be updated it to prevent
rtt calculation.

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:820: }
On 2016/02/03 10:03:11, stefan-webrtc (holmer) wrote:
> Maybe we can reuse a few lines from the other test you added? See that as
> optional if it turns out to be non trivial.

Do not want to think about test improvements in this CL.
There is another CL where I refactor this tests in more radical way
(https://codereview.webrtc.org/1527003002/)

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

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc:352: EXPECT_NEAR(2 *
kOneWayNetworkDelayMs, receiver_.impl_->rtt_ms(), 1);
On 2016/02/03 10:03:11, stefan-webrtc (holmer) wrote:
> Here too, it'd be nice to know why this isn't as exact any longer?

It wasn't exact before too, but with simulated clock using milliseconds it was
more likely answer would be exact.
It is possible to increase chance to get exact rtt in tests by changing Rounding
from 'round down' to 'round to nearest'. Or pick different values to get exact
values, but I think it is better to show in tests that RTT can't be estimated
exactly.

[danilchap, 2016-02-12 16:01:00]

changed rounding to nearest instead of lowest.
test would now pass with expectation changed to _EQ from _NEAR(,,1)
but would like to keep NEAR expectation to indicate rtt in ms can't be
calculated exactly.

[stefan-webrtc, 2016-02-22 16:14:26]

On 2016/02/12 16:01:00, danilchap wrote:
> changed rounding to nearest instead of lowest.
> test would now pass with expectation changed to _EQ from _NEAR(,,1)
> but would like to keep NEAR expectation to indicate rtt in ms can't be
> calculated exactly.

Sounds good.

lgtm

[stefan-webrtc, 2016-02-22 16:14:36]

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

https://codereview.webrtc.org/1491843004/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc:217:
EXPECT_NEAR(kRttMs, rtt_ms, 1);
On 2016/02/03 13:27:11, danilchap wrote:
> On 2016/02/03 10:03:11, stefan-webrtc (holmer) wrote:
> > What's the reason we can't get an exact rtt here?
> 
> Rounding.
> if rtt would be stored and returned in 1/2^16 seconds, the result would be
> exact.

Acknowledged.

[danilchap, 2016-02-22 16:19:37]

Description was changed from

==========
RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.
==========

to

==========
RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5565
==========

[danilchap, 2016-02-22 16:19:45]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-02-22 16:19:48]

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

[commit-bot: I haz the power, 2016-02-22 16:21:54]

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

[commit-bot: I haz the power, 2016-02-22 16:21:54]

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

[danilchap, 2016-02-22 17:59:52]

Description was changed from

==========
RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5565
==========

to

==========
RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5565
R=stefan@webrtc.org

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

[commit-bot: I haz the power, 2016-02-22 17:59:53]

Description was changed from

==========
RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5565
R=stefan@webrtc.org

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

to

==========
RTT intermediate calculation use ntp time instead of milliseconds.
  Compact NTP representation was designed exactly for that purpose: calculate
RTT. No need to map to ms before doing arithmetic on this values.
  Because of this change there is no need to keep mapping between compact ntp
presentation and milliseconds in the RTCPSender.

BUG=webrtc:5565
R=stefan@webrtc.org

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

[danilchap, 2016-02-22 17:59:53]

Committed patchset #6 (id:100001) manually as
a094fd1550322552809fea8bcdd8f3e297318323 (presubmit successful).

[commit-bot: I haz the power, 2016-02-22 17:59:54]

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