Fix race condition when accessing time_progressing_.

media/renderers/video_renderer_impl.h

 // 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.
 
 #ifndef MEDIA_RENDERERS_VIDEO_RENDERER_IMPL_H_
 #define MEDIA_RENDERERS_VIDEO_RENDERER_IMPL_H_
 
 #include <deque>
 
 #include "base/memory/ref_counted.h"
@@ skipping 17 matching lines @@
 #include "media/renderers/gpu_video_accelerator_factories.h"
 #include "media/video/gpu_memory_buffer_video_frame_pool.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 class TickClock;
 }
 
 namespace media {
 
 // VideoRendererImpl creates its own thread for the sole purpose of timing frame

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

This needs to be updated :)

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

I'll do so in the CL that removes the old pathway.

 // presentation.  It handles reading from the VideoFrameStream and stores the
 // results in a queue of decoded frames and executing a callback when a frame is
 // ready for rendering.
 class MEDIA_EXPORT VideoRendererImpl
     : public VideoRenderer,
       public NON_EXPORTED_BASE(VideoRendererSink::RenderCallback),
       public base::PlatformThread::Delegate {
  public:
   // |decoders| contains the VideoDecoders to use when initializing.
   //
@@ skipping 91 matching lines @@
 
   // Returns true if there is no more room for additional buffered frames.
   bool HaveReachedBufferingCap();
 
   // Starts or stops |sink_| respectively. Do not call while |lock_| is held.
   void StartSink();
   void StopSink();
 
   // Fires |ended_cb_| if there are no remaining usable frames and
   // |received_end_of_stream_| is true.  Sets |rendered_end_of_stream_| if it
-  // does so.  Returns algorithm_->EffectiveFramesQueued().
-  size_t MaybeFireEndedCallback();
+  // does so.
+  //
+  // When called from the media thread, |time_progressing| should reflect the
+  // value of |time_progressing_|.  When called from Render() on the sink
+  // callback thread, the inverse of |render_first_frame_and_stop_| should be
+  // used as a proxy for |time_progressing_|.
+  //
+  // Returns algorithm_->EffectiveFramesQueued().
+  size_t MaybeFireEndedCallback(bool time_progressing);
 
   // Helper method for converting a single media timestamp to wall clock time.
   base::TimeTicks ConvertMediaTimestamp(base::TimeDelta media_timestamp);
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   // Enables the use of VideoRendererAlgorithm and VideoRendererSink for frame
   // rendering instead of using a thread in a sleep-loop.  Set via the command
   // line flag kEnableNewVideoRenderer or via test methods.
   bool use_new_video_renderering_path_;
 
   // Sink which calls into VideoRendererImpl via Render() for video frames.  Do
   // not call any methods on the sink while |lock_| is held or the two threads
   // might deadlock. Do not call Start() or Stop() on the sink directly, use
   // StartSink() and StopSink() to ensure background rendering is started.
   VideoRendererSink* const sink_;
   bool sink_started_;

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

Add a comment that these two can only be accessed on the |tak_runner_|?

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

Done.

 
   // Used for accessing data members.
   base::Lock lock_;

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

Now it's unfortunate that it's pretty hard to know which member needs to be
accessed under the |lock_|.

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

Yeah, I'll reflow this in the deletion CL.

 
   // Provides video frames to VideoRendererImpl.
   scoped_ptr<VideoFrameStream> video_frame_stream_;
 
   // Pool of GpuMemoryBuffers and resources used to create hardware frames.
   scoped_ptr<GpuMemoryBufferVideoFramePool> gpu_memory_buffer_pool_;
 
   // Flag indicating low-delay mode.
   bool low_delay_;
 
@@ skipping 82 matching lines @@
 
   // Algorithm for selecting which frame to render; manages frames and all
   // timing related information.
   scoped_ptr<VideoRendererAlgorithm> algorithm_;
 
   // Indicates that Render() was called with |background_rendering| set to true,
   // so we've entered a background rendering mode where dropped frames are not
   // counted.  Must be accessed under |lock_| once |sink_| is started.
   bool was_background_rendering_;
 
-  // Indicates whether or not media time is currently progressing or not.
+  // Indicates whether or not media time is currently progressing or not.  Must
+  // only be accessed on the media thread.

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

This file doesn't know about the media thread. Replace it with something related
to the task runner?

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

Done.

   bool time_progressing_;
 
   // Indicates that Render() should only render the first frame and then request
   // that the sink be stopped.  |posted_maybe_stop_after_first_paint_| is used
   // to avoid repeated task posts.
   bool render_first_frame_and_stop_;
   bool posted_maybe_stop_after_first_paint_;
 
   // NOTE: Weak pointers must be invalidated before all other member variables.
   base::WeakPtrFactory<VideoRendererImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoRendererImpl);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_RENDERERS_VIDEO_RENDERER_IMPL_H_