Change the TimeSource interface to return wallclock time for video.

media/renderers/video_renderer_impl.cc

Index: media/renderers/video_renderer_impl.cc
diff --git a/media/renderers/video_renderer_impl.cc b/media/renderers/video_renderer_impl.cc
index 0ad19e4348d702bade9856c1b37682469d307d1d..f2853a586728b038b5e442d5813df95524d2008e 100644
--- a/media/renderers/video_renderer_impl.cc
+++ b/media/renderers/video_renderer_impl.cc
@@ -10,6 +10,7 @@
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/platform_thread.h"
+#include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/buffers.h"
@@ -36,11 +37,10 @@ VideoRendererImpl::VideoRendererImpl(
       pending_read_(false),
       drop_frames_(drop_frames),
       buffering_state_(BUFFERING_HAVE_NOTHING),
-      last_timestamp_(kNoTimestamp()),
-      last_painted_timestamp_(kNoTimestamp()),
       frames_decoded_(0),
       frames_dropped_(0),
       is_shutting_down_(false),
+      tick_clock_(new base::DefaultTickClock()),
       weak_factory_(this) {
 }
 
@@ -109,7 +109,7 @@ void VideoRendererImpl::Initialize(
     const PaintCB& paint_cb,
     const base::Closure& ended_cb,
     const PipelineStatusCB& error_cb,
-    const TimeDeltaCB& get_time_cb,
+    const TimeConverterCB& time_converter_cb,
     const base::Closure& waiting_for_decryption_key_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
@@ -120,7 +120,7 @@ void VideoRendererImpl::Initialize(
   DCHECK(!buffering_state_cb.is_null());
   DCHECK(!paint_cb.is_null());
   DCHECK(!ended_cb.is_null());
-  DCHECK(!get_time_cb.is_null());
+  DCHECK(!time_converter_cb.is_null());
   DCHECK_EQ(kUninitialized, state_);
 
   low_delay_ = (stream->liveness() == DemuxerStream::LIVENESS_LIVE);
@@ -134,7 +134,7 @@ void VideoRendererImpl::Initialize(
   paint_cb_ = paint_cb,
   ended_cb_ = ended_cb;
   error_cb_ = error_cb;
-  get_time_cb_ = get_time_cb;
+  time_converter_cb_ = time_converter_cb;
   state_ = kInitializing;
 
   video_frame_stream_->Initialize(
@@ -190,11 +190,6 @@ void VideoRendererImpl::ThreadMain() {
   const base::TimeDelta kIdleTimeDelta =
       base::TimeDelta::FromMilliseconds(10);
 
-  // If we have no frames and haven't painted any frame for certain amount of
-  // time, declare BUFFERING_HAVE_NOTHING.
-  const base::TimeDelta kTimeToDeclareHaveNothing =
-      base::TimeDelta::FromSeconds(3);
-
   for (;;) {
     base::AutoLock auto_lock(lock_);
 
@@ -208,7 +203,7 @@ void VideoRendererImpl::ThreadMain() {
       continue;
     }
 
-    base::TimeDelta now = get_time_cb_.Run();
+    base::TimeTicks now = tick_clock_->NowTicks();
 
     // Remain idle until we have the next frame ready for rendering.
     if (ready_frames_.empty()) {
@@ -217,8 +212,7 @@ void VideoRendererImpl::ThreadMain() {
           rendered_end_of_stream_ = true;
           task_runner_->PostTask(FROM_HERE, ended_cb_);
         }
-      } else if (last_painted_timestamp_ != kNoTimestamp() &&
-                 now - last_painted_timestamp_ >= kTimeToDeclareHaveNothing) {
+      } else {
         buffering_state_ = BUFFERING_HAVE_NOTHING;
         task_runner_->PostTask(
             FROM_HERE, base::Bind(buffering_state_cb_, BUFFERING_HAVE_NOTHING));
@@ -228,8 +222,16 @@ void VideoRendererImpl::ThreadMain() {
       continue;
     }
 
-    base::TimeDelta target_paint_timestamp = ready_frames_.front()->timestamp();
-    base::TimeDelta latest_paint_timestamp;
+    base::TimeTicks target_paint_timestamp =
+        time_converter_cb_.Run(ready_frames_.front()->timestamp());
+
+    // If media time has stopped, don't attempt to paint any more frames.
+    if (target_paint_timestamp.is_null()) {
+      UpdateStatsAndWait_Locked(kIdleTimeDelta);
+      continue;
+    }
+
+    base::TimeTicks latest_paint_timestamp;

[xhwang, 2015/03/23 22:20:14]

bikesheddings not related to your change:

|latest| can also mean "most recent", which is a bit confusing given we also
have "last_painted_timestamp_"....

We seem to use timestamp for all time variables. I feel it's a bit weird to use
"timestamp" for a future wall clock time (TimeTicks)... Also, by looking at the
implementation, it's hard to tell which is media time (TimeDelta) and which is
wall time (TimeTicks). How about we use "timestamp" for media time, and just
"time" for wall clock time?

[DaleCurtis, 2015/03/25 00:31:45]

Done; great suggestion!

 
     // Deadline is defined as the duration between this frame and the next
     // frame, using the delta between this frame and the previous frame as the
@@ -237,8 +239,8 @@ void VideoRendererImpl::ThreadMain() {
     //
     // TODO(scherkus): This can be vastly improved. Use a histogram to measure
     // the accuracy of our frame timing code. http://crbug.com/149829
-    if (last_timestamp_ == kNoTimestamp()) {
-      latest_paint_timestamp = base::TimeDelta::Max();
+    if (last_timestamp_.is_null()) {
+      latest_paint_timestamp = now;
     } else {
       base::TimeDelta duration = target_paint_timestamp - last_timestamp_;
       latest_paint_timestamp = target_paint_timestamp + duration;
@@ -263,6 +265,11 @@ void VideoRendererImpl::ThreadMain() {
   }
 }
 
+void VideoRendererImpl::SetTickClockForTesting(
+    scoped_ptr<base::TickClock> tick_clock) {
+  tick_clock_.swap(tick_clock);
+}
+
 void VideoRendererImpl::PaintNextReadyFrame_Locked() {
   lock_.AssertAcquired();
 
@@ -270,8 +277,8 @@ void VideoRendererImpl::PaintNextReadyFrame_Locked() {
   ready_frames_.pop_front();
   frames_decoded_++;
 
-  last_timestamp_ = next_frame->timestamp();
-  last_painted_timestamp_ = next_frame->timestamp();
+  last_timestamp_ = last_painted_timestamp_ =
+      time_converter_cb_.Run(next_frame->timestamp());

[xhwang, 2015/03/23 22:20:14]

hmm, do we allow chained assignment? I don't see it banned anywhere, but I
rarely (if ever) see it in our code base.

[DaleCurtis, 2015/03/25 00:31:45]

Hmm, I tend to use it in the audio pathways, but I'm happy to change it if you
prefer otherwise.

 
   paint_cb_.Run(next_frame);
 
@@ -285,7 +292,8 @@ void VideoRendererImpl::DropNextReadyFrame_Locked() {
 
   lock_.AssertAcquired();
 
-  last_timestamp_ = ready_frames_.front()->timestamp();
+  last_timestamp_ = time_converter_cb_.Run(ready_frames_.front()->timestamp());
+
   ready_frames_.pop_front();
   frames_decoded_++;
   frames_dropped_++;
@@ -426,8 +434,7 @@ void VideoRendererImpl::OnVideoFrameStreamResetDone() {
   DCHECK_EQ(buffering_state_, BUFFERING_HAVE_NOTHING);
 
   state_ = kFlushed;
-  last_timestamp_ = kNoTimestamp();
-  last_painted_timestamp_ = kNoTimestamp();
+  last_timestamp_ = last_painted_timestamp_ = base::TimeTicks();

[xhwang, 2015/03/23 22:20:14]

ditto

[DaleCurtis, 2015/03/25 00:31:45]

Acknowledged.

   base::ResetAndReturn(&flush_cb_).Run();
 }