Change to calculate the real NTP in TimeTicks.

Change to calculate the real NTP in TimeTicks.

Added static method to correlate the TimeTicks to a real date and time; in this case UnixEpoch since that is a commonly known time/date.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232194

[pwestin, 2013-10-23 16:15:01]

Yuri please advice on how to get this functionality in to the media code. We
need the current system date/time to create an accurate NTP, in this CL we only
call it once hence there is no risk of glitches introduced by the timer.

[miu, 2013-10-23 18:05:33]

On 2013/10/23 16:15:01, pwestin wrote:
> Yuri please advice on how to get this functionality in to the media code. We
> need the current system date/time to create an accurate NTP, in this CL we
only
> call it once hence there is no risk of glitches introduced by the timer.

This seems like a reasonable use case for base::Time.  In order to use
base::Time::Now(), you would need to modify src/media/PRESUBMIT.py to exclude
cast_ntp_tick_clock.cc (probably the method at line 15).  You could include
those modifications with this patch (include scherkus@ for OWNERS) and, IIRC,
this should allow presubmit to succeed.

Some questions:

1. How exactly are these NTP timestamps used?  Are they basically used by the
remote to determine the timing around the transport protocols (e.g., when to
send NACKs)?  Are they used to timestamp frames of audio/video, or anywhere
else?
2. How would other systems normally deal with, for example, Daylight Saving Time
resets in the middle of a session?
3. The remote and local clocks can never be exactly the same, so is the remote
expected to tolerate any NTP timestamp (e.g., a timestamp that seems to be
several minutes/hours/days off)?
4. This would definitely be a horrible hack, but would the system work if you
replaced base::Time::Now() with a fixed value?

[miu, 2013-10-23 18:24:02]

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_defines.h#new...
media/cast/cast_defines.h:107: int64 time_us = time.ToInternalValue();
IMHO, it's much cleaner to work with only one timeline in the cast code. 
Meaning, I'd suggest you only convert to/from NTP time for the packets, but
otherwise have all other code be on the same timeline (i.e., same origin) as
base::TimeTicks::Now().

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
File media/cast/cast_ntp_tick_clock.cc (right):

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
media/cast/cast_ntp_tick_clock.cc:29: tick_clock_ = tick_clock;
start_offset_ needs to be recalculated here.  Perhaps you can just move all the
ctor code into here, and have the ctor call this method?

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
File media/cast/cast_ntp_tick_clock_unittest.cc (right):

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
media/cast/cast_ntp_tick_clock_unittest.cc:52: base::TimeDelta advence_time =
base::TimeDelta::FromSeconds(kAdvanceTime);
typo: s/advence/advance/

[pwestin, 2013-10-23 21:07:49]

Addressed comments and modified the PRESUBMIT script.

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_defines.h#new...
media/cast/cast_defines.h:107: int64 time_us = time.ToInternalValue();
On 2013/10/23 18:24:02, Yuri wrote:
> IMHO, it's much cleaner to work with only one timeline in the cast code. 
> Meaning, I'd suggest you only convert to/from NTP time for the packets, but
> otherwise have all other code be on the same timeline (i.e., same origin) as
> base::TimeTicks::Now().
 

Removed this function since it did not provide any value any longer

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
File media/cast/cast_ntp_tick_clock.cc (right):

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
media/cast/cast_ntp_tick_clock.cc:29: tick_clock_ = tick_clock;
On 2013/10/23 18:24:02, Yuri wrote:
> start_offset_ needs to be recalculated here.  Perhaps you can just move all
the
> ctor code into here, and have the ctor call this method?

Done.

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
File media/cast/cast_ntp_tick_clock_unittest.cc (right):

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
media/cast/cast_ntp_tick_clock_unittest.cc:52: base::TimeDelta advence_time =
base::TimeDelta::FromSeconds(kAdvanceTime);
On 2013/10/23 18:24:02, Yuri wrote:
> typo: s/advence/advance/

Done.

https://codereview.chromium.org/34623008/diff/1/media/cast/cast_ntp_tick_cloc...
media/cast/cast_ntp_tick_clock_unittest.cc:52: base::TimeDelta advence_time =
base::TimeDelta::FromSeconds(kAdvanceTime);
On 2013/10/23 18:24:02, Yuri wrote:
> typo: s/advence/advance/

Done.

[miu, 2013-10-23 21:08:38]

I was just chatting about this "time problem" with hubbe@.  One issue I noticed
is that the type of the object (e.g., base::Time vs base::TimeTicks) also
implies the time origin.  Meaning, all base::TimeTicks values should be on the
same timeline with the same origin.

