Decouple capture timestamp and reference time

Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
TBR=palmer@chromium.org

Committed: https://crrev.com/4387613998060e4d1691434d0be687bf012596ce
Cr-Commit-Position: refs/heads/master@{#399304}

[qiangchen, 2016-06-07 17:54:23]

Description was changed from

==========
Decouple capture timestamp and reference time

BUG=601657
==========

to

==========
Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
==========

[qiangchen, 2016-06-07 17:54:23]

qiangchen@chromium.org changed reviewers:
+ miu@chromium.org

[qiangchen, 2016-06-07 17:55:15]

qiangchen@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[qiangchen, 2016-06-07 17:56:08]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-06-07 17:56:29]

qiangchen@chromium.org changed reviewers:
+ palmer@chromium.org

[qiangchen, 2016-06-07 17:59:24]

The fix CL for capture timestamp vs reference_time.

palmer@: can you take a look at "content/common/media/video_capture_messages.h"
We add a parameter, namely "timestamp" for the IPC.

[miu, 2016-06-07 20:03:46]

miu@chromium.org changed reviewers:
+ mcasas@chromium.org
- palmer@chromium.org

[miu, 2016-06-07 20:03:47]

Good news! You won't need to make any IPC message change to pass the reference
time across processes. So, removing palmer@ as a reviewer.

Explained in comments below:

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:360:
base::TimeTicks reference_time) {
(See comment for video_capture_host.cc.) You could have the caller of this
method (or some caller further upstream) set the REFERENCE_TIME field in
|frame|'s metadata. Then, you don't need the 3rd argument to this method.

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:52:
base::TimeTicks reference_time) = 0;
ditto: You can let frame->metadata() carry the REFERENCE_TIME value instead of
having a 3rd argument here. (Explained in comment below for
video_capture_host.cc.)

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:83:
EXPECT_EQ(frame->format(), media::PIXEL_FORMAT_I420);
Up to you, but maybe there should also be:

  EXPECT_FALSE(reference_time.is_null());

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc
(right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc:150:
decode_done_cb_, base::Passed(&out_buffer), out_frame, reference_time);
ditto: Just set out_frame->metadata()->SetTimeTicks(...) instead of needing an
extra argument here.

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:104:
params.reference_time = reference_time;
There's no need to add a new |reference_time| to the IPC message.
VideoFrameMetadata already has what you need:

  video_frame->metadata()->SetTimeTicks(VideoFrameMetadata::REFERENCE_TIME,
reference_time);

Also, this will allow you to eliminate the extra |reference_time| argument to
this method (and in several other places). Just set the metadata property
upstream of here, and let the VideoFrame carry the REFERENCE_TIME in its
metadata as it is passed throughout the stack to the render process.

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:331:
reference_time.ToInternalValue(), "timestamp",
Hmm...These uses of ToInternalValue() are questionable. It would be more
readable to write these as:

  TRACE_EVENT_INSTANT2(
      "cast_perf_test", "OnBufferReceived",
      TRACE_EVENT_SCOPE_THREAD, "reference_time",
      (reference_time - base::TimeTicks()).InMicroseconds(),
      "timestamp", timestamp.InMicroseconds());

