Decouple 'give me more data' and 'rendered end of stream' audio callbacks.

content/renderer/media/audio_renderer_impl.h

 // Copyright (c) 2012 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.
 //
 // Audio rendering unit utilizing AudioDevice.
 //
 // This class lives inside three threads during it's lifetime, namely:
 // 1. Render thread.
 //    This object is created on the render thread.
 // 2. Pipeline thread
 //    OnInitialize() is called here with the audio format.
 //    Play/Pause/Seek also happens here.
 // 3. Audio thread created by the AudioDevice.
 //    Render() is called here where audio data is decoded into raw PCM data.
 
 #ifndef CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_IMPL_H_
 #define CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_IMPL_H_
 #pragma once
 
 #include <vector>
 
+#include "base/atomicops.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop_proxy.h"
 #include "base/synchronization/lock.h"
+#include "base/time.h"
 #include "content/renderer/media/audio_device.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_parameters.h"
 #include "media/base/audio_renderer_sink.h"
 #include "media/filters/audio_renderer_base.h"
 
 class AudioMessageFilter;
 
 class CONTENT_EXPORT AudioRendererImpl
     : public media::AudioRendererBase,
@@ skipping 35 matching lines @@
   // Helper methods.
   // Convert number of bytes to duration of time using information about the
   // number of channels, sample rate and sample bits.
   base::TimeDelta ConvertToDuration(int bytes);
 
   // Methods called on pipeline thread ----------------------------------------
   void DoPlay();
   void DoPause();
   void DoSeek();
 
+  // Methods called on IO thread ----------------------------------------------
+  void DoSignalEndOfStream(base::subtle::Atomic32 stream_id);
+
   // media::AudioRendererSink::RenderCallback implementation.
   virtual size_t Render(const std::vector<float*>& audio_data,
                         size_t number_of_frames,
                         size_t audio_delay_milliseconds) OVERRIDE;
   virtual void OnError() OVERRIDE;
 
+  // Returns delay in ms before call to OnRenderEndOfStream().
+  // Made virtual so test can override.
+  virtual int64 OnRenderEndOfStreamDelay();
+
   // Accessors used by tests.
-  base::Time earliest_end_time() const {
+  base::Time earliest_end_time() {
+    base::AutoLock auto_lock(earliest_end_time_lock_);
     return earliest_end_time_;
   }
 
   void set_earliest_end_time(const base::Time& earliest_end_time) {
+    base::AutoLock auto_lock(earliest_end_time_lock_);
     earliest_end_time_ = earliest_end_time;
   }
 
   uint32 bytes_per_second() const {
     return bytes_per_second_;
   }
 
   // Estimate earliest time when current buffer can stop playing.
   void UpdateEarliestEndTime(int bytes_filled,
                              base::TimeDelta request_delay,
                              base::Time time_now);
 
   // Used to calculate audio delay given bytes.
   uint32 bytes_per_second_;
 
   // A flag that indicates this filter is called to stop.
   bool stopped_;
 
   // The sink (destination) for rendered audio.
   scoped_refptr<media::AudioRendererSink> sink_;
 
   // Set to true when OnInitialize() is called.
   bool is_initialized_;
 
+  // Set to true when we post delayed task to signal 'ended' event.
+  // Made Atomic, not bool, because it can be accessed simultaneously
+  // by pipeline and audio threads.
+  base::subtle::Atomic32 ended_event_scheduled_;
+
   // We're supposed to know amount of audio data OS or hardware buffered, but
   // that is not always so -- on my Linux box
   // AudioBuffersState::hardware_delay_bytes never reaches 0.
   //
   // As a result we cannot use it to find when stream ends. If we just ignore
   // buffered data we will notify host that stream ended before it is actually
   // did so, I've seen it done ~140ms too early when playing ~150ms file.
   //
   // Instead of trying to invent OS-specific solution for each and every OS we
   // are supporting, use simple workaround: every time we fill the buffer we
   // remember when it should stop playing, and do not assume that buffer is
   // empty till that time. Workaround is not bulletproof, as we don't exactly
   // know when that particular data would start playing, but it is much better
   // than nothing.
+  // Access protected by lock as it can be accessed by pipeline and audio
+  // threads, and there is no Atomic64 on 32-bit systems.
   base::Time earliest_end_time_;
+  base::Lock earliest_end_time_lock_;
 
   AudioParameters audio_parameters_;
 
+  // Use message loop proxy, not message loop itself, to avoid crash
+  // because of message loop that ended while we are still playing.
+  // We don't need complex shutdown operations, just not posting tasks
+  // is enough, so we can use message loop proxy.
+  scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
+
+  // Stream id -- necessary because there is no way to cancel scheduled task.
+  // We want be sure we are still playing the same stream when delayed task is
+  // called. If seek or pause happened after scheduling but before task was
+  // called, there would be mismatch between expected and actual ids and delayed
+  // task would not do anything.
+  base::subtle::Atomic32 stream_id_;

[Ami GONE FROM CHROMIUM, 2012/02/23 20:18:25]

I might be missing something, but ISTM this member is terribly misused.  
Atomic32 is just an int32 under the covers.

++'ing an Atomic32 is not atomic (two concurrent ++'s of a 0-valued Atomic32 can
result in a final value of 1, for example).

The later test of this member:
if (stream_id == stream_id_)
looks like another race since:
a) it's just an int32 to the compiler, so there are no memory fences around that
read, nor volatile semantics, so the value read may not reflect updates in other
threads
b) nothing guarantees that stream_id_ won't change in another thread after
reading it and before acting on it.

Is there a reason a lock can't be used to guard a simple state-tracking int32?

(or, better yet, obviously, make the threading story explicit in this class
hierarchy and avoid touching state from multiple threads, but that's a much
bigger undertaking :))

[enal1, 2012/02/23 20:28:04]

++stream_id_ happens only on the pipeline thread, so there is no race condition
here.

There was race condition between seek/signal end of stream anyways, lack of
memory barrier/lock does not make it worse.

Anyways, I am rewriting the code to make Andrew happy and make code more robust
and simpler to understand...

+
   DISALLOW_COPY_AND_ASSIGN(AudioRendererImpl);
 };
 
 #endif  // CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_IMPL_H_