To solve the problem(s) at-hand, we think it makes sense for you to add a
base::TimeTicks::UnixEpoch() static method.  Inside this method would be the
code that stores a single snapshot of the difference between base::Time::Now()
and base::TimeTicks::Now() to produce the UnixEpoch event on the TimeTicks
timeline.  This might look like the following:

  // In src/base/time.cc...

  // static
  TimeTicks TimeTicks::UnixEpoch() {
    static const TimeTicks ticks_unix_epoch =
        TimeTicks::Now() - (Time::Now() - Time::UnixEpoch());

    return ticks_unix_epoch;
  }

With this, you can now easily convert from base::TimeTicks values to NTP
microseconds in media/cast code, like so:

  int64 ToNTPMicoseconds(base::TimeTicks ticks) {
    static const base::TimeDelta kNtpEpochDelta =
        base::TimeDelta::FromSeconds(GG_INT64_C(9435484800));

    base::TimeDelta elapsed_since_unix_epoch = ticks -
base::TimeTicks::UnixEpoch();
    return (elapsed_since_unix_epoch + kNtpEpochDelta).ToMicroseconds();
  }

Make sense?  Benefits: 1) No PRESUBMIT violations would occur in src/media; 2)
Fairly clean; 3) Allows future code (anywhere in Chromium) to use TimeTicks and
be able to convert to real-world time.

[pwestin, 2013-10-23 21:15:16]

See inline answers

On 2013/10/23 18:05:33, Yuri wrote:
> On 2013/10/23 16:15:01, pwestin wrote:
> > Yuri please advice on how to get this functionality in to the media code. We
> > need the current system date/time to create an accurate NTP, in this CL we
> only
> > call it once hence there is no risk of glitches introduced by the timer.
> 
> This seems like a reasonable use case for base::Time.  In order to use
> base::Time::Now(), you would need to modify src/media/PRESUBMIT.py to exclude
> cast_ntp_tick_clock.cc (probably the method at line 15).  You could include
> those modifications with this patch (include scherkus@ for OWNERS) and, IIRC,
> this should allow presubmit to succeed.
> 
> Some questions:
> 
> 1. How exactly are these NTP timestamps used?  Are they basically used by the
> remote to determine the timing around the transport protocols (e.g., when to
> send NACKs)?  Are they used to timestamp frames of audio/video, or anywhere
> else?

They are used to synchronize the audio and video streams; hence the real
relation 
to the current time should not matter; however many implementations do care but
not ours. In theory you can sync multiple sources in the receiver IF ALL the
senders have synchronized their NTP clocks...  

> 2. How would other systems normally deal with, for example, Daylight Saving
Time
> resets in the middle of a session?

Yes they would since is should refer to UTC which does not have Daylight Saving
Time

> 3. The remote and local clocks can never be exactly the same, so is the remote
> expected to tolerate any NTP timestamp (e.g., a timestamp that seems to be
> several minutes/hours/days off)?

There is a protocol to synchronize your NTP time across all machines ...
However we don't require it or depend on it


> 4. This would definitely be a horrible hack, but would the system work if you
> replaced base::Time::Now() with a fixed value?

Yes it would work

[pwestin, 2013-10-23 21:19:22]

On 2013/10/23 21:08:38, Yuri wrote:
> I was just chatting about this "time problem" with hubbe@.  One issue I
noticed
> is that the type of the object (e.g., base::Time vs base::TimeTicks) also
> implies the time origin.  Meaning, all base::TimeTicks values should be on the
> same timeline with the same origin.
> 
> To solve the problem(s) at-hand, we think it makes sense for you to add a
> base::TimeTicks::UnixEpoch() static method.  Inside this method would be the
> code that stores a single snapshot of the difference between base::Time::Now()
> and base::TimeTicks::Now() to produce the UnixEpoch event on the TimeTicks
> timeline.  This might look like the following:
> 
>   // In src/base/time.cc...
> 
>   // static
>   TimeTicks TimeTicks::UnixEpoch() {
>     static const TimeTicks ticks_unix_epoch =
>         TimeTicks::Now() - (Time::Now() - Time::UnixEpoch());
> 
>     return ticks_unix_epoch;
>   }
> 
> With this, you can now easily convert from base::TimeTicks values to NTP
> microseconds in media/cast code, like so:
> 
>   int64 ToNTPMicoseconds(base::TimeTicks ticks) {
>     static const base::TimeDelta kNtpEpochDelta =
>         base::TimeDelta::FromSeconds(GG_INT64_C(9435484800));
> 
>     base::TimeDelta elapsed_since_unix_epoch = ticks -
> base::TimeTicks::UnixEpoch();
>     return (elapsed_since_unix_epoch + kNtpEpochDelta).ToMicroseconds();
>   }
> 
> Make sense?  Benefits: 1) No PRESUBMIT violations would occur in src/media; 2)
> Fairly clean; 3) Allows future code (anywhere in Chromium) to use TimeTicks
and
> be able to convert to real-world time.

