Cast: Using a min filter to compute time offset

media/cast/video_receiver/video_receiver.cc

Index: media/cast/video_receiver/video_receiver.cc
diff --git a/media/cast/video_receiver/video_receiver.cc b/media/cast/video_receiver/video_receiver.cc
index 799da9a2d48b364b3bbf088c2a19d0035a299062..942bb01f81c52aaa65b0da3b9a79f55c701831f4 100644
--- a/media/cast/video_receiver/video_receiver.cc
+++ b/media/cast/video_receiver/video_receiver.cc
@@ -18,8 +18,8 @@
 namespace {
 
 static const int64 kMinSchedulingDelayMs = 1;
-static const int64 kMinTimeBetweenOffsetUpdatesMs = 2000;
-static const int kTimeOffsetFilter = 8;
+static const int64 kMinTimeBetweenOffsetUpdatesMs = 1000;
+static const int kTimeOffsetMaxCounter = 10;
 static const int64_t kMinProcessIntervalMs = 5;
 
 }  // namespace
@@ -107,6 +107,7 @@ VideoReceiver::VideoReceiver(scoped_refptr<CastEnvironment> cast_environment,
                     incoming_payload_callback_.get()),
       rtp_video_receiver_statistics_(
           new LocalRtpReceiverStatistics(&rtp_receiver_)),
+      time_offset_counter_(0),

[pwestin, 2014/03/04 00:10:54]

we should initialize time_offset_

[mikhal1, 2014/03/04 00:28:45]

will be initialized by the default const'
On 2014/03/04 00:10:54, pwestin wrote:

       decryptor_(),
       time_incoming_packet_updated_(false),
       incoming_rtp_timestamp_(0),
@@ -356,7 +357,7 @@ base::TimeTicks VideoReceiver::GetRenderTime(base::TimeTicks now,
   base::TimeTicks rtp_timestamp_in_ticks;
 
   // Compute the time offset_in_ticks based on the incoming_rtp_timestamp_.
-  if (time_offset_.InMilliseconds() == 0) {
+  if (time_offset_counter_ == 0) {
     if (!rtcp_->RtpTimestampInSenderTime(kVideoFrequency,
                                          incoming_rtp_timestamp_,
                                          &rtp_timestamp_in_ticks)) {
@@ -364,7 +365,7 @@ base::TimeTicks VideoReceiver::GetRenderTime(base::TimeTicks now,
       // possible.
       return now;
     }
-    time_offset_ = time_incoming_packet_ - rtp_timestamp_in_ticks;
+    ++time_offset_counter_;

[pwestin, 2014/03/04 00:10:54]

This will result in a bad first value; since we have no valid  time_offset_; I
think we should return now

[mikhal1, 2014/03/04 00:28:45]

true. fixed.
On 2014/03/04 00:10:54, pwestin wrote:

   } else if (time_incoming_packet_updated_) {
     if (rtcp_->RtpTimestampInSenderTime(kVideoFrequency,
                                         incoming_rtp_timestamp_,
@@ -372,8 +373,10 @@ base::TimeTicks VideoReceiver::GetRenderTime(base::TimeTicks now,
       // Time to update the time_offset.
       base::TimeDelta time_offset =
           time_incoming_packet_ - rtp_timestamp_in_ticks;
-      time_offset_ = ((kTimeOffsetFilter - 1) * time_offset_ + time_offset) /
-                     kTimeOffsetFilter;
+      if (time_offset_counter_  < kTimeOffsetMaxCounter &&
+          time_offset < time_offset_)

[pwestin, 2014/03/04 00:10:54]

we should do 
if (time_offset_counter_ == 1)
  time_offset_ = time_offset; 
else if (time_offset_counter_  < kTimeOffsetMaxCounter)
  time_offset_ = std::min(time_offset_, time_offset);

[mikhal1, 2014/03/04 00:28:45]

updated.
On 2014/03/04 00:10:54, pwestin wrote:

+        time_offset_ = time_offset;

[hguihot1, 2014/03/04 00:06:40]

This will select the minimum value within the first 10 samples (9 actually since
the first one is ignored). Is that the intent? There is still no guarantee we'll
get a 'good' time_offset_ during that time, so wouldn't it make more sense the
select the minimum within a moving window?

[mikhal1, 2014/03/04 00:28:45]

That indeed is the intention, the underlying assumption is that the drift is
insignificant (for the duration of a reasonable session), and therefore can be
ignored. Added a comment.
On 2014/03/04 00:06:40, hguihot1 wrote:

+      ++time_offset_counter_;
     }
   }
   // Reset |time_incoming_packet_updated_| to enable a future measurement.