Associate audio streams with their source/destination RenderView.

content/renderer/media/render_audiosourceprovider.cc

Index: content/renderer/media/render_audiosourceprovider.cc
diff --git a/content/renderer/media/render_audiosourceprovider.cc b/content/renderer/media/render_audiosourceprovider.cc
index 750db7fe0a47472c99bbdc50ff3ba7f612062da2..fb1295f9173d5393c69a5f2e74759ce3883ce6ad 100644
--- a/content/renderer/media/render_audiosourceprovider.cc
+++ b/content/renderer/media/render_audiosourceprovider.cc
@@ -9,9 +9,10 @@
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/logging.h"
-#include "content/renderer/media/audio_device_factory.h"
+#include "content/renderer/media/audio_message_filter.h"
 #include "content/renderer/media/audio_renderer_mixer_manager.h"
 #include "content/renderer/render_thread_impl.h"
+#include "media/audio/audio_output_device.h"
 #include "media/base/audio_renderer_mixer_input.h"
 #include "media/base/media_switches.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebAudioSourceProviderClient.h"
@@ -21,7 +22,7 @@ using WebKit::WebVector;
 
 namespace content {
 
-RenderAudioSourceProvider::RenderAudioSourceProvider()
+RenderAudioSourceProvider::RenderAudioSourceProvider(int source_render_view_id)
     : is_initialized_(false),
       channels_(0),
       sample_rate_(0),
@@ -44,8 +45,22 @@ RenderAudioSourceProvider::RenderAudioSourceProvider()
   if (use_mixing) {
     default_sink_ = RenderThreadImpl::current()->
         GetAudioRendererMixerManager()->CreateInput();
+    // TODO(miu): Partition mixer instances per RenderView.
   } else {
-    default_sink_ = AudioDeviceFactory::NewOutputDevice();
+    RenderThreadImpl* const render_thread = RenderThreadImpl::current();

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

the convention is:
const RenderThreadImpl* render_thread = ...

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

Actually, that has totally different meaning:

  const RenderThreadImpl* -- RenderThreadImpl is const, but the pointer is
mutable.
  RenderThreadImpl* const -- RenderThreadImpl is mutable, but the pointer is
const.

+    scoped_refptr<media::AudioOutputDevice> device =
+        new media::AudioOutputDevice(render_thread->audio_message_filter(),
+                                     render_thread->GetIOMessageLoopProxy());

[Chris Rogers, 2012/11/29 11:14:32]

I don't understand why the factory has been removed.  The AudioOutputDevice is
supposed to hide its implementation details about using an AudioMessageFilter. 
Now we're exposing these tender details right here, which is not good.  Can't we
pass the |source_render_view_id| to the factory, and then let the factory or the
AudioOutputDevice constructor handle the gory details about calling
AssociateStreamWithProducer()

I understand that there has been some discussion about layering violations where
the "media" code (including AudioOutputDevice) is not supposed to know about the
view ID.  But now the relationships between AudioMessageFilter,
AudioOutputDevice, and this class are becoming way too entangled and messed up. 
I think it's *really* important to get this part right, which means that this
class should keep things simple and only be dealing with a view ID.  Let other
code worry about AudioMessageFilters and the gory details.  This class is just
supposed to be a simple client of AudioOutputDevice.

One solution is to just consider the |source_render_view_id| as a generic ID or
"extra user data".  It's just an integer.  It seems like an AudioOutputDevice
factory can accept a generic "int" ID without considering it to be a layering
violation.  Or the factory itself can be moved outside of the "media" directory.

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

Maybe I'm misunderstanding your point, but AudioOutputDevice accepts an
AudioOutputIPC* in its constructor.  It isn't really aware of
AudioMessageFilter.  AudioMessageFilter just implements AudioOutputIPC.

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

Chris, Henrik and I just talked about this.  I understand where Chris is coming
from and I agree.

So, here's what we propose instead:

* Keep the factory.
* Create a new class in content that inherits from AudioOutputDevice.
  - This new class will have the content specific attributes.
* Change the NewOutputDevice to accept a source_render_view_id.
* In the factory, create an instance of the new class, inject the message filter
and render view id.

There are a few benefits that I can see with this approach:

* The association is kept in the device instance itself instead of in a separate
map in the audio filter (i.e. you can revert that).
* The new class is content specific, so it's fine to use content concepts.
* The media class and interfaces remain unchanged.
* Less clutter/boilerplate code where audio device instances are constructed.

How does that sound?

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

Done.
Done.
Not done...yet.  There's an issue I don't think you guys were considering: In
the case of renderer_webaudiodevice_impl.cc, the source_render_view_id is not
known until start() is called.  Please see my code comments in that file for
more explanation.  It's not clear to me whether it's appropriate (or safe?) to
construct, initialize and start the AudioOutputDevice all within the start()
method.

Now, if it *is* appropriate/safe, that's a game changer.  In this case, it would
make sense to also do this everywhere else (i.e., in
render_audiosourceprovider.cc, webrtc_audio_renderer.cc, etc.).  In other words,
all "audio provider modules" would delay calling
AudioDeviceFactory::NewOutputDevice() until just before Start() and, likewise,
destroy the AudioOutputDevice immediately after Stop().  This would move us one
important step in the direction of http://crbug.com/151051.

What do you think?
Not sure what you mean here.  What separate map are you talking about?
Agreed.  :-)
The subclass (RendererAudioOutputDevice) needs access to the message filter and
stream_id_ from AudioOutputDevice.  Also, to keep things clean, the same
stream_id_ is now re-used for the lifetime of an AudioOutputDevice, rather than
grabbing a new one each time CreateStream() is about to be called.  This is
in-line with the direction we want to go in http://crbug.com/151051.
Agreed.  :-)
PTAL at the latest patchset.

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

I can't think of a reason for why at least the start case wouldn't be safe.
However you might have to jump through some extra hoops for the stop() case
since Stop() is async.  But a step towards fixing 151051 is a good thing!

+    // The RenderView creating RenderAudioSourceProvider will be the source of
+    // the audio (WebMediaPlayer is always associated with a single frame).
+    //
+    // TODO(miu): Unfortunately, there is one use case where this is not true:
+    // JavaScript code can call new Audio() and pass the object around.  The
+    // RenderAudioSourceProvider is not instantiated until the "src" attribute
+    // has been set.
+    render_thread->audio_message_filter()->AssociateStreamWithProducer(
+        device->stream_id(), source_render_view_id);
+    default_sink_ = device;
   }
 }