ok make sense; assuming we can get an owner of base to approve the change

[hubbe, 2013-10-23 22:40:02]

On 2013/10/23 21:19:22, pwestin wrote:
> On 2013/10/23 21:08:38, Yuri wrote:
> > I was just chatting about this "time problem" with hubbe@.  One issue I
> noticed
> > is that the type of the object (e.g., base::Time vs base::TimeTicks) also
> > implies the time origin.  Meaning, all base::TimeTicks values should be on
the
> > same timeline with the same origin.
> > 
> > To solve the problem(s) at-hand, we think it makes sense for you to add a
> > base::TimeTicks::UnixEpoch() static method.  Inside this method would be the
> > code that stores a single snapshot of the difference between
base::Time::Now()
> > and base::TimeTicks::Now() to produce the UnixEpoch event on the TimeTicks
> > timeline.  This might look like the following:
> > 
> >   // In src/base/time.cc...
> > 
> >   // static
> >   TimeTicks TimeTicks::UnixEpoch() {
> >     static const TimeTicks ticks_unix_epoch =
> >         TimeTicks::Now() - (Time::Now() - Time::UnixEpoch());
> > 
> >     return ticks_unix_epoch;
> >   }
> > 
> > With this, you can now easily convert from base::TimeTicks values to NTP
> > microseconds in media/cast code, like so:
> > 
> >   int64 ToNTPMicoseconds(base::TimeTicks ticks) {
> >     static const base::TimeDelta kNtpEpochDelta =
> >         base::TimeDelta::FromSeconds(GG_INT64_C(9435484800));
> > 
> >     base::TimeDelta elapsed_since_unix_epoch = ticks -
> > base::TimeTicks::UnixEpoch();
> >     return (elapsed_since_unix_epoch + kNtpEpochDelta).ToMicroseconds();
> >   }
> > 


I think this would be simpler:
int64 ToNTPMicroseconds(base::TimeTicks ticks) {
  return (ticks - base::TimeTicks::UnixEpoch()).ToMicroseconds() +
GG_INT64_C(9435484800000000);
}

> > Make sense?  Benefits: 1) No PRESUBMIT violations would occur in src/media;
2)
> > Fairly clean; 3) Allows future code (anywhere in Chromium) to use TimeTicks
> and
> > be able to convert to real-world time.
> 
> ok make sense; assuming we can get an owner of base to approve the change

[pwestin, 2013-10-23 22:47:50]

On 2013/10/23 22:40:02, hubbe wrote:
> On 2013/10/23 21:19:22, pwestin wrote:
> > On 2013/10/23 21:08:38, Yuri wrote:
> > > I was just chatting about this "time problem" with hubbe@.  One issue I
> > noticed
> > > is that the type of the object (e.g., base::Time vs base::TimeTicks) also
> > > implies the time origin.  Meaning, all base::TimeTicks values should be on
> the
> > > same timeline with the same origin.
> > > 
> > > To solve the problem(s) at-hand, we think it makes sense for you to add a
> > > base::TimeTicks::UnixEpoch() static method.  Inside this method would be
the
> > > code that stores a single snapshot of the difference between
> base::Time::Now()
> > > and base::TimeTicks::Now() to produce the UnixEpoch event on the TimeTicks
> > > timeline.  This might look like the following:
> > > 
> > >   // In src/base/time.cc...
> > > 
> > >   // static
> > >   TimeTicks TimeTicks::UnixEpoch() {
> > >     static const TimeTicks ticks_unix_epoch =
> > >         TimeTicks::Now() - (Time::Now() - Time::UnixEpoch());
> > > 
> > >     return ticks_unix_epoch;
> > >   }
> > > 
> > > With this, you can now easily convert from base::TimeTicks values to NTP
> > > microseconds in media/cast code, like so:
> > > 
> > >   int64 ToNTPMicoseconds(base::TimeTicks ticks) {
> > >     static const base::TimeDelta kNtpEpochDelta =
> > >         base::TimeDelta::FromSeconds(GG_INT64_C(9435484800));
> > > 
> > >     base::TimeDelta elapsed_since_unix_epoch = ticks -
> > > base::TimeTicks::UnixEpoch();
> > >     return (elapsed_since_unix_epoch + kNtpEpochDelta).ToMicroseconds();
> > >   }
> > > 
> 
> 
> I think this would be simpler:
> int64 ToNTPMicroseconds(base::TimeTicks ticks) {
>   return (ticks - base::TimeTicks::UnixEpoch()).ToMicroseconds() +
> GG_INT64_C(9435484800000000);
> }
> 
> > > Make sense?  Benefits: 1) No PRESUBMIT violations would occur in
src/media;
> 2)
> > > Fairly clean; 3) Allows future code (anywhere in Chromium) to use
TimeTicks
> > and
> > > be able to convert to real-world time.
> > 
> > ok make sense; assuming we can get an owner of base to approve the change