Also, this trace event is read by a performance test (see comment on line 328).
If you change the names here, you'll need to update that test code as well.

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:384:
frame->metadata()->SetTimeTicks(media::VideoFrameMetadata::REFERENCE_TIME,
This can become:

  DCHECK(frame->metadata()->HasKey(media::VideoFrameMetadata::REFERENCE_TIME));

(Since this field should have been set in the browser process.)

Oh, and it'll need to be moved down, after line 390 below, where the metadata is
copied from the IPC message.

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
File content/renderer/media/video_capture_impl_unittest.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_unittest.cc:173:
base::TimeDelta::FromInternalValue(now.ToInternalValue());
It'd be a bit cleaner to do this instead:

  base::TimeDelta timestamp = now - base::TimeTicks();

[qiangchen, 2016-06-07 20:20:21]

On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> Good news! You won't need to make any IPC message change to pass the reference
> time across processes. So, removing palmer@ as a reviewer.
> 
> Explained in comments below:
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> content/browser/renderer_host/media/video_capture_controller.cc:360:
> base::TimeTicks reference_time) {
> (See comment for video_capture_host.cc.) You could have the caller of this
> method (or some caller further upstream) set the REFERENCE_TIME field in
> |frame|'s metadata. Then, you don't need the 3rd argument to this method.
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> File
> content/browser/renderer_host/media/video_capture_controller_event_handler.h
> (right):
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/video_capture_controller_event_handler.h:52:
> base::TimeTicks reference_time) = 0;
> ditto: You can let frame->metadata() carry the REFERENCE_TIME value instead of
> having a 3rd argument here. (Explained in comment below for
> video_capture_host.cc.)
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/media/video_capture_controller_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> content/browser/renderer_host/media/video_capture_controller_unittest.cc:83:
> EXPECT_EQ(frame->format(), media::PIXEL_FORMAT_I420);
> Up to you, but maybe there should also be:
> 
>   EXPECT_FALSE(reference_time.is_null());
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc
> (right):
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc:150:
> decode_done_cb_, base::Passed(&out_buffer), out_frame, reference_time);
> ditto: Just set out_frame->metadata()->SetTimeTicks(...) instead of needing an
> extra argument here.
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/media/video_capture_host.cc (right):
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
> content/browser/renderer_host/media/video_capture_host.cc:104:
> params.reference_time = reference_time;
> There's no need to add a new |reference_time| to the IPC message.
> VideoFrameMetadata already has what you need:
> 
>   video_frame->metadata()->SetTimeTicks(VideoFrameMetadata::REFERENCE_TIME,
> reference_time);
> 
> Also, this will allow you to eliminate the extra |reference_time| argument to
> this method (and in several other places). Just set the metadata property
> upstream of here, and let the VideoFrame carry the REFERENCE_TIME in its
> metadata as it is passed throughout the stack to the render process.
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
> File content/renderer/media/video_capture_impl.cc (right):
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
> content/renderer/media/video_capture_impl.cc:331:
> reference_time.ToInternalValue(), "timestamp",
> Hmm...These uses of ToInternalValue() are questionable. It would be more
> readable to write these as:
> 
>   TRACE_EVENT_INSTANT2(
>       "cast_perf_test", "OnBufferReceived",
>       TRACE_EVENT_SCOPE_THREAD, "reference_time",
>       (reference_time - base::TimeTicks()).InMicroseconds(),
>       "timestamp", timestamp.InMicroseconds());
> 
> Also, this trace event is read by a performance test (see comment on line
328).
> If you change the names here, you'll need to update that test code as well.
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
> content/renderer/media/video_capture_impl.cc:384:
> frame->metadata()->SetTimeTicks(media::VideoFrameMetadata::REFERENCE_TIME,
> This can become:
> 
>  
DCHECK(frame->metadata()->HasKey(media::VideoFrameMetadata::REFERENCE_TIME));
> 
> (Since this field should have been set in the browser process.)
> 
> Oh, and it'll need to be moved down, after line 390 below, where the metadata
is
> copied from the IPC message.
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
> File content/renderer/media/video_capture_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
> content/renderer/media/video_capture_impl_unittest.cc:173:
> base::TimeDelta::FromInternalValue(now.ToInternalValue());
> It'd be a bit cleaner to do this instead:
> 
>   base::TimeDelta timestamp = now - base::TimeTicks();

Can you double check how we can avoid a parameter in IPC? I do not think it is
possible, because VideoFrame cannot be carried over IPC.

[miu, 2016-06-07 20:27:11]

On 2016/06/07 20:20:21, qiangchenC wrote:
> Can you double check how we can avoid a parameter in IPC? I do not think it is
> possible, because VideoFrame cannot be carried over IPC.

The VideoFrame's metadata dictionary is sent over the IPC:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

...and re-constituted in the render process here:
https://cs.chromium.org/chromium/src/content/renderer/media/video_capture_imp...

[qiangchen, 2016-06-07 23:51:07]

Patchset #3 (id:60001) has been deleted

[qiangchen, 2016-06-07 23:51:11]

Patchset #2 (id:40001) has been deleted

[qiangchen, 2016-06-07 23:51:16]

Patchset #2 (id:80001) has been deleted

[qiangchen, 2016-06-08 17:03:30]

Patchset #2 (id:90019) has been deleted

[qiangchen, 2016-06-08 18:01:31]

Patchset #2 (id:110001) has been deleted

[qiangchen, 2016-06-08 18:04:29]

qiangchen@chromium.org changed reviewers:
+ palmer@chromium.org

[qiangchen, 2016-06-08 18:04:31]

We still need palmer@ to do a security review.
As the type of the IPC parameter changes from TimeTicks to TimeDelta.

Fixed.

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:360:
base::TimeTicks reference_time) {
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> (See comment for video_capture_host.cc.) You could have the caller of this
> method (or some caller further upstream) set the REFERENCE_TIME field in
> |frame|'s metadata. Then, you don't need the 3rd argument to this method.

Done.

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:52:
base::TimeTicks reference_time) = 0;
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> ditto: You can let frame->metadata() carry the REFERENCE_TIME value instead of
> having a 3rd argument here. (Explained in comment below for
> video_capture_host.cc.)

