Use microsecond timestamp in cricket::VideoFrame.

Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740
Committed: https://crrev.com/f30ba114bb33dd1d8643bc640dda2e0c86dbbd32
Cr-Commit-Position: refs/heads/master@{#12348}

Committed: https://crrev.com/b17712ff894b0bb06c36d12354f0a12def11ee81
Cr-Commit-Position: refs/heads/master@{#12358}

[nisse-webrtc, 2016-04-07 13:09:39]

nisse@webrtc.org changed reviewers:
+ pbos@webrtc.org, perkj@webrtc.org

[nisse-webrtc, 2016-04-07 13:09:40]

This cl implements step 1 of the grand plan. If we can agree this is the right
direction, I need to add some unit tests, calling OnFrameCaptured with frames
with increasing timestamps unrelated to current time, and have a registered sink
check that the received sequence of cricket::VideoFrame have timestamps with the
same intervals, but with a starting time close to rtc::TimeMicros.

Any suggestion on where such a test belongs?

[perkj_webrtc, 2016-04-08 08:22:49]

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:227: int64_t timestamp_us =
do this in the frame factory instead.

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:421: broadcaster_.OnFrame(*frame);
Unfortunately there are capturers that does not use neither the OnFrameCaptured
or the frame factory. ie- the Chromoting capturer. 

It directly calls  OnFrame:
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.h:297: // Same value as input_size_valid_ below,
but different locking.
?

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:62: virtual void set_timestamp_us(int64_t
time_us) = 0;
no need for set_timestamp. See motivation below.

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:71: virtual void SetTimeStamp(int64_t time_ns) {
Can we deprecate this? Creating VideoFrames is very cheap since they just
reference a buffer. So I don't see the need why it has to be modified.

[nisse-webrtc, 2016-04-08 09:15:44]

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:227: int64_t timestamp_us =
On 2016/04/08 08:22:48, perkj_webrtc wrote:
> do this in the frame factory instead. 

Moving the logic there makes sense, I'll give it a try. Main question is how it
affects chrome's farme factory, but hopefully it has usec timestamps in the
right timescale already.

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:421: broadcaster_.OnFrame(*frame);
On 2016/04/08 08:22:48, perkj_webrtc wrote:
> Unfortunately there are capturers that does not use neither the
OnFrameCaptured
> or the frame factory. ie- the Chromoting capturer. 
> 
> It directly calls  OnFrame:
>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...


That code then has to produce the right timestamps. Do you know what chrome's
TimeTicks::Now() produces? For this to work out easily, it has to be the same as
rtc::TimeMicros().

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.h:297: // Same value as input_size_valid_ below,
but different locking.
On 2016/04/08 08:22:49, perkj_webrtc wrote:
> ?

Both timestamp_offset_valid and input_size_valid have the same meaning: Have we
seen the first frame yet? But we can't really use a shared flag, because the
timestamp offset logic shouldn't touch the frame_stats_crit_ lock.

Anyway, this is moot if the logic is moved to the frame factory.

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:71: virtual void SetTimeStamp(int64_t time_ns) {
On 2016/04/08 08:22:49, perkj_webrtc wrote:
> Can we deprecate this? Creating VideoFrames is very cheap since they just
> reference a buffer. So I don't see the need why it has to be modified.

Typical use, in videoframefactory,

    output_frame_->SetTimeStamp(cropped_input_frame->GetTimeStamp());

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

I don't really like making new copies just to set the timestamp. If we have to
choose between either set_timestamp, or MakeCopyWithTimestamp, I'd prefer the
former. So to get rid of timestamp setters, we should add timestamp argument to
all methods that create frames.

[nisse-webrtc, 2016-04-08 11:46:35]

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

[commit-bot: I haz the power, 2016-04-08 11:46:40]

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

[nisse-webrtc, 2016-04-08 11:48:45]

Dropped time offset and conversion for now.

Instead, use CapturedFrame timestamps as is, and add a DCHECK that they are
close to the rtc::Time clock. If that ever fails, revisit conversion.

[commit-bot: I haz the power, 2016-04-08 12:05:08]

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

[commit-bot: I haz the power, 2016-04-08 12:05:09]

Dry run: Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/14082)

[perkj_webrtc, 2016-04-08 14:52:18]

looks like there is one more test to update.
Thinking about it I wonder if the the DCHECK for ts will work in all webrtc unit
tests where a fake clock is used.
Humm- I wonder how to do this....

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:70: // Also used as a helper method for
WebRtcVideoFrame methods with ns input.
nit : Weird description.

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:486: // TODO(nisse): This
test should be no longer needed when the
why not?

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:33: // setting the timestamp?
I hope not... 
Can't you just create a new ctor where you switch the order of rotation and
time_stamp_ns?

[nisse-webrtc, 2016-04-11 06:58:57]

About the DCHECK, I think it should probably be deleted before landing, and
replaced by a LOG(LS_WARN).

I haven't yet investigated the test failures. For the test code I have modified,
my approach have been to set initial timestamps to rtc::TimeMicros. I hope that
should be possible for all tests.

For clocks and testing in general, I think I would like to have a global clock
(like rtc::Time()), but with some hooks to insert a simulated clock while
testing. Passing around a webrtc::Clock * to everything (like some parts of the
code do) is unnecessary clutter. 

Since we compare timestamps created in different parts of the code, those parts
have to use the same clock. We can't really have different parts use different
clocks, neither in testing or otherwise.

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:70: // Also used as a helper method for
WebRtcVideoFrame methods with ns input.
On 2016/04/08 14:52:18, perkj_webrtc wrote:
> nit : Weird description.

Deleted.

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:486: // TODO(nisse): This
test should be no longer needed when the
On 2016/04/08 14:52:18, perkj_webrtc wrote:
> why not? 

Because it should be the responsibility of whoever creates the frames to use
rtc::TimeMicros timescale, which by definition is increasing. And then there's
no need for the videoengine to check for or fix jumps in that time stamp.

I'm not 100% sure we can get rid of all time jump hacks, though. One mostly
hypothetical case is a camera with a free running clock. Then the if the
capturer determines a fix offset on first frame and applies that as frames are
created (like in a previous iteration of this cl), then as the the cameras clock
slowly drifts away, we will get smallish time jumps when switching between this
source and other sources which use a properly synchronized clock.

We may also be small jumps at source switch due do different delays for the two
sources (or simply races) between the frames being captured, and they reach
WebRtcVideoSendStream::OnFrame.

If downstream code is confused by small time jumps, yet another alternative may
be to record the timestamp of the latest seen frame and enforce increasing
timestamps by simply dropping frames which are too early. Or setting timestamp
to last_timestamp+1 until it catches up.

But I don't think it's appropriate for the videoengine to compute and apply an
offset to all frames. That will break lipsync to audio channel using
rtc::TimeMicros timestamps. And when removing that, this test should be deleted
or significantly revamped.

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:33: // setting the timestamp?
On 2016/04/08 14:52:18, perkj_webrtc wrote:
> I hope not... 
> Can't you just create a new ctor where you switch the order of rotation and
> time_stamp_ns? 

That should work. Sounds a little hackish, but maybe that's the least bad way of
transitioning?

[nisse-webrtc, 2016-04-12 09:56:46]

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

[commit-bot: I haz the power, 2016-04-12 09:56:48]

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

[commit-bot: I haz the power, 2016-04-12 09:57:38]

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

[commit-bot: I haz the power, 2016-04-12 09:57:39]

Dry run: Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/6600)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/6591)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8923)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8843)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14131)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13765)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2240)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8466)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14190)