That does not work :( due to UnixEpoch only exist in Time not TimeTicks that is
the core of the problem if that function existed we would not have this issue

[pwestin, 2013-10-23 23:18:15]

On 2013/10/23 22:47:50, pwestin wrote:
> On 2013/10/23 22:40:02, hubbe wrote:
> > On 2013/10/23 21:19:22, pwestin wrote:
> > > On 2013/10/23 21:08:38, Yuri wrote:
> > > > I was just chatting about this "time problem" with hubbe@.  One issue I
> > > noticed
> > > > is that the type of the object (e.g., base::Time vs base::TimeTicks)
also
> > > > implies the time origin.  Meaning, all base::TimeTicks values should be
on
> > the
> > > > same timeline with the same origin.
> > > > 
> > > > To solve the problem(s) at-hand, we think it makes sense for you to add
a
> > > > base::TimeTicks::UnixEpoch() static method.  Inside this method would be
> the
> > > > code that stores a single snapshot of the difference between
> > base::Time::Now()
> > > > and base::TimeTicks::Now() to produce the UnixEpoch event on the
TimeTicks
> > > > timeline.  This might look like the following:
> > > > 
> > > >   // In src/base/time.cc...
> > > > 
> > > >   // static
> > > >   TimeTicks TimeTicks::UnixEpoch() {
> > > >     static const TimeTicks ticks_unix_epoch =
> > > >         TimeTicks::Now() - (Time::Now() - Time::UnixEpoch());
> > > > 
> > > >     return ticks_unix_epoch;
> > > >   }
> > > > 
> > > > With this, you can now easily convert from base::TimeTicks values to NTP
> > > > microseconds in media/cast code, like so:
> > > > 
> > > >   int64 ToNTPMicoseconds(base::TimeTicks ticks) {
> > > >     static const base::TimeDelta kNtpEpochDelta =
> > > >         base::TimeDelta::FromSeconds(GG_INT64_C(9435484800));
> > > > 
> > > >     base::TimeDelta elapsed_since_unix_epoch = ticks -
> > > > base::TimeTicks::UnixEpoch();
> > > >     return (elapsed_since_unix_epoch + kNtpEpochDelta).ToMicroseconds();
> > > >   }
> > > > 
> > 
> > 
> > I think this would be simpler:
> > int64 ToNTPMicroseconds(base::TimeTicks ticks) {
> >   return (ticks - base::TimeTicks::UnixEpoch()).ToMicroseconds() +
> > GG_INT64_C(9435484800000000);
> > }
> > 
> > > > Make sense?  Benefits: 1) No PRESUBMIT violations would occur in
> src/media;
> > 2)
> > > > Fairly clean; 3) Allows future code (anywhere in Chromium) to use
> TimeTicks
> > > and
> > > > be able to convert to real-world time.
> > > 
> > > ok make sense; assuming we can get an owner of base to approve the change
> 
> 
> That does not work :( due to UnixEpoch only exist in Time not TimeTicks that
is
> the core of the problem if that function existed we would not have this issue

Thinking about this a bit more; I don't think it's a good idea to add this to
the base/time if we do that we basically open up for issues that the presubmit
script is trying to block. Seems better to just do an exception for this class.

[hubbe, 2013-10-24 00:00:54]

On 2013/10/23 23:18:15, pwestin wrote:
> On 2013/10/23 22:47:50, pwestin wrote:
> > On 2013/10/23 22:40:02, hubbe wrote:
> > > On 2013/10/23 21:19:22, pwestin wrote:
> > > > On 2013/10/23 21:08:38, Yuri wrote:
> > > > > I was just chatting about this "time problem" with hubbe@.  One issue
I
> > > > noticed
> > > > > is that the type of the object (e.g., base::Time vs base::TimeTicks)
> also
> > > > > implies the time origin.  Meaning, all base::TimeTicks values should
be
> on
> > > the
> > > > > same timeline with the same origin.
> > > > > 
> > > > > To solve the problem(s) at-hand, we think it makes sense for you to
add
> a
> > > > > base::TimeTicks::UnixEpoch() static method.  Inside this method would
be
> > the
> > > > > code that stores a single snapshot of the difference between
> > > base::Time::Now()
> > > > > and base::TimeTicks::Now() to produce the UnixEpoch event on the
> TimeTicks
> > > > > timeline.  This might look like the following:
> > > > > 
> > > > >   // In src/base/time.cc...
> > > > > 
> > > > >   // static
> > > > >   TimeTicks TimeTicks::UnixEpoch() {
> > > > >     static const TimeTicks ticks_unix_epoch =
> > > > >         TimeTicks::Now() - (Time::Now() - Time::UnixEpoch());
> > > > > 
> > > > >     return ticks_unix_epoch;
> > > > >   }
> > > > > 
> > > > > With this, you can now easily convert from base::TimeTicks values to
NTP
> > > > > microseconds in media/cast code, like so:
> > > > > 
> > > > >   int64 ToNTPMicoseconds(base::TimeTicks ticks) {
> > > > >     static const base::TimeDelta kNtpEpochDelta =
> > > > >         base::TimeDelta::FromSeconds(GG_INT64_C(9435484800));
> > > > > 
> > > > >     base::TimeDelta elapsed_since_unix_epoch = ticks -
> > > > > base::TimeTicks::UnixEpoch();
> > > > >     return (elapsed_since_unix_epoch +
kNtpEpochDelta).ToMicroseconds();
> > > > >   }
> > > > > 
> > > 
> > > 
> > > I think this would be simpler:
> > > int64 ToNTPMicroseconds(base::TimeTicks ticks) {
> > >   return (ticks - base::TimeTicks::UnixEpoch()).ToMicroseconds() +
> > > GG_INT64_C(9435484800000000);
> > > }
> > > 
> > > > > Make sense?  Benefits: 1) No PRESUBMIT violations would occur in
> > src/media;
> > > 2)
> > > > > Fairly clean; 3) Allows future code (anywhere in Chromium) to use
> > TimeTicks
> > > > and
> > > > > be able to convert to real-world time.
> > > > 
> > > > ok make sense; assuming we can get an owner of base to approve the
change
> > 
> > 
> > That does not work :( due to UnixEpoch only exist in Time not TimeTicks that
> is
> > the core of the problem if that function existed we would not have this
issue
> 
> Thinking about this a bit more; I don't think it's a good idea to add this to
> the base/time if we do that we basically open up for issues that the presubmit
> script is trying to block. Seems better to just do an exception for this
class.

No, modifying base is the right thing.
Yuri wrote that part of the presubmit script, he should know.
Also, TimeTick::UnixEpoch() will be designed to not have the same problems that
Time::UnixEpoch() has. (It will have other problems instead) so the intention of
the presubmit script will be unblemished.

[hubbe, 2013-10-24 00:03:30]

On 2013/10/24 00:00:54, hubbe wrote:
> On 2013/10/23 23:18:15, pwestin wrote:
> > On 2013/10/23 22:47:50, pwestin wrote:
> > > On 2013/10/23 22:40:02, hubbe wrote:
> > > > On 2013/10/23 21:19:22, pwestin wrote:
> > > > > On 2013/10/23 21:08:38, Yuri wrote:
> > > > > > I was just chatting about this "time problem" with hubbe@.  One
issue
> I
> > > > > noticed
> > > > > > is that the type of the object (e.g., base::Time vs base::TimeTicks)
> > also
> > > > > > implies the time origin.  Meaning, all base::TimeTicks values should
> be
> > on
> > > > the
> > > > > > same timeline with the same origin.
> > > > > > 
> > > > > > To solve the problem(s) at-hand, we think it makes sense for you to
> add
> > a
> > > > > > base::TimeTicks::UnixEpoch() static method.  Inside this method
would
> be
> > > the
> > > > > > code that stores a single snapshot of the difference between
> > > > base::Time::Now()
> > > > > > and base::TimeTicks::Now() to produce the UnixEpoch event on the
> > TimeTicks
> > > > > > timeline.  This might look like the following:
> > > > > > 
> > > > > >   // In src/base/time.cc...
> > > > > > 
> > > > > >   // static
> > > > > >   TimeTicks TimeTicks::UnixEpoch() {
> > > > > >     static const TimeTicks ticks_unix_epoch =
> > > > > >         TimeTicks::Now() - (Time::Now() - Time::UnixEpoch());
> > > > > > 
> > > > > >     return ticks_unix_epoch;
> > > > > >   }
> > > > > > 
> > > > > > With this, you can now easily convert from base::TimeTicks values to
> NTP
> > > > > > microseconds in media/cast code, like so:
> > > > > > 
> > > > > >   int64 ToNTPMicoseconds(base::TimeTicks ticks) {
> > > > > >     static const base::TimeDelta kNtpEpochDelta =
> > > > > >         base::TimeDelta::FromSeconds(GG_INT64_C(9435484800));
> > > > > > 
> > > > > >     base::TimeDelta elapsed_since_unix_epoch = ticks -
> > > > > > base::TimeTicks::UnixEpoch();
> > > > > >     return (elapsed_since_unix_epoch +
> kNtpEpochDelta).ToMicroseconds();
> > > > > >   }
> > > > > > 
> > > > 
> > > > 
> > > > I think this would be simpler:
> > > > int64 ToNTPMicroseconds(base::TimeTicks ticks) {
> > > >   return (ticks - base::TimeTicks::UnixEpoch()).ToMicroseconds() +
> > > > GG_INT64_C(9435484800000000);
> > > > }
> > > > 
> > > > > > Make sense?  Benefits: 1) No PRESUBMIT violations would occur in
> > > src/media;
> > > > 2)
> > > > > > Fairly clean; 3) Allows future code (anywhere in Chromium) to use
> > > TimeTicks
> > > > > and
> > > > > > be able to convert to real-world time.
> > > > > 
> > > > > ok make sense; assuming we can get an owner of base to approve the
> change
> > > 
> > > 
> > > That does not work :( due to UnixEpoch only exist in Time not TimeTicks
that
> > is
> > > the core of the problem if that function existed we would not have this
> issue
> > 
> > Thinking about this a bit more; I don't think it's a good idea to add this
to
> > the base/time if we do that we basically open up for issues that the
presubmit
> > script is trying to block. Seems better to just do an exception for this
> class.
> 
> No, modifying base is the right thing.
> Yuri wrote that part of the presubmit script, he should know.
> Also, TimeTick::UnixEpoch() will be designed to not have the same problems
that
> Time::UnixEpoch() has. (It will have other problems instead) so the intention
of
> the presubmit script will be unblemished.

Btw, I hope you noticed that ticks_unix_epoch is static, and thus does not make
TimeTicks non-incremental, which is what the presubmit script is meant to
assure.

[pwestin, 2013-10-24 19:42:35]

Please review this version before adding base OWNER

[miu, 2013-10-24 21:48:52]

Now that we have the new TimeTicks::UnixEpoch() function, we shouldn't be adding
a new NTP Clock class.  The cast library, as Chromium code, should just work
with base::TimeTicks values throughout.  The conversion to raw NTP int64's can
be done at the last possible moment (i.e., when sending raw values in packets). 
In general, it's important to use the TimeTicks/TimeDelta classes as much as
possible to provide compile-time checking that we're manipulating time in
semantically valid ways.

Also, regarding the use of the ToInternalValue() method: It's actually
documented not to use it for anything except serialization.  If you must do
this, you should convert to a TimeDelta, and call ToMicroseconds().  Example:
(value - TimeTicks::UnixEpoch()).ToMicroseconds() + kNTPEpochOffsetMicros

In all, I think this CL just needs to be: 1) the base::TimeTicks changes; and 2)
altering the ConvertTimeToNtp() and ConvertNtpToTime() functions in
media/cast/cast_defines.h.

