Mac Video Capture: Sending the timestamps provided by the driver to the capture pipeline

WebRTC render smoothness test shows on mac platform the timestamps on the video frames are quite irregular. The reason is that we read the system clock time, when the frame is available to chromium, but the system clock may not be high definition.

In this CL, we changed the behavior to reading timestamp from the capture driver.

BUG=549744
TEST=all unit test and browser test passing, apprtc working as before.

Committed: https://crrev.com/543582a37b414c18a7593560bbfe4eda978e41f0
Cr-Commit-Position: refs/heads/master@{#359620}

[qiangchen, 2015-10-30 17:49:01]

Patchset #1 (id:1) has been deleted

[qiangchen, 2015-10-30 18:23:14]

Description was changed from

==========
Revert Unnecessary Change


Merge branch 'Timestamp' into Timestamp2


Mac TS


Let reference time propagate

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

to

==========
Revert Unnecessary Change


Merge branch 'Timestamp' into Timestamp2


Mac TS


Let reference time propagate

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

[qiangchen, 2015-10-30 21:21:59]

Description was changed from

==========
Revert Unnecessary Change


Merge branch 'Timestamp' into Timestamp2


Mac TS


Let reference time propagate

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

to

==========
WebRTC render smoothness test shows on windows platform the timestamps on the
video frames are quite irregular. The reason is that we read the system clock
time, when the frame is available to chromium, but the system clock may not be
high definition.

In this CL, we changed the behavior to reading timestamp from the capture
driver.

BUG=549744
TEST=all unit test and browser test passing, apprtc working as before.
==========

[qiangchen, 2015-10-30 21:21:59]

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

[qiangchen, 2015-10-30 21:22:06]

Patchset #1 (id:20001) has been deleted

[qiangchen, 2015-10-30 21:22:14]

Patchset #1 (id:40001) has been deleted

[qiangchen, 2015-10-30 21:22:47]

Description was changed from

==========
WebRTC render smoothness test shows on windows platform the timestamps on the
video frames are quite irregular. The reason is that we read the system clock
time, when the frame is available to chromium, but the system clock may not be
high definition.

In this CL, we changed the behavior to reading timestamp from the capture
driver.

BUG=549744
TEST=all unit test and browser test passing, apprtc working as before.
==========

to

==========
WebRTC render smoothness test shows on mac platform the timestamps on the video
frames are quite irregular. The reason is that we read the system clock time,
when the frame is available to chromium, but the system clock may not be high
definition.

In this CL, we changed the behavior to reading timestamp from the capture
driver.

BUG=549744
TEST=all unit test and browser test passing, apprtc working as before.
==========

[qiangchen, 2015-10-30 22:40:44]

A similiar CL as we did not for Windows timestamp.

[mcasas, 2015-10-31 03:24:26]

Almost there.

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:331: const int
microseconds_per_second = 1000000;
Instead of |microseconds_per_second|, you should use
base::TimeBase::kMicrosecondsPerSecond| [1], here and
in the QTKit file.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s...

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:334: const
base::TimeTicks timestamp =
At least DCHECK_NE(0, cm_timestamp.timescale);

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
File media/capture/video/mac/video_capture_device_qtkit_mac.mm (right):

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
media/capture/video/mac/video_capture_device_qtkit_mac.mm:327: QTTime
qt_timestamp = [sampleBuffer presentationTime];
const

[qiangchen, 2015-11-02 18:16:25]

Patchset #2 (id:80001) has been deleted

[qiangchen, 2015-11-02 18:17:42]

Patchset #2 (id:100001) has been deleted

[qiangchen, 2015-11-02 18:21:21]

Did a fallback code path for invalid raw timestamps.

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:331: const int
microseconds_per_second = 1000000;
On 2015/10/31 03:24:25, mcasas wrote:
> Instead of |microseconds_per_second|, you should use
> base::TimeBase::kMicrosecondsPerSecond| [1], here and
> in the QTKit file.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s...

Done.

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:334: const
base::TimeTicks timestamp =
On 2015/10/31 03:24:25, mcasas wrote:
> At least DCHECK_NE(0, cm_timestamp.timescale);

Did a fallback code path in case the timestamp is invalid.

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
File media/capture/video/mac/video_capture_device_qtkit_mac.mm (right):

https://codereview.chromium.org/1421583007/diff/60001/media/capture/video/mac...
media/capture/video/mac/video_capture_device_qtkit_mac.mm:327: QTTime
qt_timestamp = [sampleBuffer presentationTime];
On 2015/10/31 03:24:25, mcasas wrote:
> const

Done.

[mcasas, 2015-11-02 18:59:18]

https://codereview.chromium.org/1421583007/diff/120001/media/capture/video/ma...
File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/1421583007/diff/120001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:8: #import
<CoreMedia/CoreMedia.h>
Imports, same as includes, should follow alphabetical
order. So, swap l.7 and 8.

https://codereview.chromium.org/1421583007/diff/120001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:335: if
((cm_timestamp.flags & kCMTimeFlags_Valid) && cm_timestamp.timescale) {
What about 

  const base::TimeTicks timestamp =
      CMTIME_IS_INVALID(cm_timestamp)
          ? base::TimeTicks::Now()
          : base::TimeTicks() + base::TimeDelta::FromMicroseconds(
                                    cm_timestamp.value *
                                    base::TimeTicks::kMicrosecondsPerSecond /
                                    cm_timestamp.timescale);

I would think that |cm_timestamp.timescale != 0| 
is included in the validity.

Or even better:

  const base::TimeTicks timestamp =
      CMTIME_IS_VALID(cm_timestamp)
          ? base::TimeTicks() +
                base::TimeDelta::FromSecondsD(CMTimeGetSeconds(cm_timestamp))
          : base::TimeTicks::Now();

Which is more readable IMHO, WDYT?

[qiangchen, 2015-11-02 21:35:47]

Done.

https://codereview.chromium.org/1421583007/diff/120001/media/capture/video/ma...
File media/capture/video/mac/video_capture_device_avfoundation_mac.mm (right):

https://codereview.chromium.org/1421583007/diff/120001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:8: #import
<CoreMedia/CoreMedia.h>
On 2015/11/02 18:59:18, mcasas wrote:
> Imports, same as includes, should follow alphabetical
> order. So, swap l.7 and 8.

Done.

https://codereview.chromium.org/1421583007/diff/120001/media/capture/video/ma...
media/capture/video/mac/video_capture_device_avfoundation_mac.mm:335: if
((cm_timestamp.flags & kCMTimeFlags_Valid) && cm_timestamp.timescale) {
On 2015/11/02 18:59:18, mcasas wrote:
> What about 
> 
>   const base::TimeTicks timestamp =
>       CMTIME_IS_INVALID(cm_timestamp)
>           ? base::TimeTicks::Now()
>           : base::TimeTicks() + base::TimeDelta::FromMicroseconds(
>                                     cm_timestamp.value *
>                                     base::TimeTicks::kMicrosecondsPerSecond /
>                                     cm_timestamp.timescale);
> 
> I would think that |cm_timestamp.timescale != 0| 
> is included in the validity.
> 
> Or even better:
> 
>   const base::TimeTicks timestamp =
>       CMTIME_IS_VALID(cm_timestamp)
>           ? base::TimeTicks() +
>                 base::TimeDelta::FromSecondsD(CMTimeGetSeconds(cm_timestamp))
>           : base::TimeTicks::Now();
> 
> Which is more readable IMHO, WDYT?

Done.

But not using CMTimeGetSeconds, because CoreMediaGlue::CMTime is not the apple
defined CMTime, but we redefined the same struct in coremedia_glue.h. Not sure
why, but in order to use a CM*** function, we have to expose it from the library
in coremedia_glue.h(and .cc) in a tedious way as we did for
CMGetPresentationTime, which I do not think necessary for just time format
conversion.

[mcasas, 2015-11-02 22:19:48]

lgtm

[qiangchen, 2015-11-02 22:44:09]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2015-11-02 22:46:04]

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

[commit-bot: I haz the power, 2015-11-02 22:57:28]

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

[commit-bot: I haz the power, 2015-11-02 22:57:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[qiangchen, 2015-11-02 23:25:00]

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

[qiangchen, 2015-11-02 23:26:57]

Hi, Dale

    Can you take a quick look at this CL. We are solving the issue of capture
timestamp irregularity on Mac.

[qiangchen, 2015-11-05 21:56:38]

Hi, Dale:

    Can you take a quick look at this CL, specially the two files under
media/base. Miguel has gone through the code review. 

    The clock timestamps at receiving the captured data contain noises that
would finally bring down the rendering smoothness. We instead try to extract
timestamp from the media sample, which is much better.

Qiang

[DaleCurtis, 2015-11-05 23:56:54]

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

[DaleCurtis, 2015-11-05 23:56:55]

Hmm, well by doing this TimeTicks is no longer meaningful if we end up comparing
it with other TimeTicks (say ::Now() which the VideoRendererAlgorithm will do
based on compositor clock).

What's the core problem with the tick clock in this case? Is it drifting
relative to the frame timestamp?

+miu who has experience in these matters.

[qiangchen, 2015-11-06 00:15:02]

On 2015/11/05 23:56:55, DaleCurtis wrote:
> Hmm, well by doing this TimeTicks is no longer meaningful if we end up
comparing
> it with other TimeTicks (say ::Now() which the VideoRendererAlgorithm will do
> based on compositor clock).
> 
> What's the core problem with the tick clock in this case? Is it drifting
> relative to the frame timestamp?
> 
> +miu who has experience in these matters.

Yep. The irregularity is not drift but the noise in frame length.
For example for 30FPS, we may observe frame length pattern like
{32,31,33,32....,47}. The timestamps taken from the sample buffer
are much better.

The defect of this simple solution is the alignment of the TimeTicks with Now(),
as mentioned by Dale. But functionally, it should be fine. As in 
VideoCaptureImpl::OnBufferReceived, when VideoFrame is generated, only 
elapsed timestamp (timestamp - first_timestamp) is used.

Or maybe, should we make another clean up CL to switch using TimeDelta
representing
timestamps all the way on the capture code path?

[qiangchen, 2015-11-09 18:02:34]

Ping miu@ for suggestions on how to convert the raw sample timestamps
(essentially from capture API) to TimeTicks.

[miu, 2015-11-10 21:25:33]

On 2015/11/09 18:02:34, qiangchenC wrote:
> Ping miu@ for suggestions on how to convert the raw sample timestamps
> (essentially from capture API) to TimeTicks.

TimeTicks *means* a snapshot of the common monotonic reference clock, which is a
different clock than what the capturing device is using.  Thus, these timestamp
values must always be generated relative to at least one call to
TimeTicks::Now().

I recommend you do the following in your code:

  /* ...in class declaration... */
  // These values are used to map timestamps from the capture device's clock to
the local reference clock (TimeTicks).
  base::TimeTicks first_frame_reference_time_;
  CoreMediaGlue::CMTime first_frame_pts_;

  /* ...in .cc code... */
  // The capture timestamp from the clock of the capture device.
  const CoreMediaGlue::CMTime pts =
CoreMediaGlue::CMSampleBufferGetPresentationTimeStamp(sampleBuffer);

  // Translate the capture timestamp to be in terms of the local reference
clock.
  base::TimeTicks reference_time;
  if (first_frame_reference_time_.is_null()) {
    first_frame_reference_time_ = reference_time = base::TimeTicks::Now();
    first_frame_pts_ = pts;
  } else {
    const base::TimeDelta elapsed =
        base::TimeDelta::FromSecondsD(CMTimeGetSeconds(CMTimeSubtract(pts,
first_frame_pts_)));
    reference_time = first_frame_reference_time_ + elapsed;
  }

The above will do what you want, but there is always the possibility the two
clocks will skew (i.e., run at a different rates) relative to each other.  The
solution to that would be something like ClockDriftSmoother
(https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/common/...).
 However, that may be overkill for this use case.  If there is no clock skew
adjustment in the video capture pipeline, it's likely this will occasionally
(once every several minutes/hours) cause either: 1) an excess frame to be
dropped; or 2) hold a frame for extra time.  You may want to confirm the rates
of excess/dropped frames is acceptable by testing on actual hardware.

BTW--Your CL description is incorrect: TimeTicks has <1 usec resolution on the
Mac/iOS platforms, so it *is* "high definition."  ;-)

[qiangchen, 2015-11-11 00:23:54]

Did a timestamp alignment with Now().

[qiangchen, 2015-11-11 00:23:55]

Did a timestamp alignment with Now().

[miu, 2015-11-12 03:10:14]

lgtm

[qiangchen, 2015-11-12 16:38:44]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2015-11-12 16:38:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1421583007/#ps160001
(title: "Alignment")

[commit-bot: I haz the power, 2015-11-12 16:38:59]

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

[commit-bot: I haz the power, 2015-11-12 16:48:45]

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

[commit-bot: I haz the power, 2015-11-12 16:48:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[qiangchen, 2015-11-12 16:52:12]

Ping dale@ to take a look at it.
Other reviewers are not owner of media/base files.

[DaleCurtis, 2015-11-12 19:34:48]

lgtm

[qiangchen, 2015-11-13 19:41:28]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2015-11-13 19:43:02]

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

[commit-bot: I haz the power, 2015-11-13 20:26:18]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2015-11-13 20:27:15]

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