[nisse-webrtc, 2016-04-12 10:10:26]

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

[commit-bot: I haz the power, 2016-04-12 10:10:36]

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

[nisse-webrtc, 2016-04-12 11:05:45]

The bug triggered in patchset 3 was the initialization of
first_frame_timestamp_ms_. Zero was interpreted as "uninitialized", but after
these changes, the initialization 

  first_frame_timestamp_ms_ = rtc::Time() - frame_delta_ms;

is more likely to produce a zero value, and then time stamping of the next frame
is messed up.

[nisse-webrtc, 2016-04-12 12:30:45]

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

[commit-bot: I haz the power, 2016-04-12 12:30:57]

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

[commit-bot: I haz the power, 2016-04-12 14:31:36]

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

[commit-bot: I haz the power, 2016-04-12 14:31:37]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[perkj_webrtc, 2016-04-13 05:57:06]

looks good. You should probably remove the DCHECK for ts before landing and test
this in chrome and talk to the chromoting / tab capture / screen capture or at
least test with screen share in hangouts before having it on in Chrome.

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1517:
first_frame_timestamp_valid_(false),
why this change?

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:31: // constructor and init methods.
Maybe it's easiest to drop the
Not really a problem in this class since its being deprecated. But in the end I
would prefer set on ctor and no way of changing it.