https://codereview.chromium.org/34623008/diff/260001/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/34623008/diff/260001/base/time/time.cc#newcod...
base/time/time.cc:192: // static
Remove leading space.

https://codereview.chromium.org/34623008/diff/260001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/34623008/diff/260001/base/time/time.h#newcode610
base/time/time.h:610: // the application; however it might change for next run
since your TickClock
nit: For clarity, I would replace the "Note:" text with something that describes
what's happening underneath in more detail.  Like:

  Note: Upon first invocation, this function takes a snapshot of the realtime
clock to establish a reference point.  This function will return the same value
for the duration of the application, but will be different in future application
runs.

[miu, 2013-10-24 21:49:59]

The code in base/time/... ell-gee-tee-em (with one nit comment).  I think you
can loop-in a src/base OWNER on that now.

[pwestin, 2013-10-24 21:53:27]

Jim; please review the base/time/ change.

[pwestin, 2013-10-24 22:38:33]

On 2013/10/24 21:48:52, Yuri wrote:
> Now that we have the new TimeTicks::UnixEpoch() function, we shouldn't be
adding
> a new NTP Clock class.  The cast library, as Chromium code, should just work
> with base::TimeTicks values throughout.  The conversion to raw NTP int64's can
> be done at the last possible moment (i.e., when sending raw values in
packets). 
> In general, it's important to use the TimeTicks/TimeDelta classes as much as
> possible to provide compile-time checking that we're manipulating time in
> semantically valid ways.
> 
> Also, regarding the use of the ToInternalValue() method: It's actually
> documented not to use it for anything except serialization.  If you must do
> this, you should convert to a TimeDelta, and call ToMicroseconds().  Example:
> (value - TimeTicks::UnixEpoch()).ToMicroseconds() + kNTPEpochOffsetMicros
> 
> In all, I think this CL just needs to be: 1) the base::TimeTicks changes; and
2)
> altering the ConvertTimeToNtp() and ConvertNtpToTime() functions in
> media/cast/cast_defines.h.
> 
> https://codereview.chromium.org/34623008/diff/260001/base/time/time.cc
> File base/time/time.cc (right):
> 
>
https://codereview.chromium.org/34623008/diff/260001/base/time/time.cc#newcod...
> base/time/time.cc:192: // static
> Remove leading space.
> 
> https://codereview.chromium.org/34623008/diff/260001/base/time/time.h
> File base/time/time.h (right):
> 
>
https://codereview.chromium.org/34623008/diff/260001/base/time/time.h#newcode610
> base/time/time.h:610: // the application; however it might change for next run
> since your TickClock
> nit: For clarity, I would replace the "Note:" text with something that
describes
> what's happening underneath in more detail.  Like:
> 
>   Note: Upon first invocation, this function takes a snapshot of the realtime
> clock to establish a reference point.  This function will return the same
value
> for the duration of the application, but will be different in future
application
> runs.

