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

content/renderer/media/audio_renderer_impl.h

Index: content/renderer/media/audio_renderer_impl.h
===================================================================
--- content/renderer/media/audio_renderer_impl.h	(revision 121128)
+++ content/renderer/media/audio_renderer_impl.h	(working copy)
@@ -19,9 +19,12 @@
 
 #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"
@@ -77,18 +80,27 @@
   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;
   }
 
@@ -113,6 +125,11 @@
   // 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.
@@ -127,10 +144,26 @@
   // 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);
 };