[rtp_rtcp] time helper functions

[rtp_rtcp] time helper functions
  RTP timestams helper functions moved from rtp_utility
  added functions to deal with CompactNtp timestamps

R=åsapersson
BUG=webrtc:5260
Committed: https://crrev.com/1227e8b3451b1a2f2a765bf6101cb0862f625568
Cr-Commit-Position: refs/heads/master@{#11106}

[danilchap, 2015-12-18 13:24:39]

this is a lighter alternative to CL#1481773004 (RTP utility sligthly cleaned)
This one introduce same helper functions, but leave cleaning rtp_utility for
later CLs.

[åsapersson, 2015-12-21 11:19:18]

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

https://codereview.webrtc.org/1535113002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:111: NtpTime
receive_time) {
use const ref for receive_time?

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

https://codereview.webrtc.org/1535113002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/time_util_unittest.cc:46: uint32_t ntp_to_ms_diff
= CompactNtpToMs(ntp_diff);
How about this diff?
uint32_t ms1 = CompactNtpToMs(0xffff0000);
uint32_t ms2 = CompactNtpToMs(0x00000000);
ms2-ms1 =

[danilchap, 2015-12-21 12:49:50]

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

https://codereview.webrtc.org/1535113002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc:111: NtpTime
receive_time) {
On 2015/12/21 11:19:17, åsapersson wrote:
> use const ref for receive_time?
NtpTime is a trivial type (basically it is an int64_t wrapper).
Passing it by const reference will not improve perfomance.

I've updated functions in time_util.h to use NtpTime by value to be consistent.

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

https://codereview.webrtc.org/1535113002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/time_util_unittest.cc:46: uint32_t ntp_to_ms_diff
= CompactNtpToMs(ntp_diff);
On 2015/12/21 11:19:17, åsapersson wrote:
> How about this diff?
> uint32_t ms1 = CompactNtpToMs(0xffff0000);
> uint32_t ms2 = CompactNtpToMs(0x00000000);
> ms2-ms1 =
I do not see a valid use case in this example:

Time value could be used as a period (e.g. rtt) or as an absolute value (period
since some defined point, e.g. 1st january 1970)

As an absolute value CompactNtp is not a good value: it measures time since 1970
modulo ~ 18.2 hours. I.e. measure time since some defined point that was no
longer than 18.2 hours ago, wrapping around.

Converting absolute compact ntp value to ms doesn't make much sense (to make
sense of the result value, one need arithmetic using some nontrivial non-integer
module, which is obscure. i.e. ((ms2-ms1) mod 18.2 hours) ~= 1 sec).

Converting relative compact ntp value does make sense.

Subtracting two absolute compact ntp values (preferably obtained from same
clock) would result in a valid relative compact ntp value that can be nicely
converted to ms.

May be it would be more clear to create types CompactNtp and CompactNtpInterval
as wrappers for uint32_t and define operations that make sense there?

for now
Renamed function CompactNtpToMs to CompactNtpIntervalToMs to hint that there is
little point to convert absolute CompactNtp values to ms.

[åsapersson, 2015-12-21 15:16:20]

lgtm

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

https://codereview.webrtc.org/1535113002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/time_util_unittest.cc:46: uint32_t ntp_to_ms_diff
= CompactNtpToMs(ntp_diff);
On 2015/12/21 12:49:50, danilchap wrote:
> On 2015/12/21 11:19:17, åsapersson wrote:
> > How about this diff?
> > uint32_t ms1 = CompactNtpToMs(0xffff0000);
> > uint32_t ms2 = CompactNtpToMs(0x00000000);
> > ms2-ms1 =
> I do not see a valid use case in this example:
> 
> Time value could be used as a period (e.g. rtt) or as an absolute value
(period
> since some defined point, e.g. 1st january 1970)
> 
> As an absolute value CompactNtp is not a good value: it measures time since
1970
> modulo ~ 18.2 hours. I.e. measure time since some defined point that was no
> longer than 18.2 hours ago, wrapping around.
> 
> Converting absolute compact ntp value to ms doesn't make much sense (to make
> sense of the result value, one need arithmetic using some nontrivial
non-integer
> module, which is obscure. i.e. ((ms2-ms1) mod 18.2 hours) ~= 1 sec).
> 
> Converting relative compact ntp value does make sense.
> 
> Subtracting two absolute compact ntp values (preferably obtained from same
> clock) would result in a valid relative compact ntp value that can be nicely
> converted to ms.
> 
> May be it would be more clear to create types CompactNtp and
CompactNtpInterval
> as wrappers for uint32_t and define operations that make sense there?
> 
> for now
> Renamed function CompactNtpToMs to CompactNtpIntervalToMs to hint that there
is
> little point to convert absolute CompactNtp values to ms.

Yes that makes it more clear.

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

https://codereview.webrtc.org/1535113002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util.h:40: }
Maybe add a comment that a Ntp difference up to ~18 hours should convert
correctly.

https://codereview.webrtc.org/1535113002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util.h:41: inline uint32_t
CompactNtpIntervalToMs(uint32_t ntp) {
call this ntp_diff?

[danilchap, 2015-12-21 16:14:34]

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

https://codereview.webrtc.org/1535113002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util.h:40: }
On 2015/12/21 15:16:20, åsapersson wrote:
> Maybe add a comment that a Ntp difference up to ~18 hours should convert
> correctly.

Done.

https://codereview.webrtc.org/1535113002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/time_util.h:41: inline uint32_t
CompactNtpIntervalToMs(uint32_t ntp) {
On 2015/12/21 15:16:20, åsapersson wrote:
> call this ntp_diff?

good idea. Called it compact_ntp_interval:
compact_ to stress it is not a difference between full-scale ntp timestamps.
_interval instead of _diff because diff feels like only two values could be
involved, but there is code where it is calculated from 3 values (rtt = received
- delay - send; received and send are timestamps and delay is an interval).

[danilchap, 2015-12-21 18:06:54]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2015-12-21 18:06:54]

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/1535113002/#ps60001 (title:
"rebase")

[commit-bot: I haz the power, 2015-12-21 18:06:58]

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

[commit-bot: I haz the power, 2015-12-21 19:06:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-21 19:07:02]

Description was changed from

==========
[rtp_rtcp] time helper functions
  RTP timestams helper functions moved from rtp_utility
  added functions to deal with CompactNtp timestamps

R=åsapersson
BUG=webrtc:5260
==========

to

==========
[rtp_rtcp] time helper functions
  RTP timestams helper functions moved from rtp_utility
  added functions to deal with CompactNtp timestamps

R=åsapersson
BUG=webrtc:5260

Committed: https://crrev.com/1227e8b3451b1a2f2a765bf6101cb0862f625568
Cr-Commit-Position: refs/heads/master@{#11106}
==========

[commit-bot: I haz the power, 2015-12-21 19:07:03]

Patchset 4 (id:??) landed as
https://crrev.com/1227e8b3451b1a2f2a765bf6101cb0862f625568
Cr-Commit-Position: refs/heads/master@{#11106}