The issue with this is that we loose the capability of faking the NTP in our
unittests

[pwestin, 2013-10-24 22:39:52]

The issue with these changes is that we loose the possibility to fake our NTP
time in our unittests.

https://codereview.chromium.org/34623008/diff/260001/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/34623008/diff/260001/base/time/time.cc#newcod...
base/time/time.cc:192: // static
On 2013/10/24 21:48:52, Yuri wrote:
> Remove leading space.

Done.

https://codereview.chromium.org/34623008/diff/260001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/34623008/diff/260001/base/time/time.h#newcode610
base/time/time.h:610: // the application; however it might change for next run
since your TickClock
On 2013/10/24 21:48:52, Yuri wrote:
> nit: For clarity, I would replace the "Note:" text with something that
describes
> what's happening underneath in more detail.  Like:
> 
>   Note: Upon first invocation, this function takes a snapshot of the realtime
> clock to establish a reference point.  This function will return the same
value
> for the duration of the application, but will be different in future
application
> runs.

Done.

[jar (doing other things), 2013-10-25 01:13:53]

https://codereview.chromium.org/34623008/diff/350001/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/34623008/diff/350001/base/time/time.cc#newcod...
base/time/time.cc:194: static const TimeTicks ticks_unix_epoch =
My guess is that you want a lazy singleton (probably can be leaky).

