Change rtc::TimeNanos and rtc::TimeMicros return value from uint64_t to int64_t.

Change rtc::TimeNanos and rtc::TimeMicros return value from uint64_t to int64_t.

Also updated types close to call sites.

BUG=webrtc:6733
Committed: https://crrev.com/deb95f32f47ea09e10a8c7c209dca3b9dce7f3d8
Cr-Commit-Position: refs/heads/master@{#15255}

[nisse-webrtc, 2016-11-18 14:52:42]

nisse@webrtc.org changed reviewers:
+ hbos@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org

[nisse-webrtc, 2016-11-18 15:21:41]

Seems the change needs synchronization with chrome, where the cast at 

https://codesearch.chromium.org/chromium/src/remoting/protocol/webrtc_video_r...

should be deleted, but it can't be until this change is landed.

[hbos, 2016-11-18 15:23:41]

lgtm provided it does not break chromium

https://codereview.webrtc.org/2514553003/diff/1/webrtc/base/fakeclock.h
File webrtc/base/fakeclock.h (right):

https://codereview.webrtc.org/2514553003/diff/1/webrtc/base/fakeclock.h#newco...
webrtc/base/fakeclock.h:40: int64_t time_ GUARDED_BY(lock_) = 0u;
nit: 0

[the sun, 2016-11-18 16:39:44]

lgtm

[pthatcher1, 2016-11-18 22:11:16]

lgtm

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:69: // TODO(nisse): The below looks very
wrong.
Why?

[nisse-webrtc, 2016-11-21 07:45:07]

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:69: // TODO(nisse): The below looks very
wrong.
On 2016/11/18 22:11:16, pthatcher1 wrote:
> Why?

It's wrong in several ways. 

With struct timeval representation, we should have 

 nanosecs == tv_sec * kNumNanoSecsPerSec +
             tv_usec * kNumNanoSecsPerMicroSec

with tv_usec in the range [0, 999999].

The most severe error is that the assignment to tv_usec includes the seconds
already accounted for, and in addition, the result if the division most likely
doesn't fit in 32 bits, and truncation makes the value even worse.

It should be

  tv_usec = (time % kNumNanoSecsPerSec) /
             kNumNanoSecsPerMicroSec;

Second, with the above fix, we get bad values if time happens to be negative,
because integer division in C truncates to zero, rather than producing the
floor, resulting in tv_usec < 0. (That wasn't an issue before the change in this
cl).

Third, speaking of the sign of time, tv_sec is usually expected to be seconds
since the unix 1970 epoch, while the epoch used for TimeNanos is arbitrary,
e.g., uptime. If TimeCallback is only used for calculation of intervals,
timeouts and the like, that's no problem, and actually an improvement over
gettimeofday, since the latter jumps whenever the system time is changed. But
even so, using struct timeval seems confusing.

[nisse-webrtc, 2016-11-21 11:02:35]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 11:02:36]

Dry run: 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, 2016-11-21 11:27:29]

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

[commit-bot: I haz the power, 2016-11-21 11:27:30]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-11-21 11:48:07]

nisse@webrtc.org changed reviewers:
+ henrikg@webrtc.org, mflodman@webrtc.org, sprang@webrtc.org

[nisse-webrtc, 2016-11-21 11:48:07]

OWNER's approval needed:

henrikg: audio_device
mflodman: video_capture
sprang: videoprocessor

Some types are changed from uint64_t to int64_t, quite trivial. Review
appreciated, otherwise, I won't hesitate to TBR those changes.

[sprang_webrtc, 2016-11-21 12:05:31]

lgtm for videoprocessor

[kwiberg-webrtc, 2016-11-21 12:12:34]

kwiberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2016-11-21 12:12:34]

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/api/stats/rtcstats...
File webrtc/api/stats/rtcstatsreport.h (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/api/stats/rtcstats...
webrtc/api/stats/rtcstatsreport.h:60: int64_t timestamp_us() const { return
timestamp_us_; }
These are also changes in the public API, right? See the other comment.

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:69: // TODO(nisse): The below looks very
wrong.
On 2016/11/21 07:45:07, nisse-webrtc wrote:
> On 2016/11/18 22:11:16, pthatcher1 wrote:
> > Why?
> 
> It's wrong in several ways. 
> 
> With struct timeval representation, we should have 
> 
>  nanosecs == tv_sec * kNumNanoSecsPerSec +
>              tv_usec * kNumNanoSecsPerMicroSec
> 
> with tv_usec in the range [0, 999999].
> 
> The most severe error is that the assignment to tv_usec includes the seconds
> already accounted for, and in addition, the result if the division most likely
> doesn't fit in 32 bits, and truncation makes the value even worse.
> 
> It should be
> 
>   tv_usec = (time % kNumNanoSecsPerSec) /
>              kNumNanoSecsPerMicroSec;
> 
> Second, with the above fix, we get bad values if time happens to be negative,
> because integer division in C truncates to zero, rather than producing the
> floor, resulting in tv_usec < 0. (That wasn't an issue before the change in
this
> cl).
> 
> Third, speaking of the sign of time, tv_sec is usually expected to be seconds
> since the unix 1970 epoch, while the epoch used for TimeNanos is arbitrary,
> e.g., uptime. If TimeCallback is only used for calculation of intervals,
> timeouts and the like, that's no problem, and actually an improvement over
> gettimeofday, since the latter jumps whenever the system time is changed. But
> even so, using struct timeval seems confusing.

Post a bug about this and point to it in the comment?

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/timeutils.h#n...
webrtc/base/timeutils.h:37: virtual int64_t TimeNanos() const = 0;
With this (and the other changes to this file), you're changing the public API,
right? If so, shouldn't you (1) introduce new stuff, (2) announce that people
have X weeks to stop using the old stuff, (3) wait X weeks, (4) remove the old
stuff?

[Henrik Grunell WebRTC, 2016-11-21 12:30:43]

webrtc/modules/audio_device/ lgtm

[nisse-webrtc, 2016-11-21 12:39:12]

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/api/stats/rtcstats...
File webrtc/api/stats/rtcstatsreport.h (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/api/stats/rtcstats...
webrtc/api/stats/rtcstatsreport.h:60: int64_t timestamp_us() const { return
timestamp_us_; }
On 2016/11/21 12:12:34, kwiberg-webrtc wrote:
> These are also changes in the public API, right? See the other comment.

If at all possible, I'd like to change the return type *without* renaming the
method (which as far as I understand is the only way to transition gradually).

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:69: // TODO(nisse): The below looks very
wrong.
On 2016/11/21 12:12:34, kwiberg-webrtc wrote:
> On 2016/11/21 07:45:07, nisse-webrtc wrote:
> > On 2016/11/18 22:11:16, pthatcher1 wrote:
> > > Why?
> > 
> > It's wrong in several ways. 
> > 
> > With struct timeval representation, we should have 
> > 
> >  nanosecs == tv_sec * kNumNanoSecsPerSec +
> >              tv_usec * kNumNanoSecsPerMicroSec
> > 
> > with tv_usec in the range [0, 999999].
> > 
> > The most severe error is that the assignment to tv_usec includes the seconds
> > already accounted for, and in addition, the result if the division most
likely
> > doesn't fit in 32 bits, and truncation makes the value even worse.
> > 
> > It should be
> > 
> >   tv_usec = (time % kNumNanoSecsPerSec) /
> >              kNumNanoSecsPerMicroSec;
> > 
> > Second, with the above fix, we get bad values if time happens to be
negative,
> > because integer division in C truncates to zero, rather than producing the
> > floor, resulting in tv_usec < 0. (That wasn't an issue before the change in
> this
> > cl).
> > 
> > Third, speaking of the sign of time, tv_sec is usually expected to be
seconds
> > since the unix 1970 epoch, while the epoch used for TimeNanos is arbitrary,
> > e.g., uptime. If TimeCallback is only used for calculation of intervals,
> > timeouts and the like, that's no problem, and actually an improvement over
> > gettimeofday, since the latter jumps whenever the system time is changed.
But
> > even so, using struct timeval seems confusing.
> 
> Post a bug about this and point to it in the comment?

Filed bug https://bugs.chromium.org/p/webrtc/issues/detail?id=6737

I'll consider just removing this TODO, since it's not really related to the rest
of the cl.

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/2514553003/diff/40001/webrtc/base/timeutils.h#n...
webrtc/base/timeutils.h:37: virtual int64_t TimeNanos() const = 0;
On 2016/11/21 12:12:34, kwiberg-webrtc wrote:
> With this (and the other changes to this file), you're changing the public
API,
> right? If so, shouldn't you (1) introduce new stuff, (2) announce that people
> have X weeks to stop using the old stuff, (3) wait X weeks, (4) remove the old
> stuff?

What's "public" in base/ is a fuzzy line. This ClockInterface is intended for
testing only. I've found two uses outside of webrtc (which I think I was
involved in adding), and I think they are under control; they're there mainly
because ScopedFakeClock isn't imported into the projects in question.

TimeMicros and TimeNanos are potentially more of a problem. We could introduce
new names for them, but that would also add some confusion. My opionion is that
in the long run, it's more hassle to update all applications to use a new name,
than to update them to accept a different return type; if for no other reason, a
lot fewer of the call sites will require any changes. (In practice, I think we
can almost promise that the returned values will be representable in both
int64_t and uint64_t).

[kwiberg-webrtc, 2016-11-23 14:19:45]

lgtm

[nisse-webrtc, 2016-11-25 08:14:45]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 08:14:56]

Dry run: 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, 2016-11-25 08:30:26]

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

[commit-bot: I haz the power, 2016-11-25 08:30:27]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-11-28 09:32:28]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-11-28 09:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org,
sprang@webrtc.org, kwiberg@webrtc.org, solenberg@webrtc.org, henrikg@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2514553003/#ps60001 (title:
"Rebased, on top of fixed BoringSSL TimeCallback.'")

[commit-bot: I haz the power, 2016-11-28 09:32:37]

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, 2016-11-28 09:54:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480325548059570,
"parent_rev": "8a2404ddad9faa77fa7141860820701a134174d1", "commit_rev":
"53335ca7a2ddb0e264cb5c2658c81f07cbfa7d6f"}

[commit-bot: I haz the power, 2016-11-28 09:54:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-28 09:55:08]

Description was changed from

==========
Change rtc::TimeNanos and rtc::TimeMicros return value from uint64_t to int64_t.

Also updated types close to call sites.

BUG=webrtc:6733
==========

to

==========
Change rtc::TimeNanos and rtc::TimeMicros return value from uint64_t to int64_t.

Also updated types close to call sites.

BUG=webrtc:6733

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

[commit-bot: I haz the power, 2016-11-28 09:55:09]

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