Done.

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller_unittest.cc
(right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_unittest.cc:83:
EXPECT_EQ(frame->format(), media::PIXEL_FORMAT_I420);
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> Up to you, but maybe there should also be:
> 
>   EXPECT_FALSE(reference_time.is_null());

Done.

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc
(right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc:150:
decode_done_cb_, base::Passed(&out_buffer), out_frame, reference_time);
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> ditto: Just set out_frame->metadata()->SetTimeTicks(...) instead of needing an
> extra argument here.

Done.

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_host.cc:104:
params.reference_time = reference_time;
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> There's no need to add a new |reference_time| to the IPC message.
> VideoFrameMetadata already has what you need:
> 
>   video_frame->metadata()->SetTimeTicks(VideoFrameMetadata::REFERENCE_TIME,
> reference_time);
> 
> Also, this will allow you to eliminate the extra |reference_time| argument to
> this method (and in several other places). Just set the metadata property
> upstream of here, and let the VideoFrame carry the REFERENCE_TIME in its
> metadata as it is passed throughout the stack to the render process.

Done.

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:331:
reference_time.ToInternalValue(), "timestamp",
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> Hmm...These uses of ToInternalValue() are questionable. It would be more
> readable to write these as:
> 
>   TRACE_EVENT_INSTANT2(
>       "cast_perf_test", "OnBufferReceived",
>       TRACE_EVENT_SCOPE_THREAD, "reference_time",
>       (reference_time - base::TimeTicks()).InMicroseconds(),
>       "timestamp", timestamp.InMicroseconds());
> 
> Also, this trace event is read by a performance test (see comment on line
328).
> If you change the names here, you'll need to update that test code as well.

Change the names back to be consistent with other metrics. Changing the metric
name would be out of this CL's scope.

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:384:
frame->metadata()->SetTimeTicks(media::VideoFrameMetadata::REFERENCE_TIME,
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> This can become:
> 
>  
DCHECK(frame->metadata()->HasKey(media::VideoFrameMetadata::REFERENCE_TIME));
> 
> (Since this field should have been set in the browser process.)
> 
> Oh, and it'll need to be moved down, after line 390 below, where the metadata
is
> copied from the IPC message.

It is removed, as the metadata is already prepared with this field.

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
File content/renderer/media/video_capture_impl_unittest.cc (right):

https://codereview.chromium.org/2045813003/diff/20001/content/renderer/media/...
content/renderer/media/video_capture_impl_unittest.cc:173:
base::TimeDelta::FromInternalValue(now.ToInternalValue());
On 2016/06/07 20:03:47, miu (OOO June 11-19) wrote:
> It'd be a bit cleaner to do this instead:
> 
>   base::TimeDelta timestamp = now - base::TimeTicks();

Done.

But with reference time moved into metadata, the setting of reference time would
look complicated then.

[miu, 2016-06-08 19:24:44]

PS2 lgtm % a few minor things:

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:337: // If the timestamp is not
prepared, we use reference time to make a rough
This wasn't in PS1. Looks like there are some capturers that don't set the
VideoFrame::timestamp?

Actually, when I look up the code for tab capture, I see this is the case. Could
you add a "TODO(miu): Fix upstream capturers to always set timestamp and
reference time." here, with a link to this crbug: http://crbug.com/618407

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:407: // reference time rather than
using an extra parameter.
Yep, this "deliver callback" API was developed before VideoFrameMetadata even
existed.

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
File content/renderer/media/video_capture_impl_unittest.cc (right):

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_impl_unittest.cc:174: int64_t
internal_value = now.ToInternalValue();
This might break in the future if VideoFrameMetadata changes. Instead, I
suggest:

  media::VideoFrameMetadata md;
  md.SetTimeTicks(REFERENCE_TIME, ...);
  base::DictionaryValue dict;
  md.MergeInternalValuesInto(&dict);

...and then pass dict to OnBufferReceived().

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
File content/renderer/media/video_capture_message_filter_unittest.cc (right):

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_message_filter_unittest.cc:127: const
int64_t internal_value = 1;
ditto here: Use VideoFrameMetadata here, then
MergeInternalValuesInto(&params.metadata).

[qiangchen, 2016-06-08 20:08:55]

palmer@: Can you take a look at 	content/common/media/video_capture_messages.h

I just change an IPC parameter type from TimeTicks to TimeDelta.

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:337: // If the timestamp is not
prepared, we use reference time to make a rough
On 2016/06/08 19:24:44, miu (OOO June 11-19) wrote:
> This wasn't in PS1. Looks like there are some capturers that don't set the
> VideoFrame::timestamp?
> 
> Actually, when I look up the code for tab capture, I see this is the case.
Could
> you add a "TODO(miu): Fix upstream capturers to always set timestamp and
> reference time." here, with a link to this crbug: http://crbug.com/618407

Yeah, when I change the signature of OnIncomingCapturedVideoFrame,
I found ThreadSafeCaptureOracle::DidCaptureFrame does not prepare timestamp. I
do not know what is that, and it looks like that the code path is deep.

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:407: // reference time rather than
using an extra parameter.
On 2016/06/08 19:24:44, miu (OOO June 11-19) wrote:
> Yep, this "deliver callback" API was developed before VideoFrameMetadata even
> existed.

Acknowledged.

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
File content/renderer/media/video_capture_impl_unittest.cc (right):

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_impl_unittest.cc:174: int64_t
internal_value = now.ToInternalValue();
On 2016/06/08 19:24:44, miu (OOO June 11-19) wrote:
> This might break in the future if VideoFrameMetadata changes. Instead, I
> suggest:
> 
>   media::VideoFrameMetadata md;
>   md.SetTimeTicks(REFERENCE_TIME, ...);
>   base::DictionaryValue dict;
>   md.MergeInternalValuesInto(&dict);
> 
> ...and then pass dict to OnBufferReceived().

Done.

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
File content/renderer/media/video_capture_message_filter_unittest.cc (right):

https://codereview.chromium.org/2045813003/diff/130001/content/renderer/media...
content/renderer/media/video_capture_message_filter_unittest.cc:127: const
int64_t internal_value = 1;
On 2016/06/08 19:24:44, miu (OOO June 11-19) wrote:
> ditto here: Use VideoFrameMetadata here, then
> MergeInternalValuesInto(&params.metadata).

Done.

[qiangchen, 2016-06-08 20:10:38]

Dale:

    Can you take a look at this CL? miu@ does not own all the files.

    We actually passes both reference time and timestamp from capturer to chrome
code path. It changes some interface functions' signature, and thus touches so
many files.

Qiang

[DaleCurtis, 2016-06-08 20:44:19]

lgtm

[miu, 2016-06-08 21:14:08]

still lgtm. Thank you so much for plumbing this through! We are much much closer
now to TRUE A/V synchronization throughout the Chrome Media stack. :)

https://codereview.chromium.org/2045813003/diff/150001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/2045813003/diff/150001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:340: // time.
Can you add "http://crbug.com/618407" to the end of this comment, for tracking
purposes?  ;-)

