Associate audio streams with their source/destination RenderView.

content/renderer/media/audio_message_filter.cc

Index: content/renderer/media/audio_message_filter.cc
diff --git a/content/renderer/media/audio_message_filter.cc b/content/renderer/media/audio_message_filter.cc
index d8a6e70c77eac5fa03ad3251e617b6f7b670e02c..62c984f5748c95b8cfd14b055c8cdb818934f621 100644
--- a/content/renderer/media/audio_message_filter.cc
+++ b/content/renderer/media/audio_message_filter.cc
@@ -22,18 +22,24 @@ AudioMessageFilter* AudioMessageFilter::Get() {
 }
 
 AudioMessageFilter::AudioMessageFilter()
-    : channel_(NULL) {
+    : next_stream_id_(1),
+      channel_(NULL) {
   DVLOG(1) << "AudioMessageFilter::AudioMessageFilter()";
   DCHECK(!filter_);
   filter_ = this;
 }
 
 int AudioMessageFilter::AddDelegate(media::AudioOutputIPCDelegate* delegate) {
-  return delegates_.Add(delegate);
+  base::AutoLock guard(delegates_lock_);
+  const int id = next_stream_id_;
+  delegates_.insert(std::make_pair(id, delegate));
+  ++next_stream_id_;

[tommi (sloooow) - chröme, 2012/11/29 13:08:38]

nit: might as well do:
const int id = next_stream_id_++;

to make the increment and new-id-handout clear on the same line.

[miu, 2012/11/30 02:40:35]

Done.

+  return id;
 }
 
 void AudioMessageFilter::RemoveDelegate(int id) {
-  delegates_.Remove(id);
+  base::AutoLock guard(delegates_lock_);
+  delegates_.erase(id);
 }
 
 void AudioMessageFilter::CreateStream(int stream_id,
@@ -42,6 +48,11 @@ void AudioMessageFilter::CreateStream(int stream_id,
   Send(new AudioHostMsg_CreateStream(stream_id, params, input_channels));
 }
 
+void AudioMessageFilter::AssociateStreamWithProducer(int stream_id,
+                                                     int render_view_id) {
+  Send(new AudioHostMsg_AssociateStreamWithProducer(stream_id, render_view_id));
+}
+
 void AudioMessageFilter::PlayStream(int stream_id) {
   Send(new AudioHostMsg_PlayStream(stream_id));
 }
@@ -102,15 +113,17 @@ void AudioMessageFilter::OnFilterRemoved() {
 
 void AudioMessageFilter::OnChannelClosing() {
   channel_ = NULL;
-  LOG_IF(WARNING, !delegates_.IsEmpty())
+
+  base::AutoLock guard(delegates_lock_);
+
+  LOG_IF(WARNING, !delegates_.empty())
       << "Not all audio devices have been closed.";
 
-  IDMap<media::AudioOutputIPCDelegate>::iterator it(&delegates_);
-  while (!it.IsAtEnd()) {
-    it.GetCurrentValue()->OnIPCClosed();
-    delegates_.Remove(it.GetCurrentKey());
-    it.Advance();
+  for (DelegateMap::const_iterator it = delegates_.begin();
+       it != delegates_.end(); ++it) {
+    it->second->OnIPCClosed();

[tommi (sloooow) - chröme, 2012/11/29 13:08:38]

We're calling out to OnIPCClosed() while under the delegates_lock_.  Is there a
chance that OnIPCClosed() implementations do some synchronous processing that
might depend on other threads?
I see we call out to other methods while holding the lock so perhaps this is
just a matter of making the threading contract clear (as in documented).

[miu, 2012/11/30 02:40:35]

Rather than having to document more-complex behavior, there was a simple way to
just avoid holding the lock while each remaining delegate's OnIPCClosed() method
is called.  Does my solution to this LGTY?

[tommi (sloooow) - chröme, 2012/11/30 06:51:31]

yup, lg.

   }
+  delegates_.clear();
 }
 
 AudioMessageFilter::~AudioMessageFilter() {
@@ -136,25 +149,31 @@ void AudioMessageFilter::OnStreamCreated(
 #if !defined(OS_WIN)
   base::SyncSocket::Handle socket_handle = socket_descriptor.fd;
 #endif
-  media::AudioOutputIPCDelegate* delegate = delegates_.Lookup(stream_id);
-  if (!delegate) {
-    DLOG(WARNING) << "Got audio stream event for a non-existent or removed"
-                    " audio renderer. (stream_id=" << stream_id << ").";
-    base::SharedMemory::CloseHandle(handle);
-    base::SyncSocket socket(socket_handle);
-    return;
+
+  {
+    base::AutoLock guard(delegates_lock_);
+    DelegateMap::const_iterator it = delegates_.find(stream_id);
+    if (it != delegates_.end()) {
+      it->second->OnStreamCreated(handle, socket_handle, length);
+      return;
+    }
   }
-  delegate->OnStreamCreated(handle, socket_handle, length);
+
+  DLOG(WARNING) << "Got audio stream event for a non-existent or removed"
+                   " audio renderer. (stream_id=" << stream_id << ").";
+  base::SharedMemory::CloseHandle(handle);
+  base::SyncSocket socket(socket_handle);
 }
 
 void AudioMessageFilter::OnStreamStateChanged(
     int stream_id, media::AudioOutputIPCDelegate::State state) {
-  media::AudioOutputIPCDelegate* delegate = delegates_.Lookup(stream_id);
-  if (!delegate) {
+  base::AutoLock guard(delegates_lock_);
+  DelegateMap::const_iterator it = delegates_.find(stream_id);
+  if (it == delegates_.end()) {

[tommi (sloooow) - chröme, 2012/11/29 13:08:38]

nit: you can avoid the early return here for less code (1 less call sites for
AutoLock dtor):

base::AutoLock guard(delegates_lock_);
DelegateMap::const_iterator it = delegates_.find(stream_id);
DLOG_IF(WARNING, it == delegates_.end()) << "No delegate! state=" << state;
if (it != delegates_.end())
  it->second->OnStateChanged(state);

[miu, 2012/11/30 02:40:35]

Done.

     DLOG(WARNING) << "No delegate found for state change. " << state;
     return;
   }
-  delegate->OnStateChanged(state);
+  it->second->OnStateChanged(state);
 }
 
 }  // namespace content