Stop using VoEVideoSync in Call/VideoReceiveStream.

Stop using VoEVideoSync in Call/VideoReceiveStream.

BUG=webrtc:4690
Review-Url: https://codereview.webrtc.org/2452163004
Cr-Commit-Position: refs/heads/master@{#16375}
Committed: https://chromium.googlesource.com/external/webrtc/+/3ebbcb528b7eb0d2de58b3e6c99055ca3cd3e7f6

[the sun, 2017-01-13 14:33:07]

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

[stefan-webrtc, 2017-01-13 16:21:05]

https://codereview.webrtc.org/2452163004/diff/80001/webrtc/video/rtp_streams_...
File webrtc/video/rtp_streams_synchronizer.cc (right):

https://codereview.webrtc.org/2452163004/diff/80001/webrtc/video/rtp_streams_...
webrtc/video/rtp_streams_synchronizer.cc:81:
syncable_->GetRtpRtcp(&audio_rtp_rtcp_, &audio_rtp_receiver_);
Maybe we take the opportunity to change the interface so that it doesn't have to
return a full RtpRtcp and an RtpReceiver. Instead we could return what is really
being used, which I think is the NTP/RTP timestamp pair from RTCP and the latest
played out RTP timestamp together with the last playout time. Would be cleaner,
IMO.

[the sun, 2017-01-19 11:45:29]

https://codereview.webrtc.org/2452163004/diff/80001/webrtc/video/rtp_streams_...
File webrtc/video/rtp_streams_synchronizer.cc (right):

https://codereview.webrtc.org/2452163004/diff/80001/webrtc/video/rtp_streams_...
webrtc/video/rtp_streams_synchronizer.cc:81:
syncable_->GetRtpRtcp(&audio_rtp_rtcp_, &audio_rtp_receiver_);
On 2017/01/13 16:21:04, stefan-webrtc wrote:
> Maybe we take the opportunity to change the interface so that it doesn't have
to
> return a full RtpRtcp and an RtpReceiver. Instead we could return what is
really
> being used, which I think is the NTP/RTP timestamp pair from RTCP and the
latest
> played out RTP timestamp together with the last playout time. Would be
cleaner,
> IMO.

Yes, I agree it makes sense to not expose the RtpRtcp module for audio. See
latest PS.

However, the RtpStreamsSynchronizer still knows about the RtpRtcp module for the
video case. Would it make sense to use Syncable for video as well? One obstacle
could be that the methods on Syncable are provided for video by both
vcm::VideoReceiver and RtpStreamReceiver.

[stefan-webrtc, 2017-01-19 11:59:59]

https://codereview.webrtc.org/2452163004/diff/80001/webrtc/video/rtp_streams_...
File webrtc/video/rtp_streams_synchronizer.cc (right):

https://codereview.webrtc.org/2452163004/diff/80001/webrtc/video/rtp_streams_...
webrtc/video/rtp_streams_synchronizer.cc:81:
syncable_->GetRtpRtcp(&audio_rtp_rtcp_, &audio_rtp_receiver_);
On 2017/01/19 11:45:29, the sun wrote:
> On 2017/01/13 16:21:04, stefan-webrtc wrote:
> > Maybe we take the opportunity to change the interface so that it doesn't
have
> to
> > return a full RtpRtcp and an RtpReceiver. Instead we could return what is
> really
> > being used, which I think is the NTP/RTP timestamp pair from RTCP and the
> latest
> > played out RTP timestamp together with the last playout time. Would be
> cleaner,
> > IMO.
> 
> Yes, I agree it makes sense to not expose the RtpRtcp module for audio. See
> latest PS.
> 
> However, the RtpStreamsSynchronizer still knows about the RtpRtcp module for
the
> video case. Would it make sense to use Syncable for video as well? One
obstacle
> could be that the methods on Syncable are provided for video by both
> vcm::VideoReceiver and RtpStreamReceiver.

Yes, I think it would make a lot of sense to use the same interface. All that
should be needed for something to be syncable is the ability to get the mapping
between rtp and ntp, the time and timestamp of the latest playout event and the
possibility to change the min buffer size to control the sync.

I think we should be able to make VideoReceiveStream syncable.

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:249: rtc::Optional<Syncable::Info>
AudioReceiveStream::GetInfo() const {
Should you make sure this runs on the right thread too? Seems like the same
requirement applies as for id()?

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h
File webrtc/call/syncable.h (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h#n...
webrtc/call/syncable.h:27: uint32_t latest_timestamp = 0;
latest_received_timestamp, I assume?

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h#n...
webrtc/call/syncable.h:30: uint32_t rtp_timestamp = 0;
Would be good to clarify that this is the ntp/rtp timestamp pair in RTCP to be
used to map between rtp and ntp time.

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/video/rtp_streams...
File webrtc/video/rtp_streams_synchronizer.cc (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/video/rtp_streams...
webrtc/video/rtp_streams_synchronizer.cc:28: RtpRtcp* rtp_rtcp,
git cl format

[the sun, 2017-01-19 13:15:36]

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:249: rtc::Optional<Syncable::Info>
AudioReceiveStream::GetInfo() const {
On 2017/01/19 11:59:59, stefan-webrtc wrote:
> Should you make sure this runs on the right thread too? Seems like the same
> requirement applies as for id()?

id() is called when sync is set up, part of stream create/destroy calls, on
worker thread.

GetInfo() is called on Call's processing thread. Nevertheless, I'll add a TC.

[stefan-webrtc, 2017-01-26 08:40:50]

lg! just a few more comments

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h
File webrtc/call/syncable.h (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h#n...
webrtc/call/syncable.h:27: uint32_t latest_timestamp = 0;
On 2017/01/19 11:59:59, stefan-webrtc wrote:
> latest_received_timestamp, I assume?

I think 'received' is better since this is the capture time (in rtp) of the
frame that we most recently received. It's not the receive timestamp of the
latest frame... Subtle difference, so maybe there's a better name...

latest_received_capture_timestamp?

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/rtp_streams...
File webrtc/video/rtp_streams_synchronizer.h (right):

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/rtp_streams...
webrtc/video/rtp_streams_synchronizer.h:22: #include
"webrtc/video/rtp_stream_receiver.h"
Remove this.

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/rtp_streams...
webrtc/video/rtp_streams_synchronizer.h:48: bool GetStreamSyncOffsetInMs(const
VideoFrame& frame,
If you feel like doing some more cleanup here, you could replace this with the
video (capture) timestamp instead.

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:483: // Called on Call's
module_process_thread_.
I wouldn't mind if thread checks were added here too, as for audio, please? :)

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/stream_sync...
File webrtc/video/stream_synchronization.cc (right):

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/stream_sync...
webrtc/video/stream_synchronization.cc:32: audio_stream_id_(audio_stream_id),
Rename these id or ssrc for both audio and video

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:490: // believe we should be using
remote_ssrc instead?
I think that sounds correct. Feel free to change, it's not affecting the sync
anyway, just logging.

[the sun, 2017-01-30 15:43:12]

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h
File webrtc/call/syncable.h (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h#n...
webrtc/call/syncable.h:27: uint32_t latest_timestamp = 0;
On 2017/01/26 08:40:50, stefan-webrtc wrote:
> On 2017/01/19 11:59:59, stefan-webrtc wrote:
> > latest_received_timestamp, I assume?
> 
> I think 'received' is better since this is the capture time (in rtp) of the
> frame that we most recently received. It's not the receive timestamp of the
> latest frame... Subtle difference, so maybe there's a better name...
> 
> latest_received_capture_timestamp?

Done.

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h#n...
webrtc/call/syncable.h:30: uint32_t rtp_timestamp = 0;
On 2017/01/19 11:59:59, stefan-webrtc wrote:
> Would be good to clarify that this is the ntp/rtp timestamp pair in RTCP to be
> used to map between rtp and ntp time.

I don't really like that we're mixing in RTCP. If we can make the names
transport-agnostic, that'd be preferable.

feedback_ntp_secs
feedback_ntp_frac
feedback_timestamp

?

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/rtp_streams...
File webrtc/video/rtp_streams_synchronizer.h (right):

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/rtp_streams...
webrtc/video/rtp_streams_synchronizer.h:48: bool GetStreamSyncOffsetInMs(const
VideoFrame& frame,
On 2017/01/26 08:40:50, stefan-webrtc wrote:
> If you feel like doing some more cleanup here, you could replace this with the
> video (capture) timestamp instead.

Done.

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2452163004/diff/140001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:483: // Called on Call's
module_process_thread_.
On 2017/01/26 08:40:50, stefan-webrtc wrote:
> I wouldn't mind if thread checks were added here too, as for audio, please? :)

Happy to oblige.

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/stream_sync...
File webrtc/video/stream_synchronization.cc (right):

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/stream_sync...
webrtc/video/stream_synchronization.cc:32: audio_stream_id_(audio_stream_id),
On 2017/01/26 08:40:50, stefan-webrtc wrote:
> Rename these id or ssrc for both audio and video

Going with id in both cases.

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2452163004/diff/160001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:490: // believe we should be using
remote_ssrc instead?
On 2017/01/26 08:40:50, stefan-webrtc wrote:
> I think that sounds correct. Feel free to change, it's not affecting the sync
> anyway, just logging.

Done.

[stefan-webrtc, 2017-01-30 16:11:11]

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h
File webrtc/call/syncable.h (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h#n...
webrtc/call/syncable.h:30: uint32_t rtp_timestamp = 0;
On 2017/01/30 15:43:12, the sun wrote:
> On 2017/01/19 11:59:59, stefan-webrtc wrote:
> > Would be good to clarify that this is the ntp/rtp timestamp pair in RTCP to
be
> > used to map between rtp and ntp time.
> 
> I don't really like that we're mixing in RTCP. If we can make the names
> transport-agnostic, that'd be preferable.
> 
> feedback_ntp_secs
> feedback_ntp_frac
> feedback_timestamp
> 
> ?

Me neither, bad suggestion from my side.

Maybe rtp_timestamp -> capture_time_source_clock and ntp_secs, ntp_frac ->
capture_time_ntp_frac, capture_time_ntp_secs?

Or if we use the NtpTime class, something like this?
std::pair<uint32_t, NtpTime> clock_sync_point_

[the sun, 2017-01-31 10:00:32]

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h
File webrtc/call/syncable.h (right):

https://codereview.webrtc.org/2452163004/diff/100001/webrtc/call/syncable.h#n...
webrtc/call/syncable.h:30: uint32_t rtp_timestamp = 0;
On 2017/01/30 16:11:11, stefan-webrtc wrote:
> On 2017/01/30 15:43:12, the sun wrote:
> > On 2017/01/19 11:59:59, stefan-webrtc wrote:
> > > Would be good to clarify that this is the ntp/rtp timestamp pair in RTCP
to
> be
> > > used to map between rtp and ntp time.
> > 
> > I don't really like that we're mixing in RTCP. If we can make the names
> > transport-agnostic, that'd be preferable.
> > 
> > feedback_ntp_secs
> > feedback_ntp_frac
> > feedback_timestamp
> > 
> > ?
> 
> Me neither, bad suggestion from my side.
> 
> Maybe rtp_timestamp -> capture_time_source_clock and ntp_secs, ntp_frac ->
> capture_time_ntp_frac, capture_time_ntp_secs?

Went with this - I don't like to use pair in this case.

> Or if we use the NtpTime class, something like this?
> std::pair<uint32_t, NtpTime> clock_sync_point_

[stefan-webrtc, 2017-01-31 10:44:31]

lgtm

[the sun, 2017-01-31 10:46:53]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2017-01-31 10:47:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-31 11:58:41]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485859613148240,
"parent_rev": "63b14b7d15d9481f81424fc61bd8e90fc9bafaee", "commit_rev":
"3ebbcb528b7eb0d2de58b3e6c99055ca3cd3e7f6"}

[commit-bot: I haz the power, 2017-01-31 11:58:43]

Description was changed from

==========
Stop using VoEVideoSync in Call/VideoReceiveStream.

BUG=webrtc:4690
==========

to

==========
Stop using VoEVideoSync in Call/VideoReceiveStream.

BUG=webrtc:4690

Review-Url: https://codereview.webrtc.org/2452163004
Cr-Commit-Position: refs/heads/master@{#16375}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3ebbcb528b7eb0d2de58b3e6c...
==========

[commit-bot: I haz the power, 2017-01-31 11:58:45]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/3ebbcb528b7eb0d2de58b3e6c...