[nisse-webrtc, 2016-04-13 06:54:50]

Dropped the DCHECK. About the various capturers on chrome, who should I
coordinate with?

I think the change is reasonably safe as is, since there's not yet any code
depending on the timestamp_us field using the right epoch.

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1517:
first_frame_timestamp_valid_(false),
On 2016/04/13 05:57:06, perkj_webrtc wrote:
> why this change? 

Zero is a valid timestamp offset, we need a separate flag to tell if the offset
has been set yet or not.

[pbos-webrtc, 2016-04-13 09:56:24]

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1517:
first_frame_timestamp_valid_(false),
On 2016/04/13 06:54:50, nisse-webrtc wrote:
> On 2016/04/13 05:57:06, perkj_webrtc wrote:
> > why this change? 
> 
> Zero is a valid timestamp offset, we need a separate flag to tell if the
offset
> has been set yet or not.

Sounds like we should use rtc::Optional then, or agree to not use rtc::Optional
elsewhere, it feels weird to mix styles in this code.

[nisse-webrtc, 2016-04-13 11:30:47]

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1517:
first_frame_timestamp_valid_(false),
On 2016/04/13 09:56:24, pbos-webrtc wrote:
> On 2016/04/13 06:54:50, nisse-webrtc wrote:
> > On 2016/04/13 05:57:06, perkj_webrtc wrote:
> > > why this change? 
> > 
> > Zero is a valid timestamp offset, we need a separate flag to tell if the
> offset
> > has been set yet or not.
> 
> Sounds like we should use rtc::Optional then, or agree to not use
rtc::Optional
> elsewhere, it feels weird to mix styles in this code.

Done. Is it correct to rely on the default constructor, to initialize it to
"unset"?

[perkj_webrtc, 2016-04-13 11:57:48]

On 2016/04/13 11:30:47, nisse-webrtc wrote:
>
https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
> File webrtc/media/engine/webrtcvideoengine2.cc (right):
> 
>
https://codereview.webrtc.org/1865283002/diff/120001/webrtc/media/engine/webr...
> webrtc/media/engine/webrtcvideoengine2.cc:1517:
> first_frame_timestamp_valid_(false),
> On 2016/04/13 09:56:24, pbos-webrtc wrote:
> > On 2016/04/13 06:54:50, nisse-webrtc wrote:
> > > On 2016/04/13 05:57:06, perkj_webrtc wrote:
> > > > why this change? 
> > > 
> > > Zero is a valid timestamp offset, we need a separate flag to tell if the
> > offset
> > > has been set yet or not.
> > 
> > Sounds like we should use rtc::Optional then, or agree to not use
> rtc::Optional
> > elsewhere, it feels weird to mix styles in this code.
> 
> Done. Is it correct to rely on the default constructor, to initialize it to
> "unset"?

lgtm

[pbos-webrtc, 2016-04-13 12:04:04]

