Fix race condition when accessing time_progressing_.

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 1160b65dae5773ddcbd7bba5226c319f218543b2..1c2cd8cc26e416898aaa88a2253899d34a14b967 100644
--- a/media/renderers/video_renderer_impl.cc
+++ b/media/renderers/video_renderer_impl.cc
@@ -213,7 +213,18 @@ scoped_refptr<VideoFrame> VideoRendererImpl::Render(
   // end of stream, or have frames available.  We also don't want to do this in
   // background rendering mode unless this isn't the first background render
   // tick and we haven't seen any decoded frames since the last one.
-  const size_t effective_frames = MaybeFireEndedCallback();
+  //
+  // We use the inverse of |render_first_frame_and_stop_| as a proxy for the
+  // value of |time_progressing_| here since we can't access it from the
+  // compositor thread.  If we're here (in Render()) the sink must have been
+  // started -- but if it was started only to render the first frame and stop,
+  // then |time_progressing_| is likely false.  If we're still in Render() when
+  // |render_first_frame_and_stop_| is false, then |time_progressing_| is true.
+  // If |time_progressing_| is actually true when |render_first_frame_and_stop_|
+  // is also true, then the ended callback will be harmlessly delayed until
+  // MaybeStopSinkAfterFirstPaint() runs and the next Render() call comes in.

[xhwang, 2015/07/22 00:47:00]

I hope we can simplify this a bit one day :)

+  const size_t effective_frames =
+      MaybeFireEndedCallback_Locked(!render_first_frame_and_stop_);
   if (buffering_state_ == BUFFERING_HAVE_ENOUGH && !received_end_of_stream_ &&
       !effective_frames && (!background_rendering ||
                             (!frames_decoded_ && was_background_rendering_))) {
@@ -496,7 +507,7 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
 
       // See if we can fire EOS immediately instead of waiting for Render().
       if (use_new_video_renderering_path_)
-        MaybeFireEndedCallback();
+        MaybeFireEndedCallback_Locked(time_progressing_);
     } else {
       // Maintain the latest frame decoded so the correct frame is displayed
       // after prerolling has completed.
@@ -708,13 +719,11 @@ void VideoRendererImpl::MaybeStopSinkAfterFirstPaint() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(use_new_video_renderering_path_);
 
-  {
-    base::AutoLock auto_lock(lock_);
-    render_first_frame_and_stop_ = false;
-  }
-
   if (!time_progressing_ && sink_started_)
     StopSink();
+
+  base::AutoLock auto_lock(lock_);
+  render_first_frame_and_stop_ = false;
 }
 
 bool VideoRendererImpl::HaveReachedBufferingCap() {
@@ -748,7 +757,9 @@ void VideoRendererImpl::StopSink() {
   was_background_rendering_ = false;
 }
 
-size_t VideoRendererImpl::MaybeFireEndedCallback() {
+size_t VideoRendererImpl::MaybeFireEndedCallback_Locked(bool time_progressing) {
+  lock_.AssertAcquired();
+
   // If there's only one frame in the video or Render() was never called, the
   // algorithm will have one frame linger indefinitely.  So in cases where the
   // frame duration is unknown and we've received EOS, fire it once we get down
@@ -760,7 +771,7 @@ size_t VideoRendererImpl::MaybeFireEndedCallback() {
     return effective_frames;
 
   // Don't fire ended if time isn't moving and we have frames.
-  if (!time_progressing_ && algorithm_->frames_queued())
+  if (!time_progressing && algorithm_->frames_queued())
     return effective_frames;
 
   // Fire ended if we have no more effective frames or only ever had one frame.