changes for proxy audio

chrome/renderer/pepper_plugin_delegate_impl.cc

Index: chrome/renderer/pepper_plugin_delegate_impl.cc
===================================================================
--- chrome/renderer/pepper_plugin_delegate_impl.cc	(revision 66651)
+++ chrome/renderer/pepper_plugin_delegate_impl.cc	(working copy)
@@ -123,10 +123,12 @@
 
 class PlatformAudioImpl
     : public pepper::PluginDelegate::PlatformAudio,
-      public AudioMessageFilter::Delegate {
+      public AudioMessageFilter::Delegate,
+      public base::RefCountedThreadSafe<PlatformAudioImpl> {
  public:
   explicit PlatformAudioImpl(scoped_refptr<AudioMessageFilter> filter)
-      : client_(NULL), filter_(filter), stream_id_(0) {
+      : client_(NULL), filter_(filter), stream_id_(0),
+        main_message_loop_(MessageLoop::current()) {
     DCHECK(filter_);
   }
 
@@ -177,6 +179,8 @@
   // Our ID on the MessageFilter.
   int32 stream_id_;
 
+  MessageLoop* main_message_loop_;

[neb, 2010/11/22 19:48:26]

I'm not convinced you need to store this since this should be the same as
filter_->message_loop(). You seem to be presuming that in the rest of the code.

[nfullagar, 2010/11/23 02:48:58]

Printing out filter_->message_loop() and main_message_loop_ from the constructor
show them to be different.

+
   DISALLOW_COPY_AND_ASSIGN(PlatformAudioImpl);
 };
 
@@ -281,6 +285,8 @@
   params.params.samples_per_packet = sample_count;
 
   stream_id_ = filter_->AddDelegate(this);
+  AddRef();  // Take a reference for the filter.
+
   return filter_->Send(new ViewHostMsg_CreateAudioStream(0, stream_id_, params,
                                                          true));
 }
@@ -297,7 +303,15 @@
 #endif
   DCHECK(length);
 
-  client_->StreamCreated(handle, length, socket_handle);
+  if (MessageLoop::current() == main_message_loop_) {
+    if (client_) {
+      client_->StreamCreated(handle, length, socket_handle);

[neb, 2010/11/22 19:48:26]

If the reference was taken to ensure that the object is alive while waiting for
CreateAudioStream() to return, then it should be released here after
StreamCreated() is called, and not in ShutDown().

The way I would go about writing this is to add another method called
DispatchStreamCreated() or something, that would do:

DispatchStreamCreated() {
  client->StreamCreated(handle, length, socket_handle);
  Release();
}

and then here I'd do:

filter_->message_loop()->PostTask(FROM_HERE, new RunnableMethod(this,
&PlatformAudioImpl::DispatchStreamCreated, handle, socket_handle, length));
    

[nfullagar, 2010/11/23 02:48:58]

What is here seems to be working, but I'd like to discuss more (maybe tomorrow?)

+    }
+  } else {
+    main_message_loop_->PostTask(FROM_HERE,
+        NewRunnableMethod(this, &PlatformAudioImpl::OnLowLatencyCreated,
+                          handle, socket_handle, length));
+  }
 }
 
 void PlatformAudioImpl::ShutDown() {
@@ -305,10 +319,18 @@
   if (!stream_id_) {
     return;
   }
+
   filter_->Send(new ViewHostMsg_CloseAudioStream(0, stream_id_));
   filter_->RemoveDelegate(stream_id_);

[neb, 2010/11/22 19:48:26]

I think a way to avoid the race would be to move RemoveDelegate() into the
destructor and make sure OnLowLatencyCreated() handles being called after
ShutDown().

+
+  // Release reference taken for the filter on the IO thread so that
+  // we avoid race problems with OnLowLatencyCreated.
+  filter_->message_loop()->ReleaseSoon(FROM_HERE, this);

[neb, 2010/11/22 19:48:26]

You are releasing twice, it seams?

[nfullagar, 2010/11/23 02:48:58]

There's an extra AddRef() above (line 288.)  If I understand it correctly, this
puts a release on the message queue.  If there is a pending StreamCreated also
in the queue, it will be invoked first, then this release will occur.

+
   stream_id_ = 0;
   client_ = NULL;
+
+  Release();  // May end up deleting ourselves.

[neb, 2010/11/22 19:48:26]

I think this Release() doesn't belong here.

 }
 
 // Implements the VideoDecoder.
@@ -560,7 +582,7 @@
 pepper::PluginDelegate::PlatformAudio* PepperPluginDelegateImpl::CreateAudio(
     uint32_t sample_rate, uint32_t sample_count,
     pepper::PluginDelegate::PlatformAudio::Client* client) {
-  scoped_ptr<PlatformAudioImpl> audio(
+  scoped_refptr<PlatformAudioImpl> audio(
       new PlatformAudioImpl(render_view_->audio_message_filter()));
   if (audio->Initialize(sample_rate, sample_count, client)) {
     return audio.release();