I'd like the timestamps back into constructor/copy/etc arguments, can't we do
that?

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.cc:137: video_frame_buffer_, 0 /* Dummy
timestamp, overwridden below */,
That seems weird? rtc::kNumNanosecsPerMicrosec * timestamp_us_ ?

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.cc:206: int w, int h,
git cl format here

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:56: // timescale and epoch as rtc::Time.
Consider adding a warning
Put your ldap in a TODO here for the Consider

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:107: bool Reset(uint32_t format,
Shouldn't this take the timestamp as argument?

[nisse-webrtc, 2016-04-13 13:02:12]

BTW, one user of set_timestamp_us is VideoBroadcaster, when it sends reused
black frames.

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.cc:137: video_frame_buffer_, 0 /* Dummy
timestamp, overwridden below */,
On 2016/04/13 12:04:04, pbos-webrtc wrote:
> That seems weird? rtc::kNumNanosecsPerMicrosec * timestamp_us_ ?

I added a constructor with us timestamp (and arguments in different order, as
suggested by Per).

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.cc:206: int w, int h,
On 2016/04/13 12:04:04, pbos-webrtc wrote:
> git cl format here

Done.

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:56: // timescale and epoch as rtc::Time.
Consider adding a warning
On 2016/04/13 12:04:04, pbos-webrtc wrote:
> Put your ldap in a TODO here for the Consider

Done.

https://codereview.webrtc.org/1865283002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:107: bool Reset(uint32_t format,
On 2016/04/13 12:04:04, pbos-webrtc wrote:
> Shouldn't this take the timestamp as argument?

Done.

[pbos-webrtc, 2016-04-13 13:42:09]

lgtm, I hope this lands fine :)

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:60: // RTC_DCHECK, if the time is too far
off.
line break at 80

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe_unittest.cc (right):

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe_unittest.cc:23: using
cricket::WebRtcVideoFrame::set_rotation;
Is this using required?

[nisse-webrtc, 2016-04-13 14:37:33]

I'm a bit confused about the CreateEmptyFrame method in the tests. It overrides
a private method, it still looks strange to me, but I don't want to touch it in
this cl.

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:60: // RTC_DCHECK, if the time is too far
off.
On 2016/04/13 13:42:09, pbos-webrtc wrote:
> line break at 80

Done.

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe_unittest.cc (right):

https://codereview.webrtc.org/1865283002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe_unittest.cc:23: using
cricket::WebRtcVideoFrame::set_rotation;
On 2016/04/13 13:42:09, pbos-webrtc wrote:
> Is this using required?

Yes. It's a protected method in the base class, and the ApplyRotationToFrame
testcase needs it to be public.

On the other hand, the strange CreateEmptyFrame just below appear unused.

[nisse-webrtc, 2016-04-13 14:40:02]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-13 14:40:03]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1865283002/#ps240001 (title:
"Revert deletion of CreateEmptyFrame.")

[commit-bot: I haz the power, 2016-04-13 14:40:11]

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

[commit-bot: I haz the power, 2016-04-13 16:36:58]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-04-13 16:37:06]

Description was changed from

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740
==========

to

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740

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

[commit-bot: I haz the power, 2016-04-13 16:37:08]

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

[niklas.enbom, 2016-04-13 17:45:36]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.webrtc.org/1884863004/ by niklas.enbom@webrtc.org.

The reason for reverting is: This CL breaks Chrome FYI bots compile:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/bui...
.

[nisse-webrtc, 2016-04-14 06:49:57]

On 2016/04/13 17:45:36, niklas.enbom wrote:
> A revert of this CL (patchset #13 id:240001) has been created in
> https://codereview.webrtc.org/1884863004/ by mailto:niklas.enbom@webrtc.org.
> 
> The reason for reverting is: This CL breaks Chrome FYI bots compile:
>
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/bui...
> .

After a first look, it seems inconsistent use of override, which triggers the
stricter warning and -Werror flags used in chrome. So hopefully easy to fix.

[nisse-webrtc, 2016-04-14 06:58:51]

Description was changed from

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740

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

to

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740

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

[nisse-webrtc, 2016-04-14 07:21:55]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-14 07:21:56]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1865283002/#ps260001 (title:
"Add missing override, intending to reland.")

[commit-bot: I haz the power, 2016-04-14 07:22:04]

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

[commit-bot: I haz the power, 2016-04-14 09:29:33]

Description was changed from

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740

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

to

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740

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

[commit-bot: I haz the power, 2016-04-14 09:29:34]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-04-14 09:29:43]

Description was changed from

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740

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

to

==========
Use microsecond timestamp in cricket::VideoFrame.

BUG=webrtc:5740

Committed: https://crrev.com/f30ba114bb33dd1d8643bc640dda2e0c86dbbd32
Cr-Commit-Position: refs/heads/master@{#12348}

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

[commit-bot: I haz the power, 2016-04-14 09:29:44]

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