Associate audio streams with their source/destination RenderView.

content/renderer/media/render_audiosourceprovider.cc

 // 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.
 
 #include "content/renderer/media/render_audiosourceprovider.h"
 
 #include <vector>
 
 #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"
 
 using std::vector;
 using WebKit::WebVector;
 
 namespace content {
 
-RenderAudioSourceProvider::RenderAudioSourceProvider()
+RenderAudioSourceProvider::RenderAudioSourceProvider(int source_render_view_id)
     : is_initialized_(false),
       channels_(0),
       sample_rate_(0),
       is_running_(false),
       renderer_(NULL),
       client_(NULL) {
   // We create an AudioRendererSink here, but we don't yet know the audio format
   // (sample-rate, etc.) at this point.  Later, when Initialize() is called, we
   // have the audio format information and call AudioRendererSink::Initialize()
   // to fully initialize it.
   const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
 #if defined(OS_WIN) || defined(OS_MAC)
   const bool use_mixing =
       !cmd_line->HasSwitch(switches::kDisableRendererSideMixing);
 #else
   const bool use_mixing =
       cmd_line->HasSwitch(switches::kEnableRendererSideMixing);
 #endif
 
   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;
   }
 }
 
 void RenderAudioSourceProvider::setClient(
     WebKit::WebAudioSourceProviderClient* client) {
   // Synchronize with other uses of client_ and default_sink_.
   base::AutoLock auto_lock(sink_lock_);
 
   if (client && client != client_) {
     // Detach the audio renderer from normal playback.
@@ skipping 93 matching lines @@
     // Inform WebKit about the audio stream format.
     client_->setFormat(channels_, sample_rate_);
   }
 
   is_initialized_ = true;
 }
 
 RenderAudioSourceProvider::~RenderAudioSourceProvider() {}
 
 }  // namespace content