VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc clock.

VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc clock.

render_time time field (means capture time for sender side) is used by rtcp SenderReport to calculate offset since last frame and to estimate rtp timestamp for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp packets is unchanged.

BUG=webrtc:5433, webrtc:5504, webrtc:5505
Committed: https://crrev.com/ac287ee8b57a487cdea1a2a1f220949f073c5664
Cr-Commit-Position: refs/heads/master@{#11820}

[danilchap, 2016-02-11 14:43:55]

Description was changed from

==========
VideoCaptureInput enforce render_time to be generated by webrtc clock.

ntp_time and rtp_timestamp still based on external time that might have better
precision.

BUG=webrtc:5433
BUG=webrtc:5504
BUG=webrtc:5505
==========

to

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time on sender side) is used to sync with
audio.

ntp_time and rtp_timestamp still based on external time that might have better
precision.

BUG=webrtc:5433
BUG=webrtc:5504
BUG=webrtc:5505
==========

[danilchap, 2016-02-11 14:43:55]

danilchap@webrtc.org changed reviewers:
+ pbos@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2016-02-11 16:08:47]

Can you clarify the description a bit? What is render_time_ms used for exactly?
As far as I can tell the code uses ntp_time_ms and converts it to the rtp
timestamp.

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

The rtp timestamp together with the ntp timestamp will be used for
synchronization. How does render time relate to that?

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:39: // used for audio frame timestamping.
Audio frames aren't timestamped using the clock, but are instead counting
samples, so it's always based on the sound card rate.

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:66: // Local time in webrtc time base.
It's not clear to me if anything below actually has changed. Is this mostly
refactoring? Or maybe it's just too late for me to review this.

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:68:
incoming_frame.set_render_time_ms(current_time);
Have you figured out what the render time is used for in the sender? Is it only
used for rendering locally?

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:79:
incoming_frame.set_ntp_time_ms(capture_ntp_time_ms);
We should probably comment here that the ntp time is later converted to the rtp
timestamp.

[danilchap, 2016-02-11 17:18:14]

Description was changed from

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time on sender side) is used to sync with
audio.

ntp_time and rtp_timestamp still based on external time that might have better
precision.

BUG=webrtc:5433
BUG=webrtc:5504
BUG=webrtc:5505
==========

to

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time for sender side) is used by rtcp
SenderReport to calculate offset since last frame and to estimate rtp timestamp
for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream
synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp
packets is unchanged.

BUG=webrtc:5433
BUG=webrtc:5504
BUG=webrtc:5505
==========

[danilchap, 2016-02-11 17:18:54]

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:39: // used for audio frame timestamping.
On 2016/02/11 16:08:47, stefan-webrtc (holmer) wrote:
> Audio frames aren't timestamped using the clock, but are instead counting
> samples, so it's always based on the sound card rate.

I skipped several steps in reasoning why this clock should be
Clock::GetRealTimeClock(), comment corrected.

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:66: // Local time in webrtc time base.
On 2016/02/11 16:08:46, stefan-webrtc (holmer) wrote:
> It's not clear to me if anything below actually has changed. Is this mostly
> refactoring? Or maybe it's just too late for me to review this. 

render_time is always taken from clock_ instead of incoming video_frame.

rest is refactoring to clarify how ntp_time is calculated. calculation itself is
unchanged for ntp_time

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:68:
incoming_frame.set_render_time_ms(current_time);
On 2016/02/11 16:08:46, stefan-webrtc (holmer) wrote:
> Have you figured out what the render time is used for in the sender? Is it
only
> used for rendering locally?

render_time (later named capture_time) is used by rtcp SenderReport to calculate
how much time had pass since last frame was captured. Together with rtp
timestamp of last frame SenderReport estimate what rtp timestamp would be when
it is time to send SenderReport.
SenderReports are used for stream synchronization.

https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:79:
incoming_frame.set_ntp_time_ms(capture_ntp_time_ms);
On 2016/02/11 16:08:46, stefan-webrtc (holmer) wrote:
> We should probably comment here that the ntp time is later converted to the
rtp
> timestamp.

there is such comment 2 lines below.

[danilchap, 2016-02-15 17:27:33]

Description was changed from

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time for sender side) is used by rtcp
SenderReport to calculate offset since last frame and to estimate rtp timestamp
for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream
synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp
packets is unchanged.

BUG=webrtc:5433
BUG=webrtc:5504
BUG=webrtc:5505
==========

to

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time for sender side) is used by rtcp
SenderReport to calculate offset since last frame and to estimate rtp timestamp
for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream
synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp
packets is unchanged.

BUG=webrtc:5433, webrtc:5504, webrtc:5505
==========

[niklas.enbom, 2016-02-26 03:40:55]

Any chance we can get this one done and landed?

As a drive by review it seems a bit confusing the way that "render time" ends up
being used on the capture side (only to calculate RTCP NTP delta). We don't use
the timestamp for local rendering in Chrome as it is now, but if we did we would
end up with the +-16ms inaccuracy of the system clock on mac and windows because
of this change. 

