Fix race condition when accessing time_progressing_.

media/renderers/video_renderer_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/renderers/video_renderer_impl.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
@@ skipping 195 matching lines @@
 
   // Due to how the |algorithm_| holds frames, this should never be null if
   // we've had a proper startup sequence.
   DCHECK(result);
 
   // Declare HAVE_NOTHING if we reach a state where we can't progress playback
   // any further.  We don't want to do this if we've already done so, reached
   // 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();
+  const size_t effective_frames =
+      MaybeFireEndedCallback(!render_first_frame_and_stop_);

[xhwang, 2015/07/21 21:01:38]

Can you explain a little bit more how "inverse of
|render_first_frame_and_stop_|" would suffice?

[DaleCurtis, 2015/07/22 00:44:43]

Done.

   if (buffering_state_ == BUFFERING_HAVE_ENOUGH && !received_end_of_stream_ &&
       !effective_frames && (!background_rendering ||
                             (!frames_decoded_ && was_background_rendering_))) {
     // Do not set |buffering_state_| here as the lock in FrameReady() may be
     // held already and it fire the state changes in the wrong order.
     task_runner_->PostTask(
         FROM_HERE, base::Bind(&VideoRendererImpl::TransitionToHaveNothing,
                               weak_factory_.GetWeakPtr()));
   }
 
@@ skipping 262 matching lines @@
       DCHECK_EQ(status, VideoFrameStream::DEMUXER_READ_ABORTED);
       return;
     }
 
     if (frame->metadata()->IsTrue(VideoFrameMetadata::END_OF_STREAM)) {
       DCHECK(!received_end_of_stream_);
       received_end_of_stream_ = true;
 
       // See if we can fire EOS immediately instead of waiting for Render().
       if (use_new_video_renderering_path_)
-        MaybeFireEndedCallback();
+        MaybeFireEndedCallback(time_progressing_);
     } else {
       // Maintain the latest frame decoded so the correct frame is displayed
       // after prerolling has completed.
       if (frame->timestamp() <= start_timestamp_) {
         if (use_new_video_renderering_path_)
           algorithm_->Reset();
         ready_frames_.clear();
       }
       AddReadyFrame_Locked(frame);
     }
@@ skipping 191 matching lines @@
   }
 
   if (wait_duration > base::TimeDelta())
     frame_available_.TimedWait(wait_duration);
 }
 
 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() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   const size_t kMaxVideoFrames = limits::kMaxVideoFrames;
 
   if (use_new_video_renderering_path_) {
     // When the display rate is less than the frame rate, the effective frames
     // queued may be much smaller than the actual number of frames queued.  Here
     // we ensure that frames_queued() doesn't get excessive.
@@ skipping 13 matching lines @@
 }
 
 void VideoRendererImpl::StopSink() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   sink_->Stop();
   algorithm_->set_time_stopped();
   sink_started_ = false;
   was_background_rendering_ = false;
 }
 
-size_t VideoRendererImpl::MaybeFireEndedCallback() {
+size_t VideoRendererImpl::MaybeFireEndedCallback(bool time_progressing) {

[xhwang, 2015/07/21 21:01:38]

This must be called under the lock, append _Locked to the file name, and check
lock_accquired in the function?

[DaleCurtis, 2015/07/22 00:44:43]

Done.

   // 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
   // to a single frame.
   const size_t effective_frames = algorithm_->EffectiveFramesQueued();
 
   // Don't fire ended if we haven't received EOS or have already done so.
   if (!received_end_of_stream_ || rendered_end_of_stream_)
     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.
   if (!effective_frames ||
       (algorithm_->frames_queued() == 1u &&
        algorithm_->average_frame_duration() == base::TimeDelta())) {
     rendered_end_of_stream_ = true;
     task_runner_->PostTask(FROM_HERE, ended_cb_);
   }
 
   return effective_frames;
 }
 
 base::TimeTicks VideoRendererImpl::ConvertMediaTimestamp(
     base::TimeDelta media_time) {
   std::vector<base::TimeDelta> media_times(1, media_time);
   std::vector<base::TimeTicks> wall_clock_times;
   if (!wall_clock_time_cb_.Run(media_times, &wall_clock_times))
     return base::TimeTicks();
   return wall_clock_times[0];
 }
 
 }  // namespace media