Change default timestamp to 64 bits in all webrtc directories.

Change default timestamp to 64 bits in all webrtc directories.

BUG=
R=pbos@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/82d7862fe736f851ea1cf28e8b2ce9fa6be37e45
Cr-Commit-Position: refs/heads/master@{#12646}

[honghaiz3, 2016-03-25 19:42:37]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-03-30 22:37:45]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2016-03-30 22:37:58]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2016-03-30 22:38:05]

Patchset #1 (id:60001) has been deleted

[honghaiz3, 2016-03-30 22:38:17]

Patchset #1 (id:80001) has been deleted

[honghaiz3, 2016-03-30 22:44:28]

honghaiz@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[honghaiz3, 2016-03-31 17:47:50]

honghaiz@webrtc.org changed reviewers:
+ juberti@webrtc.org

[Taylor Brandstetter, 2016-04-05 01:08:15]

Overall looks good. I'm wondering if it would make sense to use int64_t instead
of int in some places, though (for types that represent the differences between
two times). I don't think the extra bits cost much, and they might save us from
encountering a difficult-to-reproduce bug in the future.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker.cc
File webrtc/base/ratetracker.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.cc:27: bucket_start_time_milliseconds_(~0) {
It would be more clear if you replaced "~0" with "-1" here and elsewhere in the
file.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.cc:114: for (size_t i = 0;
Why was this changed from 0u? size_t is unsigned.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:56: int64_t TimeAfter(int elapsed);
Why take an int parameter instead of int64_t? Someone may accidentally try to
use this with an int64_t.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:66: inline int TimeSince(int64_t earlier) {
Similarly, why do these return ints?

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/media/base/rtpdum...
File webrtc/media/base/rtpdump.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/media/base/rtpdum...
webrtc/media/base/rtpdump.cc:32: : start_sec(static_cast<uint32_t>(start_ms /
1000)),
Why a uint32_t?

[pthatcher1, 2016-04-11 20:56:59]

I agree with Taylor about using uint64_t instead of int.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/messagequeue...
webrtc/base/messagequeue.cc:30: const int kMaxMsgLatency = 150;  // 150 ms
Why not uint64_t?

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker.cc
File webrtc/base/ratetracker.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.cc:27: bucket_start_time_milliseconds_(~0) {
On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> It would be more clear if you replaced "~0" with "-1" here and elsewhere in
the
> file.

Or, better yet, make this use rtc::optional.

Or if that's too difficult, at least have a named constant like kTimeUnset.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.cc:56: available_interval_milliseconds;
Why not just use uint64_t for all of these?

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker.h
File webrtc/base/ratetracker.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.h:58: const int bucket_milliseconds_;
Why not uint64_t?

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:56: int64_t TimeAfter(int elapsed);
On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> Why take an int parameter instead of int64_t? Someone may accidentally try to
> use this with an int64_t.

I agree.  Using int64_t consistently seems like a good idea.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils_un...
File webrtc/base/timeutils_unittest.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils_un...
webrtc/base/timeutils_unittest.cc:18: typedef int64_t TimeStamp;
Why not just search and replace TimeStamp with uint64_t?

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/media/base/rtpdum...
File webrtc/media/base/rtpdump.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/media/base/rtpdum...
webrtc/media/base/rtpdump.cc:32: : start_sec(static_cast<uint32_t>(start_ms /
1000)),
On 2016/04/05 01:08:15, Taylor Brandstetter wrote:
> Why a uint32_t?

I agree.  Why not just change to uint64_t everywhere here?

[honghaiz3, 2016-04-18 23:39:04]

I think it is not necessary to change all time-difference to 64-bit simply
because the timestamp is 64 bits. For example, in MessageQueue, when it
scheduled an event after a delay, the delay is unlikely to be more than 23 days,
so it can still use 32-bit int value. 
Let me know if you want to change all such kind of delta-time to 64-bit int.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/messagequeue...
webrtc/base/messagequeue.cc:30: const int kMaxMsgLatency = 150;  // 150 ms
On 2016/04/11 20:56:59, pthatcher1 wrote:
> Why not uint64_t?
According to style guide, we should not use uintXX only because the value does
not take negative values. 
Using uint here will require unnecessary type conversion later. 
For example if using uint64_t here, then kMaxMsgLatency + timestamp will become
uint64_t type, which cannot be directly assigned to another timestamp variable
without a type conversion.  

Using int64_t here is not necessary because the latency value does not need 64
bits. 
Basically for all delta-time, I am using int type except in a few locations
where uint32 is more appropriate for other reasons.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker.cc
File webrtc/base/ratetracker.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.cc:27: bucket_start_time_milliseconds_(~0) {
On 2016/04/11 20:56:59, pthatcher1 wrote:
> On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> > It would be more clear if you replaced "~0" with "-1" here and elsewhere in
> the
> > file.
> 
> Or, better yet, make this use rtc::optional.
> 
> Or if that's too difficult, at least have a named constant like kTimeUnset.
> 
Used a named constant kTimeUnset.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.cc:56: available_interval_milliseconds;
On 2016/04/11 20:56:59, pthatcher1 wrote:
> Why not just use uint64_t for all of these?

Done.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.cc:114: for (size_t i = 0;
On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> Why was this changed from 0u? size_t is unsigned.
I do not think the "u" here is necessary. I would only add this to make the
compiler happy. But the compiler is OK without the "u". See an example here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker.h
File webrtc/base/ratetracker.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.h:58: const int bucket_milliseconds_;
On 2016/04/11 20:56:59, pthatcher1 wrote:
> Why not uint64_t?
As explained in the other comment, I do not use uintXX here following the style
guide. 
It is highly unlikely that a call duration will overflow an integer value (more
than 23 days for a signed int), so I guess int is sufficient.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:56: int64_t TimeAfter(int elapsed);
On 2016/04/11 20:56:59, pthatcher1 wrote:
> On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> > Why take an int parameter instead of int64_t? Someone may accidentally try
to
> > use this with an int64_t.
> 
> I agree.  Using int64_t consistently seems like a good idea.

Done.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:66: inline int TimeSince(int64_t earlier) {
On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> Similarly, why do these return ints?
For all delta-time (e.g. elapsed time, or time delay), I am using 32-bit values
because it is highly unlikely that any time difference in a call will be more
than 23 days (with int 32 bits). 
Returning an int value simplifies the places where TimeSince is called and the
result value is passed to an int variable.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils_un...
File webrtc/base/timeutils_unittest.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils_un...
webrtc/base/timeutils_unittest.cc:18: typedef int64_t TimeStamp;
On 2016/04/11 20:56:59, pthatcher1 wrote:
> Why not just search and replace TimeStamp with uint64_t?

Done.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/media/base/rtpdum...
File webrtc/media/base/rtpdump.cc (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/media/base/rtpdum...
webrtc/media/base/rtpdump.cc:32: : start_sec(static_cast<uint32_t>(start_ms /
1000)),
On 2016/04/11 20:56:59, pthatcher1 wrote:
> On 2016/04/05 01:08:15, Taylor Brandstetter wrote:
> > Why a uint32_t?
> 
> I agree.  Why not just change to uint64_t everywhere here?
RtpDumpFileHeader defines the format of the RtpDumpFile header and the output
may be used elsewhere. If we change the structure here, it may cause issue when
other apps (like wireshark) try to read the dumped file. 
So I think we should avoid changing it.

[Taylor Brandstetter, 2016-04-19 20:35:27]

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker.h
File webrtc/base/ratetracker.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.h:58: const int bucket_milliseconds_;
On 2016/04/18 23:39:04, honghaiz3 wrote:
> On 2016/04/11 20:56:59, pthatcher1 wrote:
> > Why not uint64_t?
> As explained in the other comment, I do not use uintXX here following the
style
> guide. 
> It is highly unlikely that a call duration will overflow an integer value
(more
> than 23 days for a signed int), so I guess int is sufficient. 

I totally agree about using signed values. But I don't think it hurts to use a
64-bit value. I'm sure there are some niche use cases where WebRTC is used for
several days without stopping (there was even a bug recently about a problem
that occurs after 2 weeks).

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:66: inline int TimeSince(int64_t earlier) {
On 2016/04/18 23:39:04, honghaiz3 wrote:
> On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> > Similarly, why do these return ints?
> For all delta-time (e.g. elapsed time, or time delay), I am using 32-bit
values
> because it is highly unlikely that any time difference in a call will be more
> than 23 days (with int 32 bits). 
> Returning an int value simplifies the places where TimeSince is called and the
> result value is passed to an int variable. 
> 

See my other comment.

[honghaiz3, 2016-04-22 23:03:35]

Patchset #4 (id:160001) has been deleted

[honghaiz3, 2016-04-22 23:03:49]

Patchset #4 (id:180001) has been deleted

[honghaiz3, 2016-04-22 23:41:18]

Patchset #4 (id:200001) has been deleted

[honghaiz3, 2016-04-22 23:41:30]

Patchset #4 (id:220001) has been deleted

[honghaiz3, 2016-04-22 23:45:21]

I had the intention to change all delta time to int64_t, especially those in
MessageQueue, but then I realized that those time delay methods eventually call
WSAWaitForMultipleEvents in windows which supports only 32-bit wait time. So at
the end, I just leave those delay time to 32 bits. 
PTAL.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker.h
File webrtc/base/ratetracker.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/ratetracker....
webrtc/base/ratetracker.h:58: const int bucket_milliseconds_;
On 2016/04/19 20:35:27, Taylor Brandstetter wrote:
> On 2016/04/18 23:39:04, honghaiz3 wrote:
> > On 2016/04/11 20:56:59, pthatcher1 wrote:
> > > Why not uint64_t?
> > As explained in the other comment, I do not use uintXX here following the
> style
> > guide. 
> > It is highly unlikely that a call duration will overflow an integer value
> (more
> > than 23 days for a signed int), so I guess int is sufficient. 
> 
> I totally agree about using signed values. But I don't think it hurts to use a
> 64-bit value. I'm sure there are some niche use cases where WebRTC is used for
> several days without stopping (there was even a bug recently about a problem
> that occurs after 2 weeks).

Done.

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1835053002/diff/100001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:66: inline int TimeSince(int64_t earlier) {
On 2016/04/19 20:35:27, Taylor Brandstetter wrote:
> On 2016/04/18 23:39:04, honghaiz3 wrote:
> > On 2016/04/05 01:08:14, Taylor Brandstetter wrote:
> > > Similarly, why do these return ints?
> > For all delta-time (e.g. elapsed time, or time delay), I am using 32-bit
> values
> > because it is highly unlikely that any time difference in a call will be
more
> > than 23 days (with int 32 bits). 
> > Returning an int value simplifies the places where TimeSince is called and
the
> > result value is passed to an int variable. 
> > 
> 
> See my other comment.

Done.

[honghaiz3, 2016-04-27 16:13:24]

honghaiz@webrtc.org changed reviewers:
+ stefan@webrtc.org, tommi@webrtc.org

[pthatcher1, 2016-05-02 16:57:57]

lgtm, with a few minor nits

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:1616: cmsElapsed =
static_cast<int>(TimeSince(msStart));
Can you leave a comment about why we do this and why we think it's safe?

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:36: // TODO(honghaiz): remove this and replace the call
with Time().
I think it's OK to leave this so callers can be explicit when they want to be.

If you don't, at least update the comment to "Remove" instead of "remove".

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/media/base/rtpdum...
File webrtc/media/base/rtpdump.cc (right):

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/media/base/rtpdum...
webrtc/media/base/rtpdump.cc:401: if (delay >= warn_slow_writes_delay_) {
Why not change warn_slow_writes_delay_ to a uint64_t also?  That way, we
wouldn't need this cast.

[honghaiz3, 2016-05-02 22:26:06]

Patchset #7 (id:300001) has been deleted

[honghaiz3, 2016-05-02 22:26:15]

Patchset #6 (id:280001) has been deleted

[honghaiz3, 2016-05-02 22:26:35]

Patchset #5 (id:260001) has been deleted

[honghaiz3, 2016-05-02 22:26:50]

Patchset #5 (id:320001) has been deleted

[honghaiz3, 2016-05-03 01:01:33]

Patchset #5 (id:340001) has been deleted

[honghaiz3, 2016-05-03 01:01:47]

Patchset #5 (id:360001) has been deleted

[honghaiz3, 2016-05-03 01:01:59]

Patchset #5 (id:380001) has been deleted

[honghaiz3, 2016-05-03 21:15:24]

Patchset #5 (id:400001) has been deleted

[honghaiz3, 2016-05-03 21:18:26]

I merged this with the CL to replace Time64 with TimeMillis and removed all
calls to rtc::Time although still keep the method there for a while to keep
third-party backward compatibility. 
Peter, can you take another look? 
Or I will land this tomorrow.

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/physicalsock...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/physicalsock...
webrtc/base/physicalsocketserver.cc:1616: cmsElapsed =
static_cast<int>(TimeSince(msStart));
On 2016/05/02 16:57:56, pthatcher1 wrote:
> Can you leave a comment about why we do this and why we think it's safe?
Just used int64_t instead. 
Deferred the type conversion to the call WSAWaitForMultipleEvents

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:36: // TODO(honghaiz): remove this and replace the call
with Time().
On 2016/05/02 16:57:56, pthatcher1 wrote:
> I think it's OK to leave this so callers can be explicit when they want to be.
> 
> If you don't, at least update the comment to "Remove" instead of "remove".

Removed this and replaced with TimeMillis()

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/media/base/rtpdum...
File webrtc/media/base/rtpdump.cc (right):

https://codereview.webrtc.org/1835053002/diff/240001/webrtc/media/base/rtpdum...
webrtc/media/base/rtpdump.cc:401: if (delay >= warn_slow_writes_delay_) {
On 2016/05/02 16:57:56, pthatcher1 wrote:
> Why not change warn_slow_writes_delay_ to a uint64_t also?  That way, we
> wouldn't need this cast.

Done.

[honghaiz3, 2016-05-04 20:58:15]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-05-04 20:58:16]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1835053002/#ps440001 (title:
"Changed PostAt method in MessageQueue so that it won't break Chrome build")

[commit-bot: I haz the power, 2016-05-04 20:58:22]

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

[commit-bot: I haz the power, 2016-05-04 21:05:32]

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

[commit-bot: I haz the power, 2016-05-04 21:05:33]

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

[honghaiz3, 2016-05-04 21:24:53]

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

[commit-bot: I haz the power, 2016-05-04 21:25:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835053002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835053002/460001

[honghaiz3, 2016-05-04 22:33:12]

honghaiz@webrtc.org changed reviewers:
+ henrikg@webrtc.org, pbos@webrtc.org

[honghaiz3, 2016-05-04 22:33:14]

pbos@ and henrikg@, 
Can you take a look at the CL? 
It needs an owner review for webrtc/video and webrtc/voice_engine dirs.

[pbos-webrtc, 2016-05-05 15:43:56]

lgtm for webrtc/video

https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2935: int32_t timestamp_gap = timestamp -
latest_observed;
These are uint32_ts and its correctness is probably implementation-defined on
wraparounds.

Though with that said it looks like this was current behavior already since
rtc::TimeDiff was just a - b. Feel free to add a note/TODO.

[honghaiz3, 2016-05-05 17:25:14]

honghaiz@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[honghaiz3, 2016-05-05 17:25:16]

solenberg@ or herikg@,

Can one of you take a look at the CL?
I need an owner review for the dir webrtc/voice_engine. 


pbos@, Thanks for the review!

https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2935: int32_t timestamp_gap = timestamp -
latest_observed;
On 2016/05/05 15:43:56, pbos-webrtc wrote:
> These are uint32_ts and its correctness is probably implementation-defined on
> wraparounds.
> 
> Though with that said it looks like this was current behavior already since
> rtc::TimeDiff was just a - b. Feel free to add a note/TODO.
AFAIK, when you subtract two uint32_ts it will correctly return an int value
without wraparound handling. For example 1u - 2u => (int)1. The only catch is to
make sure the resulting variable to be int (int32) type. That is the reason of
recent change of rtc::TimeDiff. Added a short note.

[the sun, 2016-05-06 09:16:00]

On 2016/05/05 17:25:16, honghaiz3 wrote:
> solenberg@ or herikg@,
> 
> Can one of you take a look at the CL?
> I need an owner review for the dir webrtc/voice_engine. 
> 
> 
> pbos@, Thanks for the review!
> 
>
https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
> File webrtc/video/end_to_end_tests.cc (right):
> 
>
https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
> webrtc/video/end_to_end_tests.cc:2935: int32_t timestamp_gap = timestamp -
> latest_observed;
> On 2016/05/05 15:43:56, pbos-webrtc wrote:
> > These are uint32_ts and its correctness is probably implementation-defined
on
> > wraparounds.
> > 
> > Though with that said it looks like this was current behavior already since
> > rtc::TimeDiff was just a - b. Feel free to add a note/TODO.
> AFAIK, when you subtract two uint32_ts it will correctly return an int value
> without wraparound handling. For example 1u - 2u => (int)1. The only catch is
to
> make sure the resulting variable to be int (int32) type. That is the reason of
> recent change of rtc::TimeDiff. Added a short note.

LGTM for webrtc/voice_engine.

When we're going through the trouble to find all the places where we use a
uint32_t to represent time, would it be a good idea to not replace it with
another generic type name, but with something like:
"typedef int64_t MilliSecond" ?

Even if not achieving type safety, the code would be more self-documenting, and
it would make it easier to search for places where we rely on the timeutils.h

[honghaiz3, 2016-05-06 17:18:13]

On 2016/05/06 09:16:00, the sun wrote:
> On 2016/05/05 17:25:16, honghaiz3 wrote:
> > solenberg@ or herikg@,
> > 
> > Can one of you take a look at the CL?
> > I need an owner review for the dir webrtc/voice_engine. 
> > 
> > 
> > pbos@, Thanks for the review!
> > 
> >
>
https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
> > File webrtc/video/end_to_end_tests.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1835053002/diff/480001/webrtc/video/end_to_end_...
> > webrtc/video/end_to_end_tests.cc:2935: int32_t timestamp_gap = timestamp -
> > latest_observed;
> > On 2016/05/05 15:43:56, pbos-webrtc wrote:
> > > These are uint32_ts and its correctness is probably implementation-defined
> on
> > > wraparounds.
> > > 
> > > Though with that said it looks like this was current behavior already
since
> > > rtc::TimeDiff was just a - b. Feel free to add a note/TODO.
> > AFAIK, when you subtract two uint32_ts it will correctly return an int value
> > without wraparound handling. For example 1u - 2u => (int)1. The only catch
is
> to
> > make sure the resulting variable to be int (int32) type. That is the reason
of
> > recent change of rtc::TimeDiff. Added a short note.
> 
> LGTM for webrtc/voice_engine.
> 
> When we're going through the trouble to find all the places where we use a
> uint32_t to represent time, would it be a good idea to not replace it with
> another generic type name, but with something like:
> "typedef int64_t MilliSecond" ?
> 
> Even if not achieving type safety, the code would be more self-documenting,
and
> it would make it easier to search for places where we rely on the timeutils.h
Thanks. I will leave this at a separate CL. 
Making this change in the current CL does not make the work easier in any way. 
We still need to look for all occurrences of int64_t in all the affected files
and even other files not affected in this CL because
in some places we have already used int64_t as the timestamp.

[honghaiz3, 2016-05-06 17:25:08]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-05-06 17:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, pbos@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1835053002/#ps520001 (title:
"Add TODO for timestamp.")

[commit-bot: I haz the power, 2016-05-06 17:25:21]

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

[honghaiz3, 2016-05-06 18:29:33]

Description was changed from

==========
Change default timestamp to 64 bits in all webrtc directories.

BUG=
==========

to

==========
Change default timestamp to 64 bits in all webrtc directories.

BUG=
R=pbos@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org

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

[honghaiz3, 2016-05-06 18:29:36]

Committed patchset #10 (id:520001) manually as
82d7862fe736f851ea1cf28e8b2ce9fa6be37e45 (presubmit successful).

[commit-bot: I haz the power, 2016-05-06 18:29:37]

Description was changed from

==========
Change default timestamp to 64 bits in all webrtc directories.

BUG=
R=pbos@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org

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

to

==========
Change default timestamp to 64 bits in all webrtc directories.

BUG=
R=pbos@webrtc.org, pthatcher@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/82d7862fe736f851ea1cf28e8b2ce9fa6be37e45
Cr-Commit-Position: refs/heads/master@{#12646}
==========