On 2016/02/11 17:18:54, danilchap wrote:
>
https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
> File webrtc/video/video_capture_input.cc (right):
> 
>
https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
> webrtc/video/video_capture_input.cc:39: // used for audio frame timestamping.
> On 2016/02/11 16:08:47, stefan-webrtc (holmer) wrote:
> > Audio frames aren't timestamped using the clock, but are instead counting
> > samples, so it's always based on the sound card rate.
> 
> I skipped several steps in reasoning why this clock should be
> Clock::GetRealTimeClock(), comment corrected.
> 
>
https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
> webrtc/video/video_capture_input.cc:66: // Local time in webrtc time base.
> On 2016/02/11 16:08:46, stefan-webrtc (holmer) wrote:
> > It's not clear to me if anything below actually has changed. Is this mostly
> > refactoring? Or maybe it's just too late for me to review this. 
> 
> render_time is always taken from clock_ instead of incoming video_frame.
> 
> rest is refactoring to clarify how ntp_time is calculated. calculation itself
is
> unchanged for ntp_time
> 
>
https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
> webrtc/video/video_capture_input.cc:68:
> incoming_frame.set_render_time_ms(current_time);
> On 2016/02/11 16:08:46, stefan-webrtc (holmer) wrote:
> > Have you figured out what the render time is used for in the sender? Is it
> only
> > used for rendering locally?
> 
> render_time (later named capture_time) is used by rtcp SenderReport to
calculate
> how much time had pass since last frame was captured. Together with rtp
> timestamp of last frame SenderReport estimate what rtp timestamp would be when
> it is time to send SenderReport.
> SenderReports are used for stream synchronization.
> 
>
https://codereview.webrtc.org/1693443002/diff/20001/webrtc/video/video_captur...
> webrtc/video/video_capture_input.cc:79:
> incoming_frame.set_ntp_time_ms(capture_ntp_time_ms);
> On 2016/02/11 16:08:46, stefan-webrtc (holmer) wrote:
> > We should probably comment here that the ntp time is later converted to the
> rtp
> > timestamp.
> 
> there is such comment 2 lines below.

[danilchap, 2016-02-26 11:30:50]

Five clocks are used on Sender side to synchronize Video and Audio:
rtp video clock
delta clock for Video rtcp SenderReport
rtcp Ntp clock
delta clock for Audio rtcp SenderReport
rtp audio clock

Sender report has both rtp timestamp (from rtp audio/video clock) and ntp time
(from rtcp Ntp clock)
Receive side restores ntp time of an audio/video frame from rtp timestamp using
mapping from SenderReports.

When Sender report is created, it takes ntp time from rtcp Ntp clock and
estimate rtp timestamp for current point of time.
To do it, it need to know how much time passed since last frame was generated.
That where delta clock is used.
Delta clock captures two kind of time:
  one is as close as it can to frame capture.
  second one as close as it can to time from rtcp Ntp clock.

This cl address taking time for video delta clock near frame capture.

Before this cl it was assumed that rtp video clock and delta clock can be
perfectly synchronized (with an offset). Sometimes it's not true (I believe that
assumption is not true when time was taken from Logitech C930 camera).
This cl ensure capture time is taken from same delta clock rtcp module is using.
Downside: this introduce some delay between 'real' capture time and capture time
generated by delta clock.

Same downside already exists for audio side: capture time from delta clock is
taken when audio frame arrives in rtcp module.

render_time name is used as capture_time because same VideoFrame structure
reused on Receive and Send side.

[stefan-webrtc, 2016-02-29 08:59:52]

lgtm

[stefan-webrtc, 2016-02-29 09:00:04]

Thanks for improving the comments.

[pbos-webrtc, 2016-02-29 10:49:55]

lgtm

[danilchap, 2016-02-29 14:32:44]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-02-29 14:32:54]

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

[commit-bot: I haz the power, 2016-02-29 16:34:01]

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

[commit-bot: I haz the power, 2016-02-29 16:34:02]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[danilchap, 2016-02-29 19:51:18]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-02-29 19:51:21]

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

[commit-bot: I haz the power, 2016-02-29 20:17:07]

Description was changed from

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time for sender side) is used by rtcp
SenderReport to calculate offset since last frame and to estimate rtp timestamp
for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream
synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp
packets is unchanged.

BUG=webrtc:5433, webrtc:5504, webrtc:5505
==========

to

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time for sender side) is used by rtcp
SenderReport to calculate offset since last frame and to estimate rtp timestamp
for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream
synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp
packets is unchanged.

BUG=webrtc:5433, webrtc:5504, webrtc:5505
==========

[commit-bot: I haz the power, 2016-02-29 20:17:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-29 20:17:14]

Description was changed from

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time for sender side) is used by rtcp
SenderReport to calculate offset since last frame and to estimate rtp timestamp
for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream
synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp
packets is unchanged.

BUG=webrtc:5433, webrtc:5504, webrtc:5505
==========

to

==========
VideoCaptureInput enforce VideoFrame::render_time to be generated by webrtc
clock.

render_time time field (means capture time for sender side) is used by rtcp
SenderReport to calculate offset since last frame and to estimate rtp timestamp
for the time SenderReport should be send at.
mapping between rtp timestamp and ntp time in SenderReport is used for stream
synchronization.

calculation of rtp_timestamp (using ntp_time of incoming video frame) for rtp
packets is unchanged.

BUG=webrtc:5433, webrtc:5504, webrtc:5505

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

[commit-bot: I haz the power, 2016-02-29 20:17:15]

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