We don't like to have statics that are not POD (yeah... a silly TimeTicks
instance isn't POD... even though it just holds 64 bits inside).... and the code
as written is not thread safe.

https://codereview.chromium.org/34623008/diff/350001/media/cast/cast_ntp_tick...
File media/cast/cast_ntp_tick_clock.h (right):

https://codereview.chromium.org/34623008/diff/350001/media/cast/cast_ntp_tick...
media/cast/cast_ntp_tick_clock.h:48: 
nit: no need for trailing lines (ending linefeed is enough)

https://codereview.chromium.org/34623008/diff/350001/media/cast/cast_ntp_tick...
File media/cast/cast_ntp_tick_clock_unittest.cc (right):

https://codereview.chromium.org/34623008/diff/350001/media/cast/cast_ntp_tick...
media/cast/cast_ntp_tick_clock_unittest.cc:15: explicit PeerCastNtpTickClock()
{}
nit: no need for explicit.

https://codereview.chromium.org/34623008/diff/350001/media/cast/rtcp/rtcp.cc
File media/cast/rtcp/rtcp.cc (right):

https://codereview.chromium.org/34623008/diff/350001/media/cast/rtcp/rtcp.cc#...
media/cast/rtcp/rtcp.cc:276: ConvertTimeToFractions(now.ToInternalValue(),
nit: It is surprising to see ToInternalValue().  Do you really want the delta
from Time(), translated via ToMicroseconds()?

Reading the code, it sure seems like this method should have taken a TimeDelta.

[pwestin, 2013-10-30 00:37:43]

PTAL

[pwestin, 2013-10-30 18:54:18]

PTAL changed to use a LazyInstance

[miu, 2013-10-30 19:33:34]

Sorry for the delay.  This looks really good.  Almost there, with a few last
items...

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_defines....
media/cast/cast_defines.h:126: return base::TimeTicks() +
base::TimeDelta::FromMicroseconds(ntp_time_us);
This reverse conversion needs to map NTP time back to UNIX time, and then
convert to TimeTicks using TimeTicks::UnixEpoch().  Something like:

  base::TimeDelta elapsed_since_unix_epoch = base::TimeDelta::FromMicroseconds(
      ntp_time_us - (kUnixEpochInNtpSeconds *
base::Time::kMicrosecondsPerSecond));
  return base::TimeTicks::UnixEpoch() + elapsed_since_unix_epoch;

It would be helpful if there was a unit test somewhere that demonstrated these
functions are inverses of each other.  RtcpTest.NtpAndTime only tests that the
deltas are computed correctly.

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_environm...
File media/cast/cast_environment.h (right):

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_environm...
media/cast/cast_environment.h:36: // For real applications (not tests) the clock
provided should be a
Looks like you can remove this comment (or revert the file for the same effect).

https://codereview.chromium.org/34623008/diff/730001/media/cast/rtp_sender/rt...
File media/cast/rtp_sender/rtp_sender.cc (right):

https://codereview.chromium.org/34623008/diff/730001/media/cast/rtp_sender/rt...
media/cast/rtp_sender/rtp_sender.cc:124: ConvertTimeToFractions((now -
base::TimeTicks()).InMicroseconds(),
I'm confused: Did you mean to change this, or was this left over from a previous
patch?

[jar (doing other things), 2013-10-30 20:00:11]

base LGTM %nits below.

https://codereview.chromium.org/34623008/diff/730001/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/34623008/diff/730001/base/time/time.cc#newcode13
base/time/time.cc:13: //#include "base/memory/singleton.h"
nit: delete... not needed.

https://codereview.chromium.org/34623008/diff/730001/base/time/time.cc#newcod...
base/time/time.cc:205: };
nit: DISALLOW_COPY_AND_ASSIGN

[pwestin, 2013-10-30 23:14:30]

PTAL; fixed nits and comments

https://codereview.chromium.org/34623008/diff/730001/base/time/time.cc
File base/time/time.cc (right):

https://codereview.chromium.org/34623008/diff/730001/base/time/time.cc#newcode13
base/time/time.cc:13: //#include "base/memory/singleton.h"
On 2013/10/30 20:00:12, jar wrote:
> nit: delete... not needed.

Done.

https://codereview.chromium.org/34623008/diff/730001/base/time/time.cc#newcod...
base/time/time.cc:205: };
On 2013/10/30 20:00:12, jar wrote:
> nit: DISALLOW_COPY_AND_ASSIGN
> 
> 