[qiangchen, 2016-06-09 20:06:36]

Patchset #4 (id:170001) has been deleted

[qiangchen, 2016-06-10 16:00:02]

Description was changed from

==========
Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
==========

to

==========
Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
TBR=palmer@chromium.org
==========

[qiangchen, 2016-06-10 16:05:17]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-06-10 16:05:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2045813003/#ps190001
(title: "Nit")

[commit-bot: I haz the power, 2016-06-10 16:05:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2045813003/190001

[palmer, 2016-06-10 19:14:32]

lgtm

[commit-bot: I haz the power, 2016-06-10 22:46:08]

Description was changed from

==========
Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
TBR=palmer@chromium.org
==========

to

==========
Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
TBR=palmer@chromium.org
==========

[commit-bot: I haz the power, 2016-06-10 22:46:09]

Committed patchset #4 (id:190001)

[commit-bot: I haz the power, 2016-06-10 22:46:40]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 22:48:09]

Description was changed from

==========
Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
TBR=palmer@chromium.org
==========

to

==========
Decouple capture timestamp and reference time

In this CL, we make the captured VideoFrame to take capture device
timestamp; and take system clock reference time.

Background:
Previously, we have only timestamp parameter, and it actually
plays the role as timestamp and reference_time.
Here we define timestamp as the ideal time span between
the current frame and the first frame in the stream.
Define reference time as the system clock time when the frame
gets generated.

Ideally, timestamp can be derived from reference_time.
However, in reality, especially in a multi process application
like Chrome, system clock read has noise on the order of 5ms.
To overcome that, we found the original timestamp from camera
driver is much smoother. But the defect is that the camera
clock would have a drift with respect to the system clock.

Thus a perfect solution would be that we take system time for
reference time, which is used for AV sync; we stream timestamp
to the remote side, which will make the rendering smooth.

This CL is complete for the complete fix.
BUG=601657
TBR=palmer@chromium.org

Committed: https://crrev.com/4387613998060e4d1691434d0be687bf012596ce
Cr-Commit-Position: refs/heads/master@{#399304}
==========

[commit-bot: I haz the power, 2016-06-10 22:48:11]

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

[qiangchen, 2016-06-13 16:09:53]

https://codereview.chromium.org/2045813003/diff/150001/content/renderer/media...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/2045813003/diff/150001/content/renderer/media...
content/renderer/media/video_capture_impl.cc:340: // time.
On 2016/06/08 21:14:08, miu (OOO June 11-19) wrote:
> Can you add "http://crbug.com/618407" to the end of this comment, for tracking
> purposes?  ;-)

Done.