Done.

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_defines....
media/cast/cast_defines.h:126: return base::TimeTicks() +
base::TimeDelta::FromMicroseconds(ntp_time_us);
On 2013/10/30 19:33:35, Yuri wrote:
> This reverse conversion needs to map NTP time back to UNIX time, and then
> convert to TimeTicks using TimeTicks::UnixEpoch().  Something like:
> 
>   base::TimeDelta elapsed_since_unix_epoch =
base::TimeDelta::FromMicroseconds(
>       ntp_time_us - (kUnixEpochInNtpSeconds *
> base::Time::kMicrosecondsPerSecond));
>   return base::TimeTicks::UnixEpoch() + elapsed_since_unix_epoch;
> 
> It would be helpful if there was a unit test somewhere that demonstrated these
> functions are inverses of each other.  RtcpTest.NtpAndTime only tests that the
> deltas are computed correctly.

Done.

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_environm...
File media/cast/cast_environment.h (right):

https://codereview.chromium.org/34623008/diff/730001/media/cast/cast_environm...
media/cast/cast_environment.h:36: // For real applications (not tests) the clock
provided should be a
On 2013/10/30 19:33:35, Yuri wrote:
> Looks like you can remove this comment (or revert the file for the same
effect).

Done.

https://codereview.chromium.org/34623008/diff/730001/media/cast/rtp_sender/rt...
File media/cast/rtp_sender/rtp_sender.cc (right):

https://codereview.chromium.org/34623008/diff/730001/media/cast/rtp_sender/rt...
media/cast/rtp_sender/rtp_sender.cc:124: ConvertTimeToFractions((now -
base::TimeTicks()).InMicroseconds(),
On 2013/10/30 19:33:35, Yuri wrote:
> I'm confused: Did you mean to change this, or was this left over from a
previous
> patch?

Yes you are correct, good catch

[miu, 2013-10-30 23:38:16]

lgtm

[Alpha Left Google, 2013-10-30 23:49:55]

LGTM.

[commit-bot: I haz the power, 2013-10-31 00:41:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/34623008/800001

[commit-bot: I haz the power, 2013-10-31 01:48:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-31 15:22:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/34623008/1090001

[commit-bot: I haz the power, 2013-10-31 15:26:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/34623008/1220001

[commit-bot: I haz the power, 2013-10-31 20:29:48